Re: partition routing layering in nodeModifyTable.c

Hi,

On 2019-07-19 17:11:10 -0400, Alvaro Herrera wrote:
> On 2019-Jul-19, Andres Freund wrote:
> > > - slot = ExecDelete(node, tupleid, oldtuple, planSlot,
> > > - &node->mt_epqstate, estate,
> > > + slot = ExecDelete(node, resultRelInfo, tupleid, oldtuple,
> > > + planSlot, &node->mt_epqstate, estate,
> > > true, node->canSetTag,
> > > false /* changingPart */ , NULL, NULL);
> > > break;
> >
> > This reminds me of another complaint: ExecDelete and ExecInsert() have
> > gotten more boolean parameters for partition moving, but only one of
> > them is explained with a comment (/* changingPart */) - think we should
> > do that for all.
>
> Maybe change the API to use a flags bitmask?
>
> (IMO the placement of the comment inside the function call, making the
> comma appear preceded with a space, looks ugly. If we want to add
> comments, let's put each param on its own line with the comment beyond
> the comma. That's what we do in other places where this pattern is
> used.)

Well, that's the pre-existing style, so I'd just have gone with
that. I'm not sure I buy there's much point in going for a bitmask, as
this is file-private code, not code where changing the signature
requires modifying multiple files.

> > > /* Initialize the executor state. */
> > > - estate = create_estate_for_relation(rel);
> > > + estate = create_estate_for_relation(rel, &resultRelInfo);
> >
> > Hm. It kinda seems cleaner if we were to instead return the relevant
> > index, rather than the entire ResultRelInfo, as an output from
> > create_estate_for_relation(). Makes it clearer that it's still in the
> > EState.
>
> Yeah.
>
> > Or perhaps we ought to compute it in a separate step? Then that'd be
> > more amenable to support replcating into partition roots.
>
> I'm not quite seeing the shape that you're imagining this would take.
> I vote not to mess with that for this patch; I bet that we'll have to
> change a few other things in this code when we add better support for
> partitioning in logical replication.

Yea, I think it's fine to do that separately. If we wanted to support
replication roots as replication targets, we'd obviously need to do
something pretty similar to what ExecInsert()/ExecUpdate() already
do. And there we can't just reference an index in EState, as partition
children aren't in there.

I kind of was wondering if we were to have a separate function for
getting the ResultRelInfo targeted, we'd be able to just extend that
function to support replication. But now that I think about it a bit
more, that's so much just scratching the surface...

We really ought to have the replication "sink" code share more code with
nodeModifyTable.c.

Greetings,

Andres Freund