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

On Wed, Aug 4, 2021 at 4:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Aug 3, 2021 at 8:38 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Thanks for reporting this, this is fixed in the v18 patch attached.
> >
>
> I have started looking into this patch and below are some initial comments.
>
> 1.
> + /* Fetch publication name and schema oid from input list */
> + schemaname = strVal(linitial(object));
> + pubname = strVal(lsecond(object));
>
> I think the comment should be: "Fetch schema name and publication name
> from input list"
>

Modified.

> 2.
> @@ -3902,6 +3958,46 @@ getObjectDescription(const ObjectAddress
> *object, bool missing_ok)
>   break;
>   }
>
> + case OCLASS_PUBLICATION_SCHEMA:
> + {
> + HeapTuple tup;
> + char    *pubname;
> + Form_pg_publication_schema psform;
> + char    *nspname;
> +
> + tup = SearchSysCache1(PUBLICATIONSCHEMA,
> +   ObjectIdGetDatum(object->objectId));
> + if (!HeapTupleIsValid(tup))
> + {
> + if (!missing_ok)
> + elog(ERROR, "cache lookup failed for publication schema %u",
> + object->objectId);
> + break;
> + }
> +
> + 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;
> + }
>
> The above code in getObjectDescription looks quite similar to what you
> have in getObjectIdentityParts(). Can we extract the common code into
> a separate function?

Modified.

> 3. Can we use column name pubkind (similar to relkind in pg_class)
> instead of pubtype? If so, please change PUBTYPE_ALLTABLES and similar
> other defines to PUBKIND_*.
>

Modified.

> 4.
> @@ -3632,6 +3650,7 @@ typedef struct CreatePublicationStmt
>   List    *options; /* List of DefElem nodes */
>   List    *tables; /* Optional list of tables to add */
>   bool for_all_tables; /* Special publication for all tables in db */
> + List    *schemas; /* Optional list of schemas */
>  } CreatePublicationStmt;
>
> Isn't it better to keep a schemas list after tables?

Modified.

> 5.
> @@ -1163,12 +1168,27 @@ get_rel_sync_entry(PGOutputData *data, Oid relid)
>   Publication *pub = lfirst(lc);
>   bool publish = false;
>
> - if (pub->alltables)
> + if (pub->pubtype == PUBTYPE_ALLTABLES)
>   {
>   publish = true;
>   if (pub->pubviaroot && am_partition)
>   publish_as_relid = llast_oid(get_partition_ancestors(relid));
>   }
> + else if (pub->pubtype == PUBTYPE_SCHEMA)
> + {
> + Oid schemaId = get_rel_namespace(relid);
> + Oid psid = GetSysCacheOid2(PUBLICATIONSCHEMAMAP,
> +    Anum_pg_publication_schema_oid,
> +    ObjectIdGetDatum(schemaId),
> +    ObjectIdGetDatum(pub->oid));
> +
> + if (OidIsValid(psid))
> + {
> + publish = true;
> + if (pub->pubviaroot && am_partition)
> + publish_as_relid = llast_oid(get_partition_ancestors(relid));
> + }
> + }
>
> Isn't it better to get schema publications once as we get relation
> publications via GetRelationPublications and then decide whether to
> publish or not? I think that will save repeated cache searches for
> each publication requested by the subscriber?

Modified.

> 6.
> + {PublicationSchemaRelationId, /* PUBLICATIONSCHEMAMAP */
> + PublicationSchemaPsnspcidPspubidIndexId,
> + 2,
> + {
> + Anum_pg_publication_schema_psnspcid,
> + Anum_pg_publication_schema_pspubid,
> + 0,
> + 0
> + },
>
> Why don't we keep pubid as the first column in this index?

I wanted to keep it similar to PUBLICATIONRELMAP, should we keep it as
it is, thoughts?

> 7.
> getPublicationSchemas()
> {
> ..
> + /* Get the publication membership for the schema */
> + printfPQExpBuffer(query,
> +   "SELECT ps.psnspcid, ps.oid, p.pubname, p.oid AS pubid "
> +   "FROM pg_publication_schema ps, pg_publication p "
> +   "WHERE ps.psnspcid = '%u' "
> +   "AND p.oid = ps.pspubid",
> +   nsinfo->dobj.catId.oid);
> ..
> }
>
> Why do you need to use join here? Why the query and another handling
> in this function be similar to what we have in getPublicationTables?
> Also, there is another function GetPublicationSchemas() in the patch,
> can we name one of these differently for the purpose of easy
> grepability?

Modified it similar to getPublicationTables without joins. The
function is renamed to getPublicationNamespaces.

Thanks for the comments, the attached v19 patch has the fixes for the comments.

Regards,
Vignesh