On Wed, Jul 14, 2021 at 6:38 AM Euler Taveira <euler@eulerto.com> wrote:
>
> Peter, thanks for quickly check the new patch. I'm attaching a new patch (v19)
> that addresses (a) this new review, (b) Tomas' review and (c) Greg's review. I
> also included the copy/equal node support for the new node (PublicationTable)
> mentioned by Tomas in another email.
>
Some minor v19 patch review points you might consider for your next
patch version:
(I'm still considering the other issues raised about WHERE clauses and
filtering)
(1) src/backend/commands/publicationcmds.c
OpenTableList
Some suggested abbreviations:
BEFORE:
if (IsA(lfirst(lc), PublicationTable))
whereclause = true;
else
whereclause = false;
AFTER:
whereclause = IsA(lfirst(lc), PublicationTable);
BEFORE:
if (whereclause)
pri->whereClause = t->whereClause;
else
pri->whereClause = NULL;
AFTER:
pri->whereClause = whereclause? t->whereClause : NULL;
(2) src/backend/parser/parse_expr.c
I think that the check below:
/* Functions are not allowed in publication WHERE clauses */
if (pstate->p_expr_kind == EXPR_KIND_PUBLICATION_WHERE &&
nodeTag(expr) == T_FuncCall)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("functions are not allowed in publication WHERE expressions"),
parser_errposition(pstate, exprLocation(expr))));
should be moved down into the "T_FuncCall" case of the switch
statement below it, so that "if (pstate->p_expr_kind ==
EXPR_KIND_PUBLICATION_WHERE" doesn't get checked every call to
transformExprRecurse() regardless of the expression Node type.
(3) Save a nanosecond when entry->exprstate is already NIL:
BEFORE:
if (entry->exprstate != NIL)
list_free_deep(entry->exprstate);
entry->exprstate = NIL;
AFTER:
if (entry->exprstate != NIL)
{
list_free_deep(entry->exprstate);
entry->exprstate = NIL;
}
Regards,
Greg Nancarrow
Fujitsu Australia
