On Fri, Oct 22, 2021 at 10:55 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Oct 21, 2021 at 6:47 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> >
> > Thanks for the comments, the attached v45 patch has the fix for the same.
> >
>
> The first patch is mostly looking good to me apart from the below
> minor comments:
>
> 1.
> + <para>
> + The catalog <structname>pg_publication_namespace</structname> contains the
> + mapping between schemas and publications in the database. This is a
> + many-to-many mapping.
>
> There are extra spaces after mapping at the end which are not required.
Modified
> 2.
> + <literal>CREATE</literal> privilege on the database. Also, the new owner
> + of a <literal>FOR ALL TABLES</literal> publication must be a superuser.
>
> I think we can modify the second line as: "Also, the new owner of a
> <literal>FOR ALL TABLES</literal> or <literal>FOR ALL TABLES IN
> SCHEMA</literal> publication must be a superuser.
Modified
> 3.
> /table's schema as part of specified schema is not supported./table's
> schema as part of the specified schema is not supported.
Modified
> 4.
> + <para>
> + Create a publication that publishes all changes for tables
> + <structname>users</structname>, <structname>departments</structname> and
> + that publishes all changes for all the tables present in the schema
> + <structname>production</structname>:
>
> I don't think '...that publishes...' is required twice in the above sentence.
Modified
> 5.
> +static List *OpenReliIdList(List *relids);
> static List *OpenTableList(List *tables);
> static void CloseTableList(List *rels);
> static void PublicationAddTables(Oid pubid, List *rels, bool if_not_exists,
> AlterPublicationStmt *stmt);
> static void PublicationDropTables(Oid pubid, List *rels, bool missing_ok);
> +static void LockSchemaList(List *schemalist);
> +static void PublicationAddSchemas(Oid pubid, List *schemas, bool if_not_exists,
> + AlterPublicationStmt *stmt);
> +static void PublicationDropSchemas(Oid pubid, List *schemas, bool missing_ok);
>
> Keep the later definitions also in this order. I suggest move
> LockSchemaList() just after CloseTableList() both in declaration and
> definition.
Modified
> 6.
> +/*
> + * Convert the PublicationObjSpecType list into schema oid list and rangevar
> + * list.
> + */
>
> I think you need to say publication table instead of rangevar in the
> above comment.
Modified
> 7.
> + /*
> + * It is quite possible that for the SET case user has not specified any
> + * schema in which case we need to remove all the existing schemas.
> + */
>
> /schema/schemas
Modified
> 8.
> +/*
> + * Open relations specified by a RangeVar list.
>
> /RangeVar/PublicationTable
Modified
> 9.
> +static bool
> +_equalPublicationObject(const PublicationObjSpec *a,
> + const PublicationObjSpec *b)
> +{
> + COMPARE_SCALAR_FIELD(pubobjtype);
> + COMPARE_STRING_FIELD(name);
> + COMPARE_NODE_FIELD(pubtable);
> + COMPARE_LOCATION_FIELD(location);
> +
> + return true;
> +}
> +
>
> Let's define this immediately before _equalPublicationTable as all
> publication functions are defined there. Also, make the handling of
> T_PublicationObjSpec before T_PublicationTable in equal() function as
> that is the way nodes are defined.
Modified
Thanks for the comments, attached v46 patch has the fix for the same.
Regards,
Vignesh
