On Thu, Sep 29, 2022 at 12:47 AM Amit Langote <amitlangot...@gmail.com> wrote:
> [ patches ]
While looking over this thread I came across this code:
/* For data reading, executor always omits detached partitions */
if (estate->es_partition_directory == NULL)
estate->es_partition_directory =
CreatePartitionDirectory(estate->es_query_cxt, false);
But CreatePartitionDirectory is declared like this:
extern PartitionDirectory CreatePartitionDirectory(MemoryContext mcxt,
bool omit_detached);
So the comment seems to say the opposite of what the code does. The
code seems to match the explanation in the commit message for
71f4c8c6f74ba021e55d35b1128d22fb8c6e1629, so I am guessing that
perhaps s/always/never/ is needed here.
I also noticed that ExecCreatePartitionPruneState no longer exists in
the code but is still referenced in
src/test/modules/delay_execution/specs/partition-addition.spec
Regarding 0003, it seems unfortunate that
find_inheritance_children_extended() will now have 6 arguments 4 of
which have to do with detached partition handling. That is a lot of
detached partition handling, and it's hard to reason about. I don't
see an obvious way of simplifying things very much, but I wonder if we
could at least have the new omit_detached_snapshot snapshot replace
the existing bool omit_detached flag. Something like the attached
incremental patch.
Probably we need to go further than the attached, though. I don't
think that PartitionDirectoryLookup() should be getting any new
arguments. The whole point of that function is that it's supposed to
ensure that the returned value is stable, and the comments say so. But
with these changes it isn't any more, because it depends on the
snapshot you pass. It seems fine to specify when you create the
partition directory that you want it to show a different, still-stable
view of the world, but as written, it seems to me to undermine the
idea that the return value is expected to be stable at all. Is there a
way we can avoid that?
--
Robert Haas
EDB: http://www.enterprisedb.com
diff --git a/src/backend/catalog/pg_inherits.c
b/src/backend/catalog/pg_inherits.c
index f810e5de0d..eb5377e7c0 100644
--- a/src/backend/catalog/pg_inherits.c
+++ b/src/backend/catalog/pg_inherits.c
@@ -60,7 +60,7 @@ typedef struct SeenRelsEntry
List *
find_inheritance_children(Oid parentrelId, LOCKMODE lockmode)
{
- return find_inheritance_children_extended(parentrelId, true,
+ return find_inheritance_children_extended(parentrelId,
ActiveSnapshotSet() ?
GetActiveSnapshot() : NULL,
lockmode, NULL, NULL);
@@ -75,7 +75,7 @@ find_inheritance_children(Oid parentrelId, LOCKMODE lockmode)
* If a partition's pg_inherits row is marked "detach pending",
* *detached_exist (if not null) is set true.
*
- * If omit_detached is true and the caller passed 'omit_detached_snapshot',
+ * If the caller passed 'omit_detached_snapshot',
* the partition whose pg_inherits tuple marks it as "detach pending" is
* omitted from the output list if the tuple is visible to that snapshot.
* That is, such a partition is omitted from the output list depending on
@@ -84,7 +84,7 @@ find_inheritance_children(Oid parentrelId, LOCKMODE lockmode)
* NULL) is set to the xmin of that pg_inherits tuple.
*/
List *
-find_inheritance_children_extended(Oid parentrelId, bool omit_detached,
+find_inheritance_children_extended(Oid parentrelId,
Snapshot
omit_detached_snapshot,
LOCKMODE
lockmode, bool *detached_exist,
TransactionId *detached_xmin)
@@ -146,7 +146,7 @@ find_inheritance_children_extended(Oid parentrelId, bool
omit_detached,
if (detached_exist)
*detached_exist = true;
- if (omit_detached && omit_detached_snapshot)
+ if (omit_detached_snapshot)
{
TransactionId xmin;
diff --git a/src/backend/partitioning/partdesc.c
b/src/backend/partitioning/partdesc.c
index 863b04c17d..23f1334dbc 100644
--- a/src/backend/partitioning/partdesc.c
+++ b/src/backend/partitioning/partdesc.c
@@ -48,7 +48,6 @@ typedef struct PartitionDirectoryEntry
} PartitionDirectoryEntry;
static PartitionDesc RelationBuildPartitionDesc(Relation rel,
-
bool omit_detached,
Snapshot omit_detached_snapshot);
@@ -76,8 +75,7 @@ static PartitionDesc RelationBuildPartitionDesc(Relation rel,
* that the data doesn't become stale.
*/
PartitionDesc
-RelationGetPartitionDescExt(Relation rel, bool omit_detached,
- Snapshot
omit_detached_snapshot)
+RelationGetPartitionDescExt(Relation rel, Snapshot omit_detached_snapshot)
{
Assert(rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE);
@@ -91,7 +89,7 @@ RelationGetPartitionDescExt(Relation rel, bool omit_detached,
* so we we can used the cached descriptor in that case too.
*/
if (likely(rel->rd_partdesc &&
- (!rel->rd_partdesc->detached_exist || !omit_detached
||
+ (!rel->rd_partdesc->detached_exist ||
omit_detached_snapshot == NULL)))
return rel->rd_partdesc;
@@ -106,9 +104,7 @@ RelationGetPartitionDescExt(Relation rel, bool
omit_detached,
* have been passed when rd_partdesc_nodetached was built, then we can
* reuse it. Otherwise we must build one from scratch.
*/
- if (omit_detached &&
- rel->rd_partdesc_nodetached &&
- omit_detached_snapshot)
+ if (rel->rd_partdesc_nodetached && omit_detached_snapshot)
{
Assert(TransactionIdIsValid(rel->rd_partdesc_nodetached_xmin));
@@ -117,8 +113,7 @@ RelationGetPartitionDescExt(Relation rel, bool
omit_detached,
return rel->rd_partdesc_nodetached;
}
- return RelationBuildPartitionDesc(rel, omit_detached,
-
omit_detached_snapshot);
+ return RelationBuildPartitionDesc(rel, omit_detached_snapshot);
}
/*
@@ -129,9 +124,11 @@ RelationGetPartitionDescExt(Relation rel, bool
omit_detached,
PartitionDesc
RelationGetPartitionDesc(Relation rel, bool omit_detached)
{
- return RelationGetPartitionDescExt(rel, omit_detached,
-
ActiveSnapshotSet() ?
-
GetActiveSnapshot() : NULL);
+ Snapshot snapshot = NULL;
+
+ if (omit_detached && ActiveSnapshotSet())
+ snapshot = GetActiveSnapshot();
+ return RelationGetPartitionDescExt(rel, snapshot);
}
/*
@@ -156,7 +153,7 @@ RelationGetPartitionDesc(Relation rel, bool omit_detached)
* for them.
*/
static PartitionDesc
-RelationBuildPartitionDesc(Relation rel, bool omit_detached,
+RelationBuildPartitionDesc(Relation rel,
Snapshot
omit_detached_snapshot)
{
PartitionDesc partdesc;
@@ -185,7 +182,6 @@ RelationBuildPartitionDesc(Relation rel, bool omit_detached,
detached_exist = false;
detached_xmin = InvalidTransactionId;
inhoids = find_inheritance_children_extended(RelationGetRelid(rel),
-
omit_detached,
omit_detached_snapshot,
NoLock,
&detached_exist,
@@ -353,7 +349,7 @@ RelationBuildPartitionDesc(Relation rel, bool omit_detached,
* have set detached_xmin in that case), we consider there to be no
* "omittable" detached partitions.
*/
- is_omit = omit_detached && detached_exist && omit_detached_snapshot &&
+ is_omit = detached_exist && omit_detached_snapshot &&
TransactionIdIsValid(detached_xmin);
/*
@@ -467,8 +463,7 @@ PartitionDirectoryLookup(PartitionDirectory pdir, Relation
rel,
Assert(omit_detached_snapshot == NULL);
if (pdir->omit_detached && ActiveSnapshotSet())
omit_detached_snapshot = GetActiveSnapshot();
- pde->pd = RelationGetPartitionDescExt(rel, pdir->omit_detached,
-
omit_detached_snapshot);
+ pde->pd = RelationGetPartitionDescExt(rel,
omit_detached_snapshot);
Assert(pde->pd != NULL);
}
return pde->pd;
diff --git a/src/include/catalog/pg_inherits.h
b/src/include/catalog/pg_inherits.h
index 67f148f2bf..14515d74d1 100644
--- a/src/include/catalog/pg_inherits.h
+++ b/src/include/catalog/pg_inherits.h
@@ -50,7 +50,7 @@ DECLARE_INDEX(pg_inherits_parent_index, 2187,
InheritsParentIndexId, on pg_inher
extern List *find_inheritance_children(Oid parentrelId, LOCKMODE lockmode);
-extern List *find_inheritance_children_extended(Oid parentrelId, bool
omit_detached,
+extern List *find_inheritance_children_extended(Oid parentrelId,
Snapshot omit_detached_snapshot,
LOCKMODE lockmode, bool *detached_exist,
TransactionId *detached_xmin);
diff --git a/src/include/partitioning/partdesc.h
b/src/include/partitioning/partdesc.h
index f42d137fc1..f3d701d5b4 100644
--- a/src/include/partitioning/partdesc.h
+++ b/src/include/partitioning/partdesc.h
@@ -65,7 +65,7 @@ typedef struct PartitionDescData
extern PartitionDesc RelationGetPartitionDesc(Relation rel, bool
omit_detached);
-extern PartitionDesc RelationGetPartitionDescExt(Relation rel, bool
omit_detached,
+extern PartitionDesc RelationGetPartitionDescExt(Relation rel,
Snapshot omit_detached_snapshot);
extern PartitionDirectory CreatePartitionDirectory(MemoryContext mcxt, bool
omit_detached);
