[PATCH] Allow CustomScan nodes to signal projection support
Hi,
The attached patch allows CustomScan nodes to signal whether they
support projection.
Currently all CustomScan nodes are treated as supporting projection.
But it would be nice
for custom nodes to opt out of this to prevent postgres from modifying
the targetlist of
the custom node.
For features similar to set-returning functions the function call
needs to be a top level expression in a custom node that implements it
but any targetlist adjustments
might be modified by postgres because it is not aware of the special
meaning of those function calls and it might push down a different
targetlist in the node cause it assumes
the node can project.
I named the flag CUSTOMPATH_SUPPORT_PROJECTION similar to the other
custom node flags, but this would revert the current logic and nodes
would have to opt into
projection. I thought about naming it CUSTOMPATH_CANNOT_PROJECT to keep
the current default and make it an opt out. But that would make the
flag name notably different from the other flag names. Any opinions on
the flag name and whether it should be opt in or opt out?
--
Regards, Sven Klemm
Attachments:
v1-0001-costumscan_projection_flag.patchapplication/octet-stream; name=v1-0001-costumscan_projection_flag.patchDownload
From 1758ca4bffdc9db6cd5ffc114ad668c15028a454 Mon Sep 17 00:00:00 2001
From: Sven Klemm <sven@timescale.com>
Date: Fri, 26 Mar 2021 19:33:50 +0100
Subject: [PATCH v1] Allow CustomScan nodes to signal projection support
Add a new flag CUSTOMPATH_SUPPORT_PROJECTION to allow CustomScan
nodes to control whether they support projection.
---
src/backend/optimizer/plan/createplan.c | 4 ++++
src/include/nodes/extensible.h | 1 +
2 files changed, 5 insertions(+)
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 906cab7053..32199bd6a3 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -7055,6 +7055,8 @@ is_projection_capable_path(Path *path)
* get relaxed later.
*/
return false;
+ case T_CustomScan:
+ return castNode(CustomPath, path)->flags & CUSTOMPATH_SUPPORT_PROJECTION;
default:
break;
}
@@ -7092,6 +7094,8 @@ is_projection_capable_plan(Plan *plan)
* get relaxed later.
*/
return false;
+ case T_CustomScan:
+ return castNode(CustomScan, plan)->flags & CUSTOMPATH_SUPPORT_PROJECTION;
default:
break;
}
diff --git a/src/include/nodes/extensible.h b/src/include/nodes/extensible.h
index 9e425e5651..7d3dc7e828 100644
--- a/src/include/nodes/extensible.h
+++ b/src/include/nodes/extensible.h
@@ -80,6 +80,7 @@ extern const ExtensibleNodeMethods *GetExtensibleNodeMethods(const char *name,
*/
#define CUSTOMPATH_SUPPORT_BACKWARD_SCAN 0x0001
#define CUSTOMPATH_SUPPORT_MARK_RESTORE 0x0002
+#define CUSTOMPATH_SUPPORT_PROJECTION 0x0004
/*
* Custom path methods. Mostly, we just need to know how to convert a
--
2.30.0
Hi Sven,
The attached patch allows CustomScan nodes to signal whether they
support projection.
I noticed that you didn't change custom-scan.sgml accordingly. The
updated patch is attached. Otherwise, it seems to be fine in terms of
compiling, passing tests etc.
I named the flag CUSTOMPATH_SUPPORT_PROJECTION similar to the other
custom node flags, but this would revert the current logic
This seems to be a typical Kobayashi Maru situation, i.e any choice is
a bad one. I suggest keeping the patch as is and hoping that the
developers of existing extensions read the release notes.
--
Best regards,
Aleksander Alekseev
Attachments:
v2-0001-costumscan_projection_flag.patchapplication/octet-stream; name=v2-0001-costumscan_projection_flag.patchDownload
diff --git a/doc/src/sgml/custom-scan.sgml b/doc/src/sgml/custom-scan.sgml
index 239ba29de7..0fd8494e45 100644
--- a/doc/src/sgml/custom-scan.sgml
+++ b/doc/src/sgml/custom-scan.sgml
@@ -73,8 +73,9 @@ typedef struct CustomPath
the row-count estimate, start and total cost, and sort ordering provided
by this path. <structfield>flags</structfield> is a bit mask, which should include
<literal>CUSTOMPATH_SUPPORT_BACKWARD_SCAN</literal> if the custom path can support
- a backward scan and <literal>CUSTOMPATH_SUPPORT_MARK_RESTORE</literal> if it
- can support mark and restore. Both capabilities are optional.
+ a backward scan, <literal>CUSTOMPATH_SUPPORT_MARK_RESTORE</literal> if it
+ can support mark and restore, and <literal>CUSTOMPATH_SUPPORT_PROJECTION</literal>
+ if it can support projections. All capabilities are optional.
An optional <structfield>custom_paths</structfield> is a list of <structname>Path</structname>
nodes used by this custom-path node; these will be transformed into
<structname>Plan</structname> nodes by planner.
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index a9aff24831..c9c8b140e5 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -7051,6 +7051,8 @@ is_projection_capable_path(Path *path)
* get relaxed later.
*/
return false;
+ case T_CustomScan:
+ return castNode(CustomPath, path)->flags & CUSTOMPATH_SUPPORT_PROJECTION;
default:
break;
}
@@ -7089,6 +7091,8 @@ is_projection_capable_plan(Plan *plan)
* get relaxed later.
*/
return false;
+ case T_CustomScan:
+ return castNode(CustomScan, plan)->flags & CUSTOMPATH_SUPPORT_PROJECTION;
default:
break;
}
diff --git a/src/include/nodes/extensible.h b/src/include/nodes/extensible.h
index 9e425e5651..7d3dc7e828 100644
--- a/src/include/nodes/extensible.h
+++ b/src/include/nodes/extensible.h
@@ -80,6 +80,7 @@ extern const ExtensibleNodeMethods *GetExtensibleNodeMethods(const char *name,
*/
#define CUSTOMPATH_SUPPORT_BACKWARD_SCAN 0x0001
#define CUSTOMPATH_SUPPORT_MARK_RESTORE 0x0002
+#define CUSTOMPATH_SUPPORT_PROJECTION 0x0004
/*
* Custom path methods. Mostly, we just need to know how to convert a
Aleksander Alekseev <aleksander@timescale.com> writes:
I named the flag CUSTOMPATH_SUPPORT_PROJECTION similar to the other
custom node flags, but this would revert the current logic
This seems to be a typical Kobayashi Maru situation, i.e any choice is
a bad one. I suggest keeping the patch as is and hoping that the
developers of existing extensions read the release notes.
Yeah, I concur that's the least bad choice.
I got annoyed by the fact that the existing checks of CustomPath flags
had several randomly different styles for basically identical tests,
and this patch wanted to introduce yet another. I cleaned that up and
pushed this.
regards, tom lane