Re: Added schema level support for publication.

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