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

On Tue, Jul 13, 2021 at 2:22 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> 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 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