Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: tarantool-patches@freelists.org,
	Kirill Shcherbatov <kshcherbatov@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH v2 4/7] sql: refactor sql_name_from_token to set diag
Date: Thu, 7 Mar 2019 20:34:26 +0300	[thread overview]
Message-ID: <cbcd5ab4-a03b-a44a-652b-5a5999c34ef6@tarantool.org> (raw)
In-Reply-To: <97dde83dd0ad392cfe47a315ce7ad931cb750a4f.1551265819.git.kshcherbatov@tarantool.org>

See 5 comments.

On 27/02/2019 14:13, Kirill Shcherbatov wrote:
> Refactored sql_name_from_token routine to use diag_set in case
> of memory allocation error.
> This change is necessary because the sql_name_from_token body has
> a sqlNameFromToken call that will be changed in subsequent
> patches.
> 
> Needed for #3931
> ---
>   src/box/sql/alter.c   |   8 +-
>   src/box/sql/analyze.c |  47 ++++----
>   src/box/sql/build.c   | 241 +++++++++++++++++++++++-------------------
>   src/box/sql/expr.c    |   2 +-
>   src/box/sql/parse.y   |  15 ++-
>   src/box/sql/pragma.c  |  24 +++--
>   src/box/sql/sqlInt.h  |  35 +++++-
>   src/box/sql/trigger.c |   4 +-
>   8 files changed, 228 insertions(+), 148 deletions(-)
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 4fe838608..2084dbfeb 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -332,7 +320,10 @@ sqlStartTable(Parse *pParse, Token *pName, int noErr)
>   		goto cleanup;
>   	sqlVdbeCountChanges(v);
>   
> -	zName = sqlNameFromToken(db, pName);
> +	assert(pName != NULL);

1. You already have the same assertion in sql_name_from_token.
Why do you need so many new equal assertions? Check other places
like build.c:1792 by yourself, please.

> +	zName = sql_name_from_token(db, pName);
> +	if (zName == NULL)
> +		goto tnt_error;
>   
>   	pParse->sNameToken = *pName;
>   	if (zName == 0)

2. Double check for zName == NULL.

> diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
> index a7224f5c2..1e4ba3518 100644
> --- a/src/box/sql/pragma.c
> +++ b/src/box/sql/pragma.c
> @@ -452,19 +452,27 @@ sqlPragma(Parse * pParse, Token * pId,	/* First part of [schema.]id field */
>   	sqlVdbeRunOnlyOnce(v);
>   	pParse->nMem = 2;
>   
> -	zLeft = sqlNameFromToken(db, pId);
> -	if (!zLeft) {
> +	if (pId == NULL) {
>   		printActivePragmas(user_session);
>   		return;
>   	}
> -
> +	zLeft = sql_name_from_token(db, pId);
> +	if (zLeft == NULL)
> +		goto tnt_error;
>   	if (minusFlag) {
>   		zRight = sqlMPrintf(db, "-%T", pValue);
>   	} else {
> -		zRight = sqlNameFromToken(db, pValue);
> +		if (pValue != NULL) {

3. You can make it shorter writing

     } else if (pValue != NULL) {
         ...
     }

Instead of

     } else {
         if (pValue != NULL) {
             ...
         }
     }

> +			zRight = sql_name_from_token(db, pValue);
> +			if (zRight == NULL)
> +				goto tnt_error;
> +		}
> +	}
> +	if (pValue2 != NULL) {
> +		zTable = sql_name_from_token(db, pValue2);
> +		if (zTable == NULL)
> +			goto tnt_error;
>   	}
> -	zTable = sqlNameFromToken(db, pValue2);
> -
>   	/* Locate the pragma in the lookup table */
>   	pPragma = pragmaLocate(zLeft);
>   	if (pPragma == 0) {
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index eae2ec8e8..10943d3c8 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -3424,7 +3424,20 @@ sql_drop_table(struct Parse *, struct SrcList *, bool, bool);
>   void sqlInsert(Parse *, SrcList *, Select *, IdList *,
>   	       enum on_conflict_action);
>   void *sqlArrayAllocate(sql *, void *, int, int *, int *);
> -IdList *sqlIdListAppend(sql *, IdList *, Token *);
> +
> +/*
> + * Append a new element to the given IdList.  Create a new IdList if
> + * need be.
> + * A new IdList is returned, or NULL if malloc() fails.

4. The last sentence is exactly what you wrote after @retvals.
Please, do not duplicate it.

> + * @param db The database connection.
> + * @param list The pointer to existent Id list if exists.
> + * @param name_token The token containing name.
> + * @retval not NULL IdList pointer is returned on success.
> + * @retval NULL otherwise.

5. Sentences are started from capital letters. I will not write
that again. Please, find and fix other places.

> + */
> +struct IdList *
> +sql_id_list_append(struct sql *db, struct IdList *pList, struct Token *pToken);
> +
>   int sqlIdListIndex(IdList *, const char *);
>   
>   /**

  reply	other threads:[~2019-03-07 17:34 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-27 11:13 [tarantool-patches] [PATCH v2 0/7] sql: store regular identifiers in case-normal form Kirill Shcherbatov
2019-02-27 11:13 ` [tarantool-patches] [PATCH v2 1/7] sql: refactor sql_alloc_src_list to set diag Kirill Shcherbatov
2019-03-07 17:34   ` [tarantool-patches] " Vladislav Shpilevoy
2019-03-11 15:04     ` Kirill Shcherbatov
2019-02-27 11:13 ` [tarantool-patches] [PATCH v2 2/7] sql: rework sql_src_list_enlarge " Kirill Shcherbatov
2019-03-07 17:34   ` [tarantool-patches] " Vladislav Shpilevoy
2019-03-11 15:04     ` Kirill Shcherbatov
2019-02-27 11:13 ` [tarantool-patches] [PATCH v2 3/7] sql: refactor sql_src_list_append " Kirill Shcherbatov
2019-03-07 17:34   ` [tarantool-patches] " Vladislav Shpilevoy
2019-03-11 15:04     ` Kirill Shcherbatov
2019-03-18 19:33       ` Vladislav Shpilevoy
2019-03-20 11:02         ` Kirill Shcherbatov
2019-03-26 17:08           ` Vladislav Shpilevoy
2019-03-26 18:07             ` Vladislav Shpilevoy
2019-02-27 11:13 ` [tarantool-patches] [PATCH v2 4/7] sql: refactor sql_name_from_token " Kirill Shcherbatov
2019-03-07 17:34   ` Vladislav Shpilevoy [this message]
2019-03-11 15:04     ` [tarantool-patches] " Kirill Shcherbatov
2019-03-18 19:33       ` Vladislav Shpilevoy
2019-03-20 11:02         ` Kirill Shcherbatov
2019-02-27 11:13 ` [tarantool-patches] [PATCH v2 5/7] sql: refactor sql_trigger_step_allocate " Kirill Shcherbatov
2019-03-07 17:34   ` [tarantool-patches] " Vladislav Shpilevoy
2019-03-11 15:04     ` Kirill Shcherbatov
2019-03-18 19:33       ` Vladislav Shpilevoy
2019-03-20 11:02         ` Kirill Shcherbatov
2019-03-26 17:08           ` Vladislav Shpilevoy
2019-02-27 11:13 ` [tarantool-patches] [PATCH v2 6/7] sql: refactor sql_expr_create " Kirill Shcherbatov
2019-03-07 17:34   ` [tarantool-patches] " Vladislav Shpilevoy
2019-03-11 15:04     ` Kirill Shcherbatov
2019-03-18 19:33       ` Vladislav Shpilevoy
2019-03-20 11:02         ` Kirill Shcherbatov
2019-03-26 17:08           ` Vladislav Shpilevoy
2019-02-27 11:13 ` [tarantool-patches] [PATCH v2 7/7] sql: store regular identifiers in case-normal form Kirill Shcherbatov
2019-03-07 17:34   ` [tarantool-patches] " Vladislav Shpilevoy
2019-03-11 15:04     ` Kirill Shcherbatov
2019-03-18 19:33       ` Vladislav Shpilevoy
2019-03-20 11:02         ` Kirill Shcherbatov
2019-03-26 17:08           ` Vladislav Shpilevoy
2019-03-18 19:33 ` [tarantool-patches] Re: [PATCH v2 0/7] " Vladislav Shpilevoy
2019-03-20 11:02   ` Kirill Shcherbatov
2019-03-26 17:09     ` Vladislav Shpilevoy
2019-03-27 14:06 ` Kirill Yukhin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cbcd5ab4-a03b-a44a-652b-5a5999c34ef6@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=kshcherbatov@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH v2 4/7] sql: refactor sql_name_from_token to set diag' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox