Re: Added schema level support for publication.
От | vignesh C |
---|---|
Тема | Re: Added schema level support for publication. |
Дата | |
Msg-id | CALDaNm1ZUOHioJm7yr_7b0Rc_obV3pTAZNkUwzmPNzu7chqDqg@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Added schema level support for publication. (Masahiko Sawada <sawada.mshk@gmail.com>) |
Список | pgsql-hackers |
On Wed, Sep 22, 2021 at 8:52 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Wed, Sep 22, 2021 at 3:02 AM vignesh C <vignesh21@gmail.com> wrote: > > > > > > Attached v30 patch has the fixes for the same. > > > > Thank you for updating the patches. > > Here are random comments on v30-0002 patch: > > + > + if (stmt->action == DEFELEM_SET && > !list_length(schemaidlist)) > + { > + delschemas = > GetPublicationSchemas(pubform->oid); > + LockSchemaList(delschemas); > + } > > I think "list_length(schemaidlist) > 0" would be more readable. We have refactored the patch which has removed these changes. > --- > This patch introduces some new static functions to publicationcmds.c > but some have function prototypes but some don't. As far as I checked, > > ObjectsInPublicationToOids() > CheckObjSchemaNotAlreadyInPublication() > GetAlterPublicationDelRelations() > AlterPublicationSchemas() > CheckPublicationAlterTables() > CheckPublicationAlterSchemas() > LockSchemaList() > OpenReliIdList() > PublicationAddSchemas() > PublicationDropSchemas() > > are newly introduced but only four functions: > > OpenReliIdList() > LockSchemaList() > PublicationAddSchemas() > PublicationDropSchemas() > > have function prototypes. Actually, there already are functions that > don't have their prototype in publicationcmds.c. But it seems better > to clarify what kind of functions don't need to have a prototype at > least in this file. My thoughts are the same as that Amit had replied at [1]. > --- > ISTM we can inline the contents of three functions: > GetAlterPublicationDelRelations(), CheckPublicationAlterTables(), and > CheckPublicationAlterSchemas(). These have only one caller and ISTM > makes the readability worse. I think it's not necessary to make > functions for them. We have refactored the patch which has removed these changes. > --- > + * This is dispatcher function for AlterPublicationOptions, > + * AlterPublicationSchemas and AlterPublicationTables. > > As this comment mentioned, AlterPublication() calls > AlterPublicationTables() and AlterPublicationSchemas() but this > function also a lot of pre-processing such as building the list and > some checks, depending on stmt->action before calling these two > functions. And those two functions also perform some operation > depending on stmt->action. So ISTM it's better to move those > pre-processing to these two functions and have AlterPublication() just > call these two functions. What do you think? We have refactored the patch which has removed these changes. > --- > +List * > +GetAllSchemasPublicationRelations(Oid puboid, PublicationPartOpt pub_partopt) > > Since this function gets all relations in the schema publication, I > think GetAllSchemaPublicationRelations() would be better as a function > name (removing 's' before 'P'). Modified > --- > + if (!IsA(node, String)) > + ereport(ERROR, > + errcode(ERRCODE_SYNTAX_ERROR), > + errmsg("invalid schema > name at or near"), > + > parser_errposition(pstate, pubobj->location)); > > The error message should mention where the invalid schema name is at > or near. Also, In the following example, the error position in the > error message seems not to be where the invalid schemaname s.s is: > postgres(1:47707)=# create publication p for all tables in schema s.s; > ERROR: invalid schema name at or near > LINE 1: create publication p for all tables in schema s.s; > ^ Modified > --- > + if (pubobj->pubobjtype == PUBLICATIONOBJ_TABLE) > + { > + if (IsA(node, RangeVar)) > + *rels = lappend(*rels, (RangeVar *) node); > + else if (IsA(node, String)) > + { > + RangeVar *rel = makeRangeVar(NULL, > strVal(node), > + > pubobj->location); > + > + *rels = lappend(*rels, rel); > + } > + } > + else if (pubobj->pubobjtype == PUBLICATIONOBJ_REL_IN_SCHEMA) > + { > (snip) > + /* Filter out duplicates if user specifies > "sch1, sch1" */ > + *schemas = list_append_unique_oid(*schemas, schemaid); > + } > > Do we need to filter out duplicates also in PUBLICATIONOBJ_TABLE case > since users can specify things like "TABLE tbl, tbl, tbl"? Currently the handling of tables is taken care at OpenTableList: ..... /* * Filter out duplicates if user specifies "foo, foo". * * Note that this algorithm is known to not be very efficient (O(N^2)) * but given that it only works on list of tables given to us by user * it's deemed acceptable. */ if (list_member_oid(relids, myrelid)) { table_close(rel, ShareUpdateExclusiveLock); continue; } ..... > --- > + if ((action == DEFELEM_ADD || action == DEFELEM_SET) && !superuser()) > + ereport(ERROR, > + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), > + errmsg("must be superuser to add or > set schemas"))); > > Why do we require the superuser privilege only for ADD and SET but not for DROP? Amit had replied with the comments for this at [1]. The v32 patch attached at [2] has the fixes for the above. [1] - https://www.postgresql.org/message-id/CAA4eK1KmccaVdKFrwKLXhewDt6rSCD2msOoYbWWWxYK5%3DbX5cg%40mail.gmail.com [2] - https://www.postgresql.org/message-id/CALDaNm1R-xbQvz4LU5OXu3KKwbWOz3uDcT_YjRU6V0R5FZDYDg%40mail.gmail.com Regards, Vignesh
В списке pgsql-hackers по дате отправления: