Hi!
> When reviewing the code in logical/worker.c, I noticed that when applying a
> cross-partition update action, it scans the old partition twice.
Nice catch!
> -/*
> - * Workhorse for apply_handle_update()
> - * relinfo is for the relation we're actually updating in
> - * (could be a child partition of edata->targetRelInfo)
> - */
> -static void
> -apply_handle_update_internal(ApplyExecutionData *edata,
> - ResultRelInfo *relinfo,
> - TupleTableSlot *remoteslot,
> - LogicalRepTupleData *newtup,
> - Oid localindexoid)
> -{
What's the necessity of this change? Can we modify a function in-place
instead of removing and rewriting it in the same file?
This will reduce diff, making patch a bit more clear.
> +/*
> + * If the tuple to be modified could not be found, a log message is emitted.
> + */
> +static void
> +report_tuple_not_found(CmdType cmd, Relation targetrel, bool is_partition)
> +{
> + Assert(cmd == CMD_UPDATE || cmd == CMD_DELETE);
> +
> + /* XXX should this be promoted to ereport(LOG) perhaps? */
> + elog(DEBUG1,
> + "logical replication did not find row to be %s in replication target relation%s \"%s\"",
> + cmd == CMD_UPDATE ? "updated" : "deleted",
> + is_partition ? "'s partition" : "",
> + RelationGetRelationName(targetrel));
> +}
Encapsulating report logic inside function is ok, but double ternary
expression is a bit out of line. I do not see similar places within
PostgreSQL,
so it probably violates code style.