Re: Added schema level support for publication.
От | vignesh C |
---|---|
Тема | Re: Added schema level support for publication. |
Дата | |
Msg-id | CALDaNm3BMLBpWOSdS3Q2vwpsM=0yovpJm8dKHRqNyFpANbrhpw@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Added schema level support for publication. (Amit Kapila <amit.kapila16@gmail.com>) |
Ответы |
Re: Added schema level support for publication.
(Amit Kapila <amit.kapila16@gmail.com>)
Re: Added schema level support for publication. (Mark Dilger <mark.dilger@enterprisedb.com>) Re: Added schema level support for publication. (Greg Nancarrow <gregn4422@gmail.com>) Re: Added schema level support for publication. (Masahiko Sawada <sawada.mshk@gmail.com>) |
Список | 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
Вложения
В списке pgsql-hackers по дате отправления: