Re: Added schema level support for publication.

On Tue, Jul 13, 2021 at 2:22 PM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
>
> On Mon, Jul 12, 2021 at 7:24 PM vignesh C <vignesh21(at)gmail(dot)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 fixed in 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;

Modified

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

Modified

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

Modified

> (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))

Modified

> (5) typedefs.list
>
> I think you also need to add "Form_pg_publication_schema" to
typedefs.list.

Modified.

Thanks for the comments, these comments are fixed in the v11 patch posted
at [1].
[1] -
https://www.postgresql.org/message-id/CALDaNm1oZzaEsZC1W8MRNGZ6LWOayC54_UzyRV%2BnCh8w0yW74g%40mail.gmail.com

Regards,
Vignesh