Julien Rouhaud <rjuju123@gmail.com> writes:
> I agree that the RelOptInfo new flag / IS_DUMMY_REL change solution is
> the best solution. Let's hope there won't be too many extensions
> relying on the old IS_DUMMY_REL() macro.
I concluded that that actually doesn't work very well. The reason that
things are set up the way they are, I now remember, is that if we exclude
all children of an appendrel then what we get is a childless Append path.
With the current data structure, that means the appendrel is automatically
recognized as dummy. If we have a separate flag then we'd have to fix a
fair number of places to also set the flag. I'm afraid we'd miss some,
or even more likely that there'd be extensions that would be silently
broken because that doesn't work the same anymore.
So it's now looking like the right solution is to fix IS_DUMMY_REL to
be able to drill down into the path to see if it's a dummy append with
projection(s) on top. This is also better for back-patching, assuming
that we need to worry about extensions using IS_DUMMY_REL:
(1) if an old extension is loaded into a server with the fix, it won't
recognize these corner cases as dummy, but that's no worse than before.
(2) if a recompiled extension is loaded into a server predating the fix,
it will get a link failure due to is_dummy_rel() not being exported.
That's not ideal but at least it's a pretty obvious failure mode.
With the other approach, this would lead to never recognizing dummy rels,
even in cases that worked before, since the extension would be looking at
probably-zero pad space instead of a correctly populated field. Silent
regressions are not nice.
> I'll work on a new version
> of the patch to implement it. In the meantime I'll add an entry for
> this bug in the next commitfest to make sure we don't miss it.
I've already got a mostly-working patch. It's causing one plan change
in select_parallel that I've not quite figured out the reason for, or
I should say that it's not obvious why the existing code appears to
work...
regards, tom lane