From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 42C6D231BB for ; Wed, 11 Sep 2019 09:32:10 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id hsqpERYwa9cJ for ; Wed, 11 Sep 2019 09:32:10 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 01F4A23065 for ; Wed, 11 Sep 2019 09:32:10 -0400 (EDT) Date: Wed, 11 Sep 2019 16:32:06 +0300 From: Nikita Pettik Subject: [tarantool-patches] Re: [PATCH] sql: add check for absence in Message-ID: <20190911133203.GA49615@tarantool.org> References: <20190802125252.54621-1-roman.habibov@tarantool.org> <6B834CCD-6A77-42D0-8AAD-B1BD2B8F51A6@tarantool.org> <8BD992A5-0A74-4E60-A239-5FDE85783467@tarantool.org> <326482ED-8190-4B72-B2B8-F20763A86F7E@tarantool.org> <1B494CAB-704F-4A62-A00D-257388DDB2B8@tarantool.org> <20190829175955.GA52650@tarantool.org> <0FDC0655-79B5-479F-8FAA-5E9A2E93F1CF@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <0FDC0655-79B5-479F-8FAA-5E9A2E93F1CF@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: roman.habibov@tarantool.org On Wed, Sep 04, 2019 at 05:14:10PM +0300, Roman Khabibov wrote: > > commit 4f78ff7cf8b4a3f496d72388235f773ae51d6efc > Author: Roman Khabibov > Date: Mon Jul 29 18:00:34 2019 +0300 > > sql: allow to create view as clause > > Allow views to use CTEs in clauses, Nit: all CTEs start from WITH clause; so your statement is a bit confusing. > which can be in any > (nested) select after . Before this patch, during view > creation all referenced spaces were fetched by name from SELECT > and their reference counters were incremented to avoid dangling > references. It occurred in update_view_references(). Obviously, CTE > tables weren't held in space cache, ergo error "space doesn’t > exist" was raised. Add check if a space from FROM is CTE. If it is, > don't increment its reference counter and don't raise the error. > > Closes #4149 > > diff --git a/src/box/alter.cc b/src/box/alter.cc > index 4f2e34bf0..a625d8c39 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -1759,6 +1759,13 @@ update_view_references(struct Select *select, int update_value, > const char *space_name = sql_src_list_entry_name(list, i); > if (space_name == NULL) > continue; > + /* > + * CTE tables isn't held in space cache and we Nit: tables aren't held ... > + * don't need to increment reference counter for > + * CTEs. I'd rephrase a bit: Views are allowed to contain CTEs. CTE is a temporary object, created and destroyed at SQL runtime (it is represented by an ephemeral table). So, it is absent in space cache and as a consequence we can't increment its reference counter. Skip iteration. > + */ > + if (sql_select_has_cte_with_name(select, space_name)) > + continue; > struct space *space = space_by_name(space_name); > if (space == NULL) { > if (! suppress_error) { > diff --git a/src/box/sql.h b/src/box/sql.h > index 7644051b4..422c8dfc2 100644 > --- a/src/box/sql.h > +++ b/src/box/sql.h > @@ -323,6 +323,17 @@ sql_select_delete(struct sql *db, struct Select *select); > struct SrcList * > sql_select_expand_from_tables(struct Select *select); > > +/** > + * Check if @a name matches with at least one of CTE names typed > + * in clauses within @a select. > + * > + * @param select Select which may contain CTE. Nit: you forgot to describe name argument. > + * @retval true Has CTE with @a name. > + * @retval false Hasn't CTE with @a name. > +*/ > +bool > +sql_select_has_cte_with_name(struct Select *select, const char *name); sql_select_contains_cte() is enough, I guess. > + > /** > * Temporary getter in order to avoid including sqlInt.h > * in alter.cc. > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index ddb5de87e..d7dab35cd 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -332,6 +332,29 @@ sql_select_expand_from_tables(struct Select *select) > return walker.u.pSrcList; > } > > +bool > +sql_select_has_cte_with_name(struct Select *select, const char *name) > +{ > + assert(select != NULL && name != NULL); > + struct With *with = select->pWith; > + if (with != NULL) { > + for (int i = 0; i < with->nCte; i++) { > + const struct Cte *cte = &with->a[i]; > + if (memcmp(name, cte->zName, strlen(name)) == 0) > + return true; > + } > + } Please explain why we don't use recursion here (cte->pSelect->pWith). I'd attach schema of SELECT for the sake of the clarity. > + struct SrcList *list = select->pSrc; > + int item_count = sql_src_list_entry_count(list); > + for (int i = 0; i < item_count; ++i) { > + if (list->a[i].pSelect != NULL) > + if (sql_select_has_cte_with_name(list->a[i].pSelect, > + name)) > + return true; > + Nit: in case 'if' body contains more than one string it is enclosed in brackets.