On Tue, Dec 21, 2021 at 9:03 PM vignesh C <vignes...@gmail.com> wrote:
>
...
> Few comments:
> 1) list_free(schemarelids) is called inside if and and outside if in
> break case. Can we move it above continue so that it does not gets
> called in the break case:
> + schemarelids =
> GetAllSchemaPublicationRelations(pub->oid,
> +
> pub->pubviaroot ?
> +
> PUBLICATION_PART_ROOT
> :
> +
>
> PUBLICATION_PART_LEAF);
> + if (list_member_oid(schemarelids, entry->relid))
> + {
> + if (pub->pubactions.pubinsert)
> + no_filter[idx_ins] = true;
> + if (pub->pubactions.pubupdate)
> + no_filter[idx_upd] = true;
> + if (pub->pubactions.pubdelete)
> + no_filter[idx_del] = true;
> +
> + list_free(schemarelids);
> +
> + /* Quick exit loop if all pubactions
> have no row-filter. */
> + if (no_filter[idx_ins] &&
> no_filter[idx_upd] && no_filter[idx_del])
> + break;
> +
> + continue;
> + }
> + list_free(schemarelids);
>
I think this review comment is mistaken. The break will break from the
loop, so the free is still needed. So I skipped this comment. If you
still think there is a problem please give a more detailed explanation
about it.
> 2) create_edata_for_relation also is using similar logic, can it also
> call this function to reduce duplicate code
> +static EState *
> +create_estate_for_relation(Relation rel)
> +{
> + EState *estate;
> + RangeTblEntry *rte;
> +
> + estate = CreateExecutorState();
> +
> + rte = makeNode(RangeTblEntry);
> + rte->rtekind = RTE_RELATION;
> + rte->relid = RelationGetRelid(rel);
> + rte->relkind = rel->rd_rel->relkind;
> + rte->rellockmode = AccessShareLock;
> + ExecInitRangeTable(estate, list_make1(rte));
> +
> + estate->es_output_cid = GetCurrentCommandId(false);
> +
> + return estate;
> +}
>
Yes, that other code looks similar, but I am not sure it is worth
rearranging things for the sake of trying to make use of only 5 or 6
common LOC. Anyway, I felt this review comment is not really related
to the RF patch; It seems more like a potential idea for a future
patch to use some common code *after* the RF code is committed. So I
skipped this comment.
> 3) In one place select is in lower case, it can be changed to upper case
> + resetStringInfo(&cmd);
> + appendStringInfo(&cmd,
> + "SELECT DISTINCT
> pg_get_expr(prqual, prrelid) "
> + " FROM pg_publication p "
> + " INNER JOIN
> pg_publication_rel pr ON (p.oid = pr.prpubid) "
> + " WHERE pr.prrelid
> = %u AND p.pubname IN ( %s ) "
> + " AND NOT (select
> bool_or(puballtables) "
> + " FROM pg_publication "
> + " WHERE pubname
> in ( %s )) "
> + " AND (SELECT count(1)=0 "
> + " FROM
> pg_publication_namespace pn, pg_class c "
> + " WHERE c.oid =
> %u AND c.relnamespace = pn.pnnspid)",
> + lrel->remoteid,
> + pub_names.data,
> + pub_names.data,
> + lrel->remoteid);
>
Fixed in v52-0001 [1]
> 5) Should we mention user should take care of deletion of row filter
> records after table sync is done.
> + ALL TABLES</literal> or <literal>FOR ALL TABLES IN
> SCHEMA</literal> publication,
> + then all other <literal>WHERE</literal> clauses (for the same
> publish operation)
> + become redundant.
> + If the subscriber is a <productname>PostgreSQL</productname>
> version before 15
> + then any row filtering is ignored during the initial data
> synchronization phase.
>
Fixed in v52-0001 [1]
> 6) Should this be an Assert, since this will be taken care earlier by
> GetTransformedWhereClause->transformWhereClause->coerce_to_boolean:
> + expr = (Expr *) coerce_to_target_type(NULL, rfnode, exprtype,
> BOOLOID, -1, COERCION_ASSIGNMENT, COERCE_IMPLICIT_CAST, -1);
> +
> + if (expr == NULL)
> + ereport(ERROR,
> + (errcode(ERRCODE_CANNOT_COERCE),
> + errmsg("row filter returns type %s
> that cannot be cast to the expected type %s",
> + format_type_be(exprtype),
> + format_type_be(BOOLOID)),
> + errhint("You will need to rewrite the
> row filter.")));
>
Not yet fixed in v52-0001 [1], but I did add another regression test for this.
> 7) This code is present in 3 places, can we make it a function or
> macro and use it:
> + if (pub->pubactions.pubinsert)
> + no_filter[idx_ins] = true;
> + if (pub->pubactions.pubupdate)
> + no_filter[idx_upd] = true;
> + if (pub->pubactions.pubdelete)
> + no_filter[idx_del] = true;
> +
> + /* Quick exit loop if all pubactions
> have no row-filter. */
> + if (no_filter[idx_ins] &&
> no_filter[idx_upd] && no_filter[idx_del])
> + break;
> +
> + continue;
>
Fixed in v52-0001 [1]. Added a macro as suggested.
> 8) Can the below transformation be done just before the
> values[Anum_pg_publication_rel_prqual - 1] is set to reduce one of the
> checks:
> + if (pri->whereClause != NULL)
> + {
> + /* Set up a ParseState to parse with */
> + pstate = make_parsestate(NULL);
> +
> + /*
> + * Get the transformed WHERE clause, of boolean type,
> with necessary
> + * collation information.
> + */
> + whereclause = GetTransformedWhereClause(pstate, pri, true);
> + }
>
> /* Form a tuple. */
> memset(values, 0, sizeof(values));
> @@ -328,6 +376,12 @@ publication_add_relation(Oid pubid,
> PublicationRelInfo *targetrel,
> values[Anum_pg_publication_rel_prrelid - 1] =
> ObjectIdGetDatum(relid);
>
> + /* Add qualifications, if available */
> + if (whereclause)
> + values[Anum_pg_publication_rel_prqual - 1] =
> CStringGetTextDatum(nodeToString(whereclause));
> + else
> + nulls[Anum_pg_publication_rel_prqual - 1] = true;
> +
>
> Like:
> /* Add qualifications, if available */
> if (pri->whereClause != NULL)
> {
> /* Set up a ParseState to parse with */
> pstate = make_parsestate(NULL);
>
> /*
> * Get the transformed WHERE clause, of boolean type, with necessary
> * collation information.
> */
> whereclause = GetTransformedWhereClause(pstate, pri, true);
>
> /*
> * Walk the parse-tree of this publication row filter expression and
> * throw an error if anything not permitted or unexpected is
> * encountered.
> */
> rowfilter_walker(whereclause, targetrel);
> values[Anum_pg_publication_rel_prqual - 1] =
> CStringGetTextDatum(nodeToString(whereclause));
> }
> else
> nulls[Anum_pg_publication_rel_prqual - 1] = true;
>
Fixed in v52-0001 [1] as suggested.
------
[1]
https://www.postgresql.org/message-id/CAHut%2BPs3BvAqcNXmMMRBUjOe3GWor0d7r%2BmPxxtzMhYEf59t_Q%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
