On 2020-Sep-24, Amit Langote wrote:
Hello Amit,
> Sorry I totally failed to see the v2 you had posted and a couple of
> other emails where you mentioned the issues I brought up.
No worries, I appreciate you reviewing this.
> However, I am a bit curious about including detached partitions in
> some cases while not in other, which can result in a (to me)
> surprising behavior as follows:
[ snip ]
> begin;
> insert into foo values (2); -- ok
> select * from foo;
> select * from foo; -- ?!
> a | b
> ---+---
> (0 rows)
>
> Maybe, it's fine to just always exclude detached partitions, although
> perhaps I am missing some corner cases that you have thought of?
Well, this particular case can be fixed by changing
ExecInitPartitionDispatchInfo specifically, from including detached
partitions to excluding them, as in the attached version. Given your
example I think the case is fairly good that they should be excluded
there. I can't think of a case that this change break.
However I'm not sure that excluding them everywhere is sensible. There
are currently two cases where they are included (search for calls to
CreatePartitionDirectory if you're curious). One is snapshot-isolation
transactions (repeatable read and serializable) in
set_relation_partition_info, per the example from Hao Wu. If we simply
exclude detached transaction there, repeatable read no longer works
properly; rows will just go missing for no apparent reason. I don't
think this is acceptable.
The other case is ExecCreatePartitionPruneState(). The whole point of
including detached partitions here is to make them available for queries
that were planned before the detach and executed after the detach. My
fear is that the pruning plan will contain references (from planner) to
partitions that the executor doesn't know about. If there are extra
partitions at the executor side, it shouldn't harm anything (and it
shouldn't change query results); but I'm not sure that things will work
OK if partitions seen by the planner disappear from under the executor.
I'm posting this version just as a fresh rebase -- it is not yet
intended for commit. I haven't touched the constraint stuff. I still
think that that can be removed before commit, which is a simple change;
but even if not, the problem with the duplicated constraints should be
easy to fix.
Again, my thanks for reviewing.