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