Re: Logical Replication of sequences - Mailing list pgsql-hackers

On Fri, Jul 5, 2024 at 9:58 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Thu, 4 Jul 2024 at 12:44, Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > 1.
> > Should there be some new test for the view? Otherwise, AFAICT this
> > patch has no tests that will exercise the new function
> > pg_get_publication_sequences.
>
> pg_publication_sequences view uses pg_get_publication_sequences which
> will be tested with 3rd patch while creating subscription/refreshing
> publication sequences. I felt it is ok not to have a test here.
>

OTOH, if there had been such a test here then the ("sequence = NIL")
bug in patch 0002 code would have been caught earlier in patch 0002
testing instead of later in patch 0003 testing. In general, I think
each patch should be self-contained w.r.t. to testing all of its new
code, but if you think another test here is overkill then I am fine
with that too.

//////////

Meanwhile, here are my review comments for patch v20240705-0002

======
doc/src/sgml/ref/create_publication.sgml

1.
The CREATE PUBLICATION page has many examples showing many different
combinations of syntax. I think it would not hurt to add another one
showing SEQUENCES being used.

======
src/backend/commands/publicationcmds.c

2.
+ if (form->puballsequences && !superuser_arg(newOwnerId))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied to change owner of publication \"%s\"",
+ NameStr(form->pubname)),
+ errhint("The owner of a FOR ALL SEQUENCES publication must be a
superuser.")));

You might consider combining this with the previous error in the same
way that the "FOR ALL TABLES" and "FOR ALL SEQUENCES" errors were
combined in CreatePublication. The result would be less code. But, I
also think your current code is fine, so I am just putting this out as
an idea in case you prefer it.

======
src/backend/parser/gram.y

nitpick - added a space in the comment
nitpick - changed the call order slightly because $6 comes before $7

======
src/bin/pg_dump/pg_dump.c

3. getPublications

- if (fout->remoteVersion >= 130000)
+ if (fout->remoteVersion >= 170000)

This should be 180000.

======
src/bin/psql/describe.c

4. describeOneTableDetails

+ /* print any publications */
+ if (pset.sversion >= 170000)
+ {

This should be 180000.

~~~

describeOneTableDetails:
nitpick - removed a redundant "else"
nitpick - simplified the "Publications:" header logic slightly

~~~

5. listPublications

+ if (pset.sversion >= 170000)
+ appendPQExpBuffer(&buf,
+   ",\n  puballsequences AS \"%s\"",
+   gettext_noop("All sequences"));

This should be 180000.

~~~

6. describePublications

+ has_pubsequence = (pset.sversion >= 170000);

This should be 180000.

~

nitpick - remove some blank lines for consistency with nearby code

======
src/include/nodes/parsenodes.h

nitpick - minor change to comment for PublicationAllObjType
nitpick - the meanings of the enums are self-evident; I didn't think
comments were very useful

======
src/test/regress/sql/publication.sql

7.
I think it will also be helpful to arrange for a SEQUENCE to be
published by *multiple* publications. This would test that they get
listed as expected in the "Publications:" part of the "describe" (\d+)
for the sequence.

======
99.
Please also see the attached diffs patch which implements any nitpicks
mentioned above.

======
Kind Regards,
Peter Smith.
Fujitsu Australia