On Wed, 12 Jul 2023 at 14:50, David Rowley <dgrowle...@gmail.com> wrote:
>
> On Wed, 12 Jul 2023 at 14:23, Tom Lane <t...@sss.pgh.pa.us> wrote:
> > I did think about that, but "shallow copy a Path" seems nontrivial
> > because the Path structs are all different sizes. Maybe it is worth
> > building some infrastructure to support that?
>
> It seems a reasonable thing to have to do. It seems the minimum thing
> we could do to ensure each Path is only mentioned in at most 1
> RelOptInfo.
I've attached a draft patch which adds copyObjectFlat() and supports
all Node types asides from the ones mentioned in @extra_tags in
gen_node_support.pl. This did require including all the node header
files in copyfuncs.c, which that file seems to have avoided until now.
I also didn't do anything about ExtensibleNode types. I assume just
copying the ExtensibleNode isn't good enough. To flat copy the actual
node I think would require adding a new function to
ExtensibleNodeMethods.
I was also unsure what we should do when shallow copying a List. The
problem there is if we just do a shallow copy, a repalloc() on the
elements array would end up pfreeing memory that might be used by a
shallow copied clone. Perhaps List is not unique in that regard?
Maybe the solution there is to add a special case and list_copy()
Lists like what is done in copyObjectImpl().
I'm hoping the attached patch will at least assist in moving the
discussion along.
David
diff --git a/src/backend/nodes/.gitignore b/src/backend/nodes/.gitignore
index 0c14b5697b..91cbd2cf24 100644
--- a/src/backend/nodes/.gitignore
+++ b/src/backend/nodes/.gitignore
@@ -1,4 +1,5 @@
/node-support-stamp
+/nodesizes.h
/nodetags.h
/*funcs.funcs.c
/*funcs.switch.c
diff --git a/src/backend/nodes/Makefile b/src/backend/nodes/Makefile
index 0a95e683d0..46ce62f828 100644
--- a/src/backend/nodes/Makefile
+++ b/src/backend/nodes/Makefile
@@ -99,4 +99,4 @@ queryjumblefuncs.o: queryjumblefuncs.c
queryjumblefuncs.funcs.c queryjumblefuncs
readfuncs.o: readfuncs.c readfuncs.funcs.c readfuncs.switch.c |
node-support-stamp
maintainer-clean: clean
- rm -f node-support-stamp $(addsuffix funcs.funcs.c,copy equal out
queryjumble read) $(addsuffix funcs.switch.c,copy equal out queryjumble read)
nodetags.h
+ rm -f node-support-stamp $(addsuffix funcs.funcs.c,copy equal out
queryjumble read) $(addsuffix funcs.switch.c,copy equal out queryjumble read)
nodesizes.h nodetags.h
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index f2568ff5e6..ccd4cbfa01 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -15,9 +15,54 @@
#include "postgres.h"
+#include "access/amapi.h"
+#include "access/tableam.h"
+#include "access/tsmapi.h"
+#include "commands/event_trigger.h"
+#include "commands/trigger.h"
+#include "foreign/fdwapi.h"
#include "miscadmin.h"
+#include "nodes/execnodes.h"
+#include "nodes/extensible.h"
+#include "nodes/miscnodes.h"
+#include "nodes/parsenodes.h"
+#include "nodes/pathnodes.h"
+#include "nodes/plannodes.h"
+#include "nodes/replnodes.h"
+#include "nodes/supportnodes.h"
+#include "nodes/tidbitmap.h"
#include "utils/datum.h"
+static const Size flat_node_sizes[] = {
+ 0, /* T_Invalid */
+#include "nodes/nodesizes.h"
+};
+
+/*
+ * copyObjectFlat
+ * Allocate a new copy of the Node type denoted by 'from' and flat
copy the
+ * contents of it into the newly allocated node and return it.
+ */
+void *
+copyObjectFlat(const void *from)
+{
+ Size size;
+ void *retval;
+ NodeTag tag = nodeTag(from);
+
+ if ((unsigned int) tag >= lengthof(flat_node_sizes))
+ {
+ elog(ERROR, "unrecognized node type: %d", (int) tag);
+ return NULL;
+ }
+
+ /* XXX how to handle ExtensibleNodes? Can we just deep copy? */
+ size = flat_node_sizes[tag];
+ retval = palloc(size);
+ memcpy(retval, from, size);
+
+ return retval;
+}
/*
* Macros to simplify copying of different kinds of fields. Use these
diff --git a/src/backend/nodes/gen_node_support.pl
b/src/backend/nodes/gen_node_support.pl
index 72c7963578..e9a759c5a0 100644
--- a/src/backend/nodes/gen_node_support.pl
+++ b/src/backend/nodes/gen_node_support.pl
@@ -2,6 +2,7 @@
#----------------------------------------------------------------------
#
# Generate node support files:
+# - nodesizes.h
# - nodetags.h
# - copyfuncs
# - equalfuncs
@@ -599,6 +600,28 @@ my $header_comment =
*/
';
+# nodesizes.h
+
+push @output_files, 'nodesizes.h';
+open my $ns, '>', "$output_path/nodesizes.h$tmpext"
+ or die "$output_path/nodesizes.h$tmpext: $!";
+
+printf $ns $header_comment, 'nodesizes.h';
+
+foreach my $n (@node_types)
+{
+ next if elem $n, @abstract_types;
+ if (defined $manual_nodetag_number{$n})
+ {
+ print $ns "\tsizeof(T_${n}),\n";
+ }
+ else
+ {
+ print $ns "\tsizeof(${n}),\n";
+ }
+}
+
+close $ns;
# nodetags.h
diff --git a/src/backend/optimizer/plan/planner.c
b/src/backend/optimizer/plan/planner.c
index 44efb1f4eb..2bfcddbce2 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -5143,7 +5143,15 @@ create_ordered_paths(PlannerInfo *root,
input_path->pathkeys, &presorted_keys);
if (is_sorted)
- sorted_path = input_path;
+ {
+ /*
+ * Perform a flat copy of the already-sorted node so as
not to reference an
+ * existing Path from another RelOptInfo. The
add_path() call below may
+ * pfree this path, which would be problematic when
it's still referenced
+ * by input_rel.
+ */
+ sorted_path = copyObjectFlat(input_path);
+ }
else
{
/*
diff --git a/src/include/Makefile b/src/include/Makefile
index 5d213187e2..283ae311b3 100644
--- a/src/include/Makefile
+++ b/src/include/Makefile
@@ -44,6 +44,7 @@ install: all installdirs
$(INSTALL_DATA) pg_config.h '$(DESTDIR)$(includedir_server)'
$(INSTALL_DATA) pg_config_ext.h '$(DESTDIR)$(includedir_server)'
$(INSTALL_DATA) pg_config_os.h '$(DESTDIR)$(includedir_server)'
+ $(INSTALL_DATA) nodes/nodesizes.h '$(DESTDIR)$(includedir_server)/nodes'
$(INSTALL_DATA) nodes/nodetags.h '$(DESTDIR)$(includedir_server)/nodes'
$(INSTALL_DATA) utils/errcodes.h '$(DESTDIR)$(includedir_server)/utils'
$(INSTALL_DATA) utils/fmgroids.h '$(DESTDIR)$(includedir_server)/utils'
@@ -75,7 +76,7 @@ clean:
rm -f storage/lwlocknames.h utils/probes.h utils/wait_event_types.h
rm -f catalog/schemapg.h catalog/system_fk_info.h
rm -f catalog/pg_*_d.h catalog/header-stamp
- rm -f nodes/nodetags.h nodes/header-stamp
+ rm -f nodes/nodesizes.h nodes/nodetags.h nodes/header-stamp
distclean maintainer-clean: clean
rm -f pg_config.h pg_config_ext.h pg_config_os.h stamp-h stamp-ext-h
diff --git a/src/include/nodes/meson.build b/src/include/nodes/meson.build
index 626dc696d5..cc25d99701 100644
--- a/src/include/nodes/meson.build
+++ b/src/include/nodes/meson.build
@@ -31,7 +31,7 @@ foreach i : node_support_input_i
endforeach
node_support_output = [
- 'nodetags.h',
+ 'nodesizes.h', 'nodetags.h',
'outfuncs.funcs.c', 'outfuncs.switch.c',
'readfuncs.funcs.c', 'readfuncs.switch.c',
'copyfuncs.funcs.c', 'copyfuncs.switch.c',
@@ -39,6 +39,7 @@ node_support_output = [
'queryjumblefuncs.funcs.c', 'queryjumblefuncs.switch.c',
]
node_support_install = [
+ dir_include_server / 'nodes',
dir_include_server / 'nodes',
false, false,
false, false,
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index f8e8fe699a..a325e15dcd 100644
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -235,6 +235,7 @@ extern int16 *readAttrNumberCols(int numCols);
/*
* nodes/copyfuncs.c
*/
+extern void *copyObjectFlat(const void *from);
extern void *copyObjectImpl(const void *from);
/* cast result back to argument type, if supported by compiler */
diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index 1cbc857e35..52b71e5f84 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -847,6 +847,14 @@ EOF
close($f);
}
+ if (IsNewer(
+ 'src/include/nodes/nodesizes.h',
+ 'src/backend/nodes/nodesizes.h'))
+ {
+ copyFile('src/backend/nodes/nodesizes.h',
+ 'src/include/nodes/nodesizes.h');
+ }
+
if (IsNewer(
'src/include/nodes/nodetags.h',
'src/backend/nodes/nodetags.h'))
diff --git a/src/tools/msvc/clean.bat b/src/tools/msvc/clean.bat
index 7cb23ea894..f38e0f8dd2 100755
--- a/src/tools/msvc/clean.bat
+++ b/src/tools/msvc/clean.bat
@@ -41,6 +41,7 @@ REM Delete files created with GenerateFiles() in Solution.pm
if exist src\include\pg_config.h del /q src\include\pg_config.h
if exist src\include\pg_config_ext.h del /q src\include\pg_config_ext.h
if exist src\include\pg_config_os.h del /q src\include\pg_config_os.h
+if exist src\include\nodes\nodesizes.h del /q src\include\nodes\nodesizes.h
if exist src\include\nodes\nodetags.h del /q src\include\nodes\nodetags.h
if exist src\include\utils\errcodes.h del /q src\include\utils\errcodes.h
if exist src\include\utils\fmgroids.h del /q src\include\utils\fmgroids.h
diff --git a/src/tools/pginclude/cpluspluscheck
b/src/tools/pginclude/cpluspluscheck
index 4e09c4686b..a5f999c5da 100755
--- a/src/tools/pginclude/cpluspluscheck
+++ b/src/tools/pginclude/cpluspluscheck
@@ -97,6 +97,10 @@ do
# sepgsql.h depends on headers that aren't there on most platforms.
test "$f" = contrib/sepgsql/sepgsql.h && continue
+ # nodesizes.h cannot be included standalone: it's just a code fragment.
+ test "$f" = src/include/nodes/nodesizes.h && continue
+ test "$f" = src/backend/nodes/nodesizes.h && continue
+
# nodetags.h cannot be included standalone: it's just a code fragment.
test "$f" = src/include/nodes/nodetags.h && continue
test "$f" = src/backend/nodes/nodetags.h && continue
diff --git a/src/tools/pginclude/headerscheck b/src/tools/pginclude/headerscheck
index 8dee1b5670..18cb54dbb1 100755
--- a/src/tools/pginclude/headerscheck
+++ b/src/tools/pginclude/headerscheck
@@ -92,6 +92,10 @@ do
# sepgsql.h depends on headers that aren't there on most platforms.
test "$f" = contrib/sepgsql/sepgsql.h && continue
+ # nodesizes.h cannot be included standalone: it's just a code fragment.
+ test "$f" = src/include/nodes/nodesizes.h && continue
+ test "$f" = src/backend/nodes/nodesizes.h && continue
+
# nodetags.h cannot be included standalone: it's just a code fragment.
test "$f" = src/include/nodes/nodetags.h && continue
test "$f" = src/backend/nodes/nodetags.h && continue
