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

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.

---
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.

---
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.

---
+ * 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?

---
+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').

---
+                       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;
                                 ^

---
+               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"?

---
+       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?

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/