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
