Re: Support EXCEPT for ALL SEQUENCES publications

Hi Shlok.

Some review comments for patches of v12

//////////
Patch v12-0001
//////////

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

ObjectsInPublicationToOids:

1.
/*
* Convert the PublicationObjSpecType list into schema oid list and
* PublicationRelation list.
*/
static void
ObjectsInPublicationToOids(List *pubobjspec_list, ParseState *pstate,
List **rels, List **excepttbls, List **exceptseqs,
List **schemas)

I think this function comment can be made clearer. IIUC, there are 4
possible outputs from this function but the comment currently only
mentions 2. Also, it might be better to rearrange to make the
description use the same order as the output parameters.

SUGGESTION
Convert the PublicationObjSpecType list into PublicationRelation lists
(`rels`, `excepttbls`, `exceptseqs`) and a schema oid list
(`schemas`).

~~~

CreatePublication:

2.
+ if (stmt->for_all_sequences)
+ {
+ /* Process EXCEPT sequence list */
+ if (exceptseqs != NIL)
+ {
+ List *rels = OpenRelationList(exceptseqs);
+
+ PublicationAddRelations(puboid, rels, true, NULL, RELKIND_SEQUENCE);
+ CloseRelationList(rels);
+ }
+ }

if (stmt->for_all_tables)
{
/* Process EXCEPT table list */
- if (exceptrelations != NIL)
+ if (excepttbls != NIL)
{
List *rels;

- rels = OpenTableList(exceptrelations);
- PublicationAddTables(puboid, rels, true, NULL);
- CloseTableList(rels);
+ rels = OpenRelationList(excepttbls);
+ PublicationAddRelations(puboid, rels, true, NULL, RELKIND_RELATION);
+ CloseRelationList(rels);

Here the 1st `rels` declaration does the assignment at the same time,
but the 2nd one does not. These code fragment are almost identical, so
let's make the declarations/assignments consistent.

~~~

3.
/* EXCEPT clause is not supported with ALTER PUBLICATION */
Assert(exceptseqs == NIL);
The comment is a bit misleading because:
- AFAIK this is only temporary for patch 0001.
- It is only EXCEPT (SEQUENCE ...) that is not supported yet; not
every EXCEPT clause.

SUGGESTION
TODO - The EXCEPT (SEQUENCE ...) clause is not yet supported with
ALTER PUBLICATION

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

4.
static void
preprocess_pub_all_objtype_list(List *all_objects_list, List **pubobjects,
bool *all_tables, bool *all_sequences,
core_yyscan_t yyscanner)
{
if (!all_objects_list)
return;

*all_tables = false;
*all_sequences = false;

Shouldn't those assignments precede the "if (!all_objects_list)",
otherwise if the booleans are not set before the check you are relying
too much that the caller's makeNode() had set them already false on
entry.

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

5.
+ /* Include EXCEPT (SEQUENCE) clause if there are except_sequences. */
+ for (SimplePtrListCell *cell = pubinfo->except_sequences.head; cell;
cell = cell->next)
+ {
+ TableInfo *tbinfo = (TableInfo *) cell->ptr;
+
+ if (++n_except == 1)
+ appendPQExpBufferStr(query, " EXCEPT (");
+ else
+ appendPQExpBufferStr(query, ", ");
+ appendPQExpBuffer(query, "SEQUENCE %s", fmtQualifiedDumpable(tbinfo));
+ }

Unlike tables which might have qualifiers 'ONLY' and '*', there are no
additional qualifier needed for sequences, so perhaps it is better to
just generate one excluded list:

e.g.
EXCEPT (SEQUENCE a, b, c, d, e);
instead of
EXCEPT (SEQUENCE a, SEQUENCE b, SEQUENCE c, SEQUENCE d, SEQUENCE e);

~

SUGGESTION:
seqname = fmtQualifiedDumpable(tbinfo);
if (++n_except == 1)
appendPQExpBufferStr(query, " EXCEPT (SEQUENCE %s", seqname);
else
appendPQExpBufferStr(query, ", %s", seqname);

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

6.
+ if (pset.sversion >= 190000)
+ {
+ appendPQExpBuffer(&buf, "\n AND NOT EXISTS (\n"
+ " SELECT 1\n"
+ " FROM pg_catalog.pg_publication_rel pr\n"
+ " WHERE pr.prpubid = p.oid AND\n"
+ " pr.prrelid = '%s')", oid);

IMO the first line should be split to make it more readable. Also the
other parts of this SQL statement put the \n at the beginning, so
maybe it is better to do same here too.

SUGGESTION
appendPQExpBuffer(&buf,
"\n AND NOT EXISTS ("
"\n SELECT 1"
"\n FROM pg_catalog.pg_publication_rel pr"
"\n WHERE pr.prpubid = p.oid AND"
"\n pr.prrelid = '%s')", oid);

//////////
Patch v12-0002.
//////////

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

AlterPublicationRelations:

1.
/*
- * In FOR ALL TABLES mode, relations are tracked as exclusions
- * (EXCEPT clause). Fetch the current excluded relations so they
- * can be reconciled with the specified EXCEPT list.
+ * In FOR ALL TABLES/ SEQUENCES mode, relations are tracked as
+ * exclusions (EXCEPT clause). Fetch the current excluded
+ * relations so they can be reconciled with the specified EXCEPT
+ * list.
*
* This applies only if the existing publication is already
- * defined as FOR ALL TABLES; otherwise, there are no exclusion
- * entries to process.
+ * defined as FOR ALL TABLES/ FOR ALL SEQUENCES; otherwise, there
+ * are no exclusion entries to process.
*/

One place refers to "FOR ALL TABLES/ SEQUENCES" and another says "FOR
ALL TABLES/ FOR ALL SEQUENCES". Should use consistent terminology. I
preferred the 2nd variant.

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