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

From Greg Nancarrow
Subject Re: Added schema level support for publication.
Date
Msg-id CAJcOf-f-72zS-uFFJ_R6W-rtJwUOQrDD+DiCWmhiMxhNck7LnQ@mail.gmail.com
Whole thread Raw
In response to Re: Added schema level support for publication.  (vignesh C <vignesh21@gmail.com>)
Responses Re: Added schema level support for publication.
List pgsql-hackers
On Mon, Jul 12, 2021 at 7:24 PM vignesh C <vignesh21@gmail.com> wrote:
>
>
> Thanks for reporting this issue. I felt this issue is the same as the issue which Hou San had reported. This issue is
fixedin the v10 patch attached at [1].
 
> [1] - https://www.postgresql.org/message-id/CALDaNm2%2BtR%2B8R-sD1CSyMbZcZbkintZE-avefjsp7LCkm6HMmw%40mail.gmail.com
>

I did some testing and the issue that I reported does seem to be fixed
by the v10 patch.

I have some patch review comments for the v10 patch:

(1)

The following:

+ if (!OidIsValid(address.objectId))
+ {
+    if (!missing_ok)
+       ereport(ERROR,
+          (errcode(ERRCODE_UNDEFINED_OBJECT),
+          errmsg("publication schema \"%s\" in publication \"%s\"
does not exist",
+             schemaname, pubname)));
+    return address;
+ }
+
+ return address;

could just be simplified to:

+ if (!OidIsValid(address.objectId) && !missing_ok)
+ {
+    ereport(ERROR,
+       (errcode(ERRCODE_UNDEFINED_OBJECT),
+       errmsg("publication schema \"%s\" in publication \"%s\" does not exist",
+          schemaname, pubname)));
+ }
+
+ return address;


(2) src/backend/catalog/objectaddress.c

I think there is a potential illegal memory access (psform->psnspcid)
in the case of "!missing_ok", as the tuple is released from the cache
on the previous line.

+ psform = (Form_pg_publication_schema) GETSTRUCT(tup);
+ pubname = get_publication_name(psform->pspubid, false);
+ nspname = get_namespace_name(psform->psnspcid);
+ if (!nspname)
+ {
+    pfree(pubname);
+    ReleaseSysCache(tup);
+    if (!missing_ok)
+       elog(ERROR, "cache lookup failed for schema %u",
+          psform->psnspcid);
+    break;
+ }


I think this should be:

+ psform = (Form_pg_publication_schema) GETSTRUCT(tup);
+ pubname = get_publication_name(psform->pspubid, false);
+ nspname = get_namespace_name(psform->psnspcid);
+ if (!nspname)
+ {
+    Oid psnspcid = psform->psnspcid;
+
+    pfree(pubname);
+    ReleaseSysCache(tup);
+    if (!missing_ok)
+       elog(ERROR, "cache lookup failed for schema %u",
+          psnspcid);
+    break;
+ }

There are two cases of this that need correction (see: case
OCLASS_PUBLICATION_SCHEMA).

(3) Incomplete function header comment

+ * makeSchemaSpec - Create a SchemaSpec with the given type

Should be:

+ * makeSchemaSpec - Create a SchemaSpec with the given type and location


(4) src/bin/psql/describe.c

Shouldn't the following comment say "version 15"?

+ /* Prior to version 14 check was based on all tables */
+ if ((has_pubtype && pubtype == PUBTYPE_TABLE) ||
+ (!has_pubtype && !puballtables))


(5) typedefs.list

I think you also need to add "Form_pg_publication_schema" to typedefs.list.


Regards,
Greg Nancarrow
Fujitsu Australia



pgsql-hackers by date:

Previous
From: gkokolatos@pm.me
Date:
Subject: Re: Introduce pg_receivewal gzip compression tests
Next
From: Magnus Hagander
Date:
Subject: Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?