Re: Added schema level support for publication. - Mailing list pgsql-hackers

On Mon, Aug 30, 2021 at 9:10 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Friday, August 27, 2021 2:13 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > I have implemented this in the 0003 patch, I have kept it separate to reduce the
> > testing effort and also it will be easier if someone disagrees with the syntax. I
> > will merge it to the main patch later based on the feedback. Attached v22 patch
> > has the changes for the same.
> > Thoughts?
>
> Hi,
>
> Here are some comments for the new version patches.
>
> About  0001
> 1)
> +                       rel->relpersistence = RELPERSISTENCE_PERMANENT;
>
> It seems we don't need to set this since makeRangeVarFromNameList()
> already set it.

Modified

> 2)
> +                       if (!relids || !schemarelids)
> +                               tables = list_concat(relids, schemarelids);
> +                       else
> +                               tables = list_concat_unique_oid(relids, schemarelids);
> +               }
>
> It seems we can simplify the above code like the following:
> tables = list_concat_unique_oid(relids, schemarelids);

Modified

> 3)
> +               relids = GetPublicationRelations(pubform->oid,
> +                                                                                PUBLICATION_PART_ALL);
> +               schemarelids = GetAllSchemasPublicationRelations(pubform->oid,
> +
PUBLICATION_PART_ALL);
> +               relids = list_concat(relids, schemarelids);
>
> should we invoke list_concat_unique_oid here ?

Modified

> 4)
>
> +                               search_path = fetch_search_path(false);
> +                               if (search_path == NIL) /* nothing valid in search_path? */
>
> It might be better to list_free(search_path) when not used.

Modified

> 5)
> +                       if (list_length(pubobj->name) == 1 &&
> +                               (strcmp(relname, "CURRENT_SCHEMA") == 0))
> +                               ereport(ERROR,
> +                                               errcode(ERRCODE_SYNTAX_ERROR),
> +                                               errmsg("invalid relation name at or near"),
> +                                               parser_errposition(pstate, pubobj->location));
>
> Maybe we don't need this check, because it will report an error in
> OpenTableList() anyway, "relation "CURRENT_SCHEMA" does not exist" , and that
> message seems readable to me.

Allowing CURRENT_SCHEMA is required to support current schema for
schema publications, currently I'm allowing this syntax during parsing
and this error is thrown for relations later, this is done to keep the
similar error as earlier before this feature support. I felt we can
keep it like this to maintain the similar error. Thoughts?

> About  0002
> 6)
>
> diff --git a/src/test/subscription/t/001_rep_changes.pl b/src/test/subscription/t/001_rep_changes.pl
> index 0c84d87873..0a479dfe36 100644
> --- a/src/test/subscription/t/001_rep_changes.pl
> +++ b/src/test/subscription/t/001_rep_changes.pl
> @@ -6,7 +6,7 @@ use strict;
>  use warnings;
>  use PostgresNode;
>  use TestLib;
> -use Test::More tests => 32;
> +use Test::More tests => 46;
>
> I think it might be better to move these testcases create a separate perl file.

Modified, added to 025_rep_changes_for_schema.pl

> About  0003
> 7)
> The v22-0003 seems simple and can remove lots of code in patch v22-0001, so
> maybe we can merge 0001 and 0003 into one patch ?

I agree that the code becomes simpler, it reduces a lot of code. I had
kept it like that as the testing effort might be more and also I was
waiting if there was no objection for that syntax from anyone else. I
will wait for a few more reviews and merge it to 0001 if there are no
objections.

I have fixed these comments as part of v23 patch attached at [1].
[1] - https://www.postgresql.org/message-id/CALDaNm0xmqJeQEfV5Wnj2BawM%3DsdFdfOXz5N%2BgGG3WB6k9%3Dtdw%40mail.gmail.com

Regards,
Vignesh