agora inbox for [email protected]
help / color / mirror / Atom feedThread-unsafe coding in ecpg
47+ messages / 13 participants
[nested] [flat]
* Thread-unsafe coding in ecpg
@ 2019-01-18 03:54 Tom Lane <[email protected]>
0 siblings, 1 reply; 47+ messages in thread
From: Tom Lane @ 2019-01-18 03:54 UTC (permalink / raw)
To: [email protected]; +Cc: Michael Meskes <[email protected]>
I've found that a couple of different OpenBSD 6.4 machines fall over
badly in the ecpg regression tests, with output like
test sql/parser ... ok
test thread/thread ... stdout stderr FAILED (test process was terminated by signal 6: Abort trap)
test thread/thread_implicit ... stdout FAILED (test process was terminated by signal 10: Bus error)
test thread/prep ... ok (test process was terminated by signal 10: Bus error)
test thread/alloc ... stderr FAILED (test process was terminated by signal 6: Abort trap)
test thread/descriptor ... ok
It's somewhat variable as to which tests fail, but it's always thread
tests. Examining the core dumps shows traces like
#0 thrkill () at -:3
#1 0x00000c04f427dd6e in _libc_abort () at /usr/src/lib/libc/stdlib/abort.c:51
#2 0x00000c04f425f7e9 in wrterror (d=Variable "d" is not available.
)
at /usr/src/lib/libc/stdlib/malloc.c:291
#3 0x00000c04f42628fb in find_chunknum (d=Variable "d" is not available.
)
at /usr/src/lib/libc/stdlib/malloc.c:1043
#4 0x00000c04f425fe23 in ofree (argpool=Variable "argpool" is not available.
)
at /usr/src/lib/libc/stdlib/malloc.c:1359
#5 0x00000c04f425f8ec in free (ptr=0xc04df0e26e0)
at /usr/src/lib/libc/stdlib/malloc.c:1419
#6 0x00000c04f427ec83 in freegl (oldgl=0xc05a022d080)
at /usr/src/lib/libc/locale/setlocale.c:32
#7 0x00000c04f427eb49 in _libc_setlocale (category=4,
locname=0xc059b605180 "C") at /usr/src/lib/libc/locale/setlocale.c:177
#8 0x00000c0531a6f955 in ecpg_do_epilogue (stmt=0xc0587bb0c00)
at execute.c:1986
#9 0x00000c0531a6fa65 in ecpg_do (lineno=Variable "lineno" is not available.
) at execute.c:2018
#10 0x00000c0531a6fb31 in ECPGdo (lineno=Variable "lineno" is not available.
) at execute.c:2037
#11 0x00000c02a9f00b19 in test_thread (arg=Variable "arg" is not available.
) at thread.pgc:131
#12 0x00000c04b180b26e in _rthread_start (v=Variable "v" is not available.
)
at /usr/src/lib/librthread/rthread.c:96
#13 0x00000c04f42ba77b in __tfork_thread ()
at /usr/src/lib/libc/arch/amd64/sys/tfork_thread.S:75
#14 0x0000000000000000 in ?? ()
or this one:
#0 _libc_strlcpy (dst=0x2c61e133ee0 "C",
src=0xdfdfdfdfdfdfdfdf <Address 0xdfdfdfdfdfdfdfdf out of bounds>,
dsize=256) at /usr/src/lib/libc/string/strlcpy.c:36
#1 0x000002c61de99a71 in _libc_setlocale (category=4, locname=0x0)
from /usr/lib/libc.so.92.5
#2 0x000002c5ae2693a8 in ecpg_do_prologue (lineno=59, compat=0,
force_indicator=1, connection_name=0x0, questionmarks=false,
statement_type=ECPGst_execute, query=0x2c333701418 "i",
args=0x2c61a96f6d0, stmt_out=0x2c61a96f5e0) at execute.c:1776
#3 0x000002c5ae269a20 in ecpg_do (lineno=Variable "lineno" is not available.
) at execute.c:2001
#4 0x000002c5ae269b31 in ECPGdo (lineno=Variable "lineno" is not available.
) at execute.c:2037
#5 0x000002c333600b47 in fn (arg=Variable "arg" is not available.
) at prep.pgc:59
#6 0x000002c56a00b26e in _rthread_start (v=Variable "v" is not available.
)
at /usr/src/lib/librthread/rthread.c:96
#7 0x000002c61ded577b in __tfork_thread ()
at /usr/src/lib/libc/arch/amd64/sys/tfork_thread.S:75
#8 0x0000000000000000 in ?? ()
The common denominator is always a call to setlocale(), and
that generally is calling malloc or free or some other libc
function that is unhappy. When output appears on stderr,
it's usually free complaining about double-frees or some such.
So my conclusion is that this version of setlocale() has some
thread-safety issues. I was all set to go file a bug report
when I noticed this in the POSIX spec for setlocale:
The setlocale() function need not be thread-safe.
as well as this:
The global locale established using setlocale() shall only be used
in threads for which no current locale has been set using
uselocale() or whose current locale has been set to the global
locale using uselocale(LC_GLOBAL_LOCALE).
IOW, not only is setlocale() not necessarily thread-safe itself,
but using it to change locales in a multithread program is seriously
unsafe because of concurrent effects on other threads.
Therefore, it's plain crazy for ecpg to be calling setlocale() inside
threaded code. It looks to me like what ecpg is doing is trying to defend
itself against non-C LC_NUMERIC settings, which is laudable, but this
implementation of that is totally unsafe.
Don't know what's the best way out of this. The simplest thing would
be to just remove that code and document that you'd better run ecpg
in LC_NUMERIC locale, but it'd be nice if we could do better.
regards, tom lane
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-19 19:15 Michael Meskes <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 2 replies; 47+ messages in thread
From: Michael Meskes @ 2019-01-19 19:15 UTC (permalink / raw)
To: Tom Lane <[email protected]>; [email protected]
> So my conclusion is that this version of setlocale() has some
> thread-safety issues. I was all set to go file a bug report
> when I noticed this in the POSIX spec for setlocale:
>
> The setlocale() function need not be thread-safe.
>
> as well as this:
>
> The global locale established using setlocale() shall only be
> used
> in threads for which no current locale has been set using
> uselocale() or whose current locale has been set to the global
> locale using uselocale(LC_GLOBAL_LOCALE).
This one was new to me.
> IOW, not only is setlocale() not necessarily thread-safe itself,
> but using it to change locales in a multithread program is seriously
> unsafe because of concurrent effects on other threads.
Agreed.
> Therefore, it's plain crazy for ecpg to be calling setlocale() inside
> threaded code. It looks to me like what ecpg is doing is trying to
> defend
> itself against non-C LC_NUMERIC settings, which is laudable, but this
> implementation of that is totally unsafe.
>
> Don't know what's the best way out of this. The simplest thing would
> be to just remove that code and document that you'd better run ecpg
> in LC_NUMERIC locale, but it'd be nice if we could do better.
How about attached patch? According to my manpages it should only
affect the calling threat. I only tested it on my own system so far.
Could you please have a look and/or test on other systems?
Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL
Attachments:
[text/x-patch] ecpg_locale.diff (1.8K, ../../[email protected]/2-ecpg_locale.diff)
download | inline diff:
diff --git a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
index 1c9bce1456..92ff7126d9 100644
--- a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
+++ b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
@@ -61,7 +61,8 @@ struct statement
bool questionmarks;
struct variable *inlist;
struct variable *outlist;
- char *oldlocale;
+ locale_t clocale;
+ locale_t oldlocale;
int nparams;
char **paramvalues;
PGresult *results;
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index 3f5034e792..ba03607d5c 100644
--- a/src/interfaces/ecpg/ecpglib/execute.c
+++ b/src/interfaces/ecpg/ecpglib/execute.c
@@ -102,7 +102,8 @@ free_statement(struct statement *stmt)
free_variable(stmt->outlist);
ecpg_free(stmt->command);
ecpg_free(stmt->name);
- ecpg_free(stmt->oldlocale);
+ if (stmt->clocale)
+ freelocale(stmt->clocale);
ecpg_free(stmt);
}
@@ -1773,13 +1774,18 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
* Make sure we do NOT honor the locale for numeric input/output since the
* database wants the standard decimal point
*/
- stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
- if (stmt->oldlocale == NULL)
+ stmt->clocale = newlocale(LC_NUMERIC_MASK, "C", (locale_t) 0);
+ if (stmt->clocale == (locale_t)0)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
+ stmt->oldlocale = uselocale(stmt->clocale);
+ if (stmt->oldlocale == (locale_t)0)
{
ecpg_do_epilogue(stmt);
return false;
}
- setlocale(LC_NUMERIC, "C");
#ifdef ENABLE_THREAD_SAFETY
ecpg_pthreads_init();
@@ -1983,7 +1989,7 @@ ecpg_do_epilogue(struct statement *stmt)
return;
if (stmt->oldlocale)
- setlocale(LC_NUMERIC, stmt->oldlocale);
+ uselocale(stmt->oldlocale);
free_statement(stmt);
}
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-20 00:10 Tom Lane <[email protected]>
parent: Michael Meskes <[email protected]>
1 sibling, 1 reply; 47+ messages in thread
From: Tom Lane @ 2019-01-20 00:10 UTC (permalink / raw)
To: Michael Meskes <[email protected]>; +Cc: [email protected]
Michael Meskes <[email protected]> writes:
>> IOW, not only is setlocale() not necessarily thread-safe itself,
>> but using it to change locales in a multithread program is seriously
>> unsafe because of concurrent effects on other threads.
> Agreed.
> How about attached patch? According to my manpages it should only
> affect the calling threat. I only tested it on my own system so far.
> Could you please have a look and/or test on other systems?
Yeah, I was wondering about uselocale() myself. We cannot assume it's
available everywhere, but it should fix the problem where available.
On machines that don't have it, we could either
(a) have ecpg do nothing, and just hope you're not using a dangerous
locale; or
(b) consider the platform not thread-safe, forcing people to specify
--disable-thread-safety to build.
While (b) has more theoretical purity, I'm inclined to think it
doesn't really improve anybody's life compared to (a), because
--disable-thread-safety doesn't actually stop anyone from using
libpq or ecpglib in threaded environments. It just makes it
more likely to fail when they do.
The OpenBSD 6.4 platform where I found this problem has uselocale
(but the man page notes they only added it as of 6.2). I can test
out the patch there, but I think the interesting questions are all
about what to do on platforms without the function.
regards, tom lane
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-20 00:23 Michael Meskes <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 47+ messages in thread
From: Michael Meskes @ 2019-01-20 00:23 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: [email protected]
> While (b) has more theoretical purity, I'm inclined to think it
> doesn't really improve anybody's life compared to (a), because
> --disable-thread-safety doesn't actually stop anyone from using
> libpq or ecpglib in threaded environments. It just makes it
> more likely to fail when they do.
The question is, what do we do on those platforms? Use setlocale() or
fallback to (a) and document that ecpg has to run in a C locale?
We could also rewrite the parsing of numbers to not be locale
dependent.
Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-20 00:37 Tom Lane <[email protected]>
parent: Michael Meskes <[email protected]>
0 siblings, 1 reply; 47+ messages in thread
From: Tom Lane @ 2019-01-20 00:37 UTC (permalink / raw)
To: Michael Meskes <[email protected]>; +Cc: [email protected]
Michael Meskes <[email protected]> writes:
>> While (b) has more theoretical purity, I'm inclined to think it
>> doesn't really improve anybody's life compared to (a), because
>> --disable-thread-safety doesn't actually stop anyone from using
>> libpq or ecpglib in threaded environments. It just makes it
>> more likely to fail when they do.
> The question is, what do we do on those platforms? Use setlocale() or
> fallback to (a) and document that ecpg has to run in a C locale?
No, we shouldn't use setlocale(), because it clearly is hazardous
even on platforms where it doesn't fail outright. I don't see
anything so wrong with just documenting the hazard. The situation
isn't noticeably more dangerous than simple use of the C library;
sscanf, strtod, etc are all likely to do surprising things when
LC_NUMERIC isn't C.
> We could also rewrite the parsing of numbers to not be locale
> dependent.
Perhaps, but that seems like a giant undertaking. I'm not excited
about duplicating strtod(), for instance.
regards, tom lane
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-20 01:32 Andrew Gierth <[email protected]>
parent: Michael Meskes <[email protected]>
1 sibling, 1 reply; 47+ messages in thread
From: Andrew Gierth @ 2019-01-20 01:32 UTC (permalink / raw)
To: Michael Meskes <[email protected]>; +Cc: Tom Lane <[email protected]>; [email protected]
>>>>> "Michael" == Michael Meskes <[email protected]> writes:
>> Therefore, it's plain crazy for ecpg to be calling setlocale()
>> inside threaded code. It looks to me like what ecpg is doing is
>> trying to defend itself against non-C LC_NUMERIC settings, which is
>> laudable, but this implementation of that is totally unsafe.
>>
>> Don't know what's the best way out of this. The simplest thing would
>> be to just remove that code and document that you'd better run ecpg
>> in LC_NUMERIC locale, but it'd be nice if we could do better.
Would it help if we had non-locale-aware functions for both
floating-point output _and_ input? i.e. import a known-working strtod()
(allowing us to remove all the hacks that have grown up around it, for
special-case input and wonky error handling) with locale functionality
removed.
--
Andrew (irc:RhodiumToad)
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-20 02:00 Tom Lane <[email protected]>
parent: Andrew Gierth <[email protected]>
0 siblings, 0 replies; 47+ messages in thread
From: Tom Lane @ 2019-01-20 02:00 UTC (permalink / raw)
To: Andrew Gierth <[email protected]>; +Cc: Michael Meskes <[email protected]>; [email protected]
Andrew Gierth <[email protected]> writes:
> Would it help if we had non-locale-aware functions for both
> floating-point output _and_ input? i.e. import a known-working strtod()
> (allowing us to remove all the hacks that have grown up around it, for
> special-case input and wonky error handling) with locale functionality
> removed.
Dunno, is there such a thing as a platform-independent strtod()?
I'd have thought that, for instance, typical implementations would
be pretty much in bed with the details of IEEE float format ---
your example where strtof() is different from (float) strtod()
makes it hard to believe that it can be written without assumptions
about the hardware's float format.
(Note that this concern is independent of whether we adopt the Ryu
code, which IIUC also depends on IEEE floats. Our answer for anyone
wanting to run on non-IEEE hardware can be to #ifdef out Ryu and use
the existing float output code. But doing the equivalent thing on the
input side wouldn't solve ecpg's problem.)
regards, tom lane
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-20 17:08 Michael Meskes <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 47+ messages in thread
From: Michael Meskes @ 2019-01-20 17:08 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: [email protected]
> > The question is, what do we do on those platforms? Use setlocale()
> > or
> > fallback to (a) and document that ecpg has to run in a C locale?
>
> No, we shouldn't use setlocale(), because it clearly is hazardous
> even on platforms where it doesn't fail outright. I don't see
> anything so wrong with just documenting the hazard. The situation
Actually I meant using setlocale() and documenting that it must not be
used with threads, or document it must not be used with locales?
Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-20 17:30 Tom Lane <[email protected]>
parent: Michael Meskes <[email protected]>
0 siblings, 1 reply; 47+ messages in thread
From: Tom Lane @ 2019-01-20 17:30 UTC (permalink / raw)
To: Michael Meskes <[email protected]>; +Cc: [email protected]
Michael Meskes <[email protected]> writes:
>> No, we shouldn't use setlocale(), because it clearly is hazardous
>> even on platforms where it doesn't fail outright. I don't see
>> anything so wrong with just documenting the hazard. The situation
> Actually I meant using setlocale() and documenting that it must not be
> used with threads, or document it must not be used with locales?
I tend to think that has more downside than upside, in situations where
people don't read the manual closely and try to do it anyway.
First, there's the probable crash if setlocale() is thread-unsafe.
(Though the lack of previous reports suggests that on most platforms,
it isn't.)
Second, if the program is indeed trying to run with non-C LC_NUMERIC,
using setlocale() will have unsynchronized, hard-to-debug side effects
on other threads. Not using it will have no downside at all if ecpg
isn't trying to read numeric data, while if it does do so, the failures
will be reproducible and easy to understand/debug.
Admittedly, removing the setlocale() call will be a net negative for
single-threaded applications, which are likely the majority. But
I don't know any safe way to tell whether the app is multi threaded.
On the third hand, the lack of previous reports suggests that maybe
this whole thing is seldom a problem in practice. Maybe we should
just use uselocale() where available and otherwise hope it's OK
to keep on doing what we were doing.
regards, tom lane
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-20 20:04 Tom Lane <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 47+ messages in thread
From: Tom Lane @ 2019-01-20 20:04 UTC (permalink / raw)
To: Michael Meskes <[email protected]>; +Cc: [email protected]
I wrote:
> On the third hand, the lack of previous reports suggests that maybe
> this whole thing is seldom a problem in practice. Maybe we should
> just use uselocale() where available and otherwise hope it's OK
> to keep on doing what we were doing.
If we go with that approach, I think we need to adapt the patch
as attached. I autoconfiscated it and fixed a portability problem
(it didn't compile on macOS, which has these decls in <xlocale.h>).
I've verified that this fixes the problem I was seeing on OpenBSD 6.4.
I've not bothered to test on a platform lacking uselocale() --- I
think it's clear by inspection that the patch doesn't change anything
in that case.
Not sure if we need to document this or not. On platforms with
uselocale(), it should fix the problem without any need for user
attention. On platforms without, there's no change, and given
the lack of previous complaints I'm not sure it's really an issue.
regards, tom lane
Attachments:
[text/x-diff] ecpg_locale-2.patch (5.2K, ../../[email protected]/2-ecpg_locale-2.patch)
download | inline diff:
diff --git a/configure b/configure
index 7602e65..1e69eda 100755
*** a/configure
--- b/configure
*************** fi
*** 15209,15215 ****
LIBS_including_readline="$LIBS"
LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'`
! for ac_func in cbrt clock_gettime copyfile fdatasync getifaddrs getpeerucred getrlimit mbstowcs_l memmove poll posix_fallocate ppoll pstat pthread_is_threaded_np readlink setproctitle setproctitle_fast setsid shm_open strchrnul strsignal symlink sync_file_range utime utimes wcstombs_l
do :
as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
--- 15209,15215 ----
LIBS_including_readline="$LIBS"
LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'`
! for ac_func in cbrt clock_gettime copyfile fdatasync getifaddrs getpeerucred getrlimit mbstowcs_l memmove poll posix_fallocate ppoll pstat pthread_is_threaded_np readlink setproctitle setproctitle_fast setsid shm_open strchrnul strsignal symlink sync_file_range uselocale utime utimes wcstombs_l
do :
as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
diff --git a/configure.in b/configure.in
index d599ad8..556186c 100644
*** a/configure.in
--- b/configure.in
*************** AC_CHECK_FUNCS(m4_normalize([
*** 1618,1623 ****
--- 1618,1624 ----
strsignal
symlink
sync_file_range
+ uselocale
utime
utimes
wcstombs_l
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 9d99816..2c899a1 100644
*** a/src/include/pg_config.h.in
--- b/src/include/pg_config.h.in
***************
*** 691,696 ****
--- 691,699 ----
/* Define to 1 if the system has the type `unsigned long long int'. */
#undef HAVE_UNSIGNED_LONG_LONG_INT
+ /* Define to 1 if you have the `uselocale' function. */
+ #undef HAVE_USELOCALE
+
/* Define to 1 if you have the `utime' function. */
#undef HAVE_UTIME
diff --git a/src/include/pg_config.h.win32 b/src/include/pg_config.h.win32
index 8a560ef..3964433 100644
*** a/src/include/pg_config.h.win32
--- b/src/include/pg_config.h.win32
***************
*** 545,550 ****
--- 545,553 ----
/* Define to 1 if you have the `unsetenv' function. */
/* #undef HAVE_UNSETENV */
+ /* Define to 1 if you have the `uselocale' function. */
+ /* #undef HAVE_USELOCALE */
+
/* Define to 1 if you have the `utime' function. */
#define HAVE_UTIME 1
diff --git a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
index 1c9bce1..22a0557 100644
*** a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
--- b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
***************
*** 12,17 ****
--- 12,20 ----
#ifndef CHAR_BIT
#include <limits.h>
#endif
+ #ifdef LOCALE_T_IN_XLOCALE
+ #include <xlocale.h>
+ #endif
enum COMPAT_MODE
{
*************** struct statement
*** 61,67 ****
--- 64,75 ----
bool questionmarks;
struct variable *inlist;
struct variable *outlist;
+ #ifdef HAVE_USELOCALE
+ locale_t clocale;
+ locale_t oldlocale;
+ #else
char *oldlocale;
+ #endif
int nparams;
char **paramvalues;
PGresult *results;
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index 3f5034e..d3e32d2 100644
*** a/src/interfaces/ecpg/ecpglib/execute.c
--- b/src/interfaces/ecpg/ecpglib/execute.c
*************** free_statement(struct statement *stmt)
*** 102,108 ****
--- 102,113 ----
free_variable(stmt->outlist);
ecpg_free(stmt->command);
ecpg_free(stmt->name);
+ #ifdef HAVE_USELOCALE
+ if (stmt->clocale)
+ freelocale(stmt->clocale);
+ #else
ecpg_free(stmt->oldlocale);
+ #endif
ecpg_free(stmt);
}
*************** ecpg_do_prologue(int lineno, const int c
*** 1771,1778 ****
/*
* Make sure we do NOT honor the locale for numeric input/output since the
! * database wants the standard decimal point
*/
stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
if (stmt->oldlocale == NULL)
{
--- 1776,1798 ----
/*
* Make sure we do NOT honor the locale for numeric input/output since the
! * database wants the standard decimal point. If available, use
! * uselocale() for this because it's thread-safe.
*/
+ #ifdef HAVE_USELOCALE
+ stmt->clocale = newlocale(LC_NUMERIC_MASK, "C", (locale_t) 0);
+ if (stmt->clocale == (locale_t) 0)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
+ stmt->oldlocale = uselocale(stmt->clocale);
+ if (stmt->oldlocale == (locale_t) 0)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
+ #else
stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
if (stmt->oldlocale == NULL)
{
*************** ecpg_do_prologue(int lineno, const int c
*** 1780,1785 ****
--- 1800,1806 ----
return false;
}
setlocale(LC_NUMERIC, "C");
+ #endif
#ifdef ENABLE_THREAD_SAFETY
ecpg_pthreads_init();
*************** ecpg_do_epilogue(struct statement *stmt)
*** 1982,1989 ****
--- 2003,2015 ----
if (stmt == NULL)
return;
+ #ifdef HAVE_USELOCALE
+ if (stmt->oldlocale)
+ uselocale(stmt->oldlocale);
+ #else
if (stmt->oldlocale)
setlocale(LC_NUMERIC, stmt->oldlocale);
+ #endif
free_statement(stmt);
}
^ permalink raw reply [nested|flat] 47+ messages in thread
* RE: Thread-unsafe coding in ecpg
@ 2019-01-21 02:14 Tsunakawa, Takayuki <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 47+ messages in thread
From: Tsunakawa, Takayuki @ 2019-01-21 02:14 UTC (permalink / raw)
To: 'Tom Lane' <[email protected]>; Michael Meskes <[email protected]>; +Cc: [email protected] <[email protected]>
On Windows, _configthreadlocale() enables us to restrict the effect of setlocale() only to the calling thread. We can call it in ecpg_do_prolog/epilog().
https://docs.microsoft.com/en-us/cpp/parallel/multithreading-and-locales?view=vs-2017
Regards
Takayuki Tsunakawa
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-21 02:20 Tom Lane <[email protected]>
parent: Tsunakawa, Takayuki <[email protected]>
0 siblings, 1 reply; 47+ messages in thread
From: Tom Lane @ 2019-01-21 02:20 UTC (permalink / raw)
To: Tsunakawa, Takayuki <[email protected]>; +Cc: Michael Meskes <[email protected]>; [email protected] <[email protected]>
"Tsunakawa, Takayuki" <[email protected]> writes:
> On Windows, _configthreadlocale() enables us to restrict the effect of setlocale() only to the calling thread. We can call it in ecpg_do_prolog/epilog().
How far back does that exist?
regards, tom lane
^ permalink raw reply [nested|flat] 47+ messages in thread
* RE: Thread-unsafe coding in ecpg
@ 2019-01-21 02:57 Tsunakawa, Takayuki <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 47+ messages in thread
From: Tsunakawa, Takayuki @ 2019-01-21 02:57 UTC (permalink / raw)
To: 'Tom Lane' <[email protected]>; +Cc: Michael Meskes <[email protected]>; [email protected] <[email protected]>
From: Tom Lane [mailto:[email protected]]
> "Tsunakawa, Takayuki" <[email protected]> writes:
> > On Windows, _configthreadlocale() enables us to restrict the effect of
> setlocale() only to the calling thread. We can call it in
> ecpg_do_prolog/epilog().
>
> How far back does that exist?
I couldn't find the relevant doc, but I've just confirmed I can use it with Visual Studio 2008 on Win7, which is my oldest combination at hand. VS 2008 is already past its EOL, and the support for Win7 will end next year, so the combination is practically enough.
Regards
Takayuki Tsunakawa
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-21 03:16 Tom Lane <[email protected]>
parent: Tsunakawa, Takayuki <[email protected]>
0 siblings, 1 reply; 47+ messages in thread
From: Tom Lane @ 2019-01-21 03:16 UTC (permalink / raw)
To: Tsunakawa, Takayuki <[email protected]>; +Cc: Michael Meskes <[email protected]>; [email protected] <[email protected]>
"Tsunakawa, Takayuki" <[email protected]> writes:
> From: Tom Lane [mailto:[email protected]]
>> How far back does that exist?
> I couldn't find the relevant doc, but I've just confirmed I can use it with Visual Studio 2008 on Win7, which is my oldest combination at hand. VS 2008 is already past its EOL, and the support for Win7 will end next year, so the combination is practically enough.
Hm. Well, I suppose we can figure that the buildfarm should tell us
if there's anything too old that we still care about.
So like this ...
regards, tom lane
Attachments:
[text/x-diff] ecpg_locale-3.patch (5.5K, ../../[email protected]/2-ecpg_locale-3.patch)
download | inline diff:
diff --git a/configure b/configure
index 7602e65..1e69eda 100755
*** a/configure
--- b/configure
*************** fi
*** 15209,15215 ****
LIBS_including_readline="$LIBS"
LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'`
! for ac_func in cbrt clock_gettime copyfile fdatasync getifaddrs getpeerucred getrlimit mbstowcs_l memmove poll posix_fallocate ppoll pstat pthread_is_threaded_np readlink setproctitle setproctitle_fast setsid shm_open strchrnul strsignal symlink sync_file_range utime utimes wcstombs_l
do :
as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
--- 15209,15215 ----
LIBS_including_readline="$LIBS"
LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'`
! for ac_func in cbrt clock_gettime copyfile fdatasync getifaddrs getpeerucred getrlimit mbstowcs_l memmove poll posix_fallocate ppoll pstat pthread_is_threaded_np readlink setproctitle setproctitle_fast setsid shm_open strchrnul strsignal symlink sync_file_range uselocale utime utimes wcstombs_l
do :
as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
diff --git a/configure.in b/configure.in
index d599ad8..556186c 100644
*** a/configure.in
--- b/configure.in
*************** AC_CHECK_FUNCS(m4_normalize([
*** 1618,1623 ****
--- 1618,1624 ----
strsignal
symlink
sync_file_range
+ uselocale
utime
utimes
wcstombs_l
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 9d99816..2c899a1 100644
*** a/src/include/pg_config.h.in
--- b/src/include/pg_config.h.in
***************
*** 691,696 ****
--- 691,699 ----
/* Define to 1 if the system has the type `unsigned long long int'. */
#undef HAVE_UNSIGNED_LONG_LONG_INT
+ /* Define to 1 if you have the `uselocale' function. */
+ #undef HAVE_USELOCALE
+
/* Define to 1 if you have the `utime' function. */
#undef HAVE_UTIME
diff --git a/src/include/pg_config.h.win32 b/src/include/pg_config.h.win32
index 8a560ef..3964433 100644
*** a/src/include/pg_config.h.win32
--- b/src/include/pg_config.h.win32
***************
*** 545,550 ****
--- 545,553 ----
/* Define to 1 if you have the `unsetenv' function. */
/* #undef HAVE_UNSETENV */
+ /* Define to 1 if you have the `uselocale' function. */
+ /* #undef HAVE_USELOCALE */
+
/* Define to 1 if you have the `utime' function. */
#define HAVE_UTIME 1
diff --git a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
index 1c9bce1..41851d5 100644
*** a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
--- b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
***************
*** 12,17 ****
--- 12,20 ----
#ifndef CHAR_BIT
#include <limits.h>
#endif
+ #ifdef LOCALE_T_IN_XLOCALE
+ #include <xlocale.h>
+ #endif
enum COMPAT_MODE
{
*************** struct statement
*** 61,67 ****
--- 64,78 ----
bool questionmarks;
struct variable *inlist;
struct variable *outlist;
+ #ifdef HAVE_USELOCALE
+ locale_t clocale;
+ locale_t oldlocale;
+ #else
char *oldlocale;
+ #ifdef WIN32
+ int oldthreadlocale;
+ #endif
+ #endif
int nparams;
char **paramvalues;
PGresult *results;
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index 3f5034e..f67d774 100644
*** a/src/interfaces/ecpg/ecpglib/execute.c
--- b/src/interfaces/ecpg/ecpglib/execute.c
*************** free_statement(struct statement *stmt)
*** 102,108 ****
--- 102,113 ----
free_variable(stmt->outlist);
ecpg_free(stmt->command);
ecpg_free(stmt->name);
+ #ifdef HAVE_USELOCALE
+ if (stmt->clocale)
+ freelocale(stmt->clocale);
+ #else
ecpg_free(stmt->oldlocale);
+ #endif
ecpg_free(stmt);
}
*************** ecpg_do_prologue(int lineno, const int c
*** 1771,1778 ****
/*
* Make sure we do NOT honor the locale for numeric input/output since the
! * database wants the standard decimal point
*/
stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
if (stmt->oldlocale == NULL)
{
--- 1776,1806 ----
/*
* Make sure we do NOT honor the locale for numeric input/output since the
! * database wants the standard decimal point. If available, use
! * uselocale() for this because it's thread-safe.
*/
+ #ifdef HAVE_USELOCALE
+ stmt->clocale = newlocale(LC_NUMERIC_MASK, "C", (locale_t) 0);
+ if (stmt->clocale == (locale_t) 0)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
+ stmt->oldlocale = uselocale(stmt->clocale);
+ if (stmt->oldlocale == (locale_t) 0)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
+ #else
+ #ifdef WIN32
+ stmt->oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
+ if (stmt->oldthreadlocale == -1)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
+ #endif
stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
if (stmt->oldlocale == NULL)
{
*************** ecpg_do_prologue(int lineno, const int c
*** 1780,1785 ****
--- 1808,1814 ----
return false;
}
setlocale(LC_NUMERIC, "C");
+ #endif
#ifdef ENABLE_THREAD_SAFETY
ecpg_pthreads_init();
*************** ecpg_do_epilogue(struct statement *stmt)
*** 1982,1989 ****
--- 2011,2028 ----
if (stmt == NULL)
return;
+ #ifdef HAVE_USELOCALE
+ if (stmt->oldlocale != (locale_t) 0)
+ uselocale(stmt->oldlocale);
+ #else
if (stmt->oldlocale)
+ {
setlocale(LC_NUMERIC, stmt->oldlocale);
+ #ifdef WIN32
+ _configthreadlocale(stmt->oldthreadlocale);
+ #endif
+ }
+ #endif
free_statement(stmt);
}
^ permalink raw reply [nested|flat] 47+ messages in thread
* RE: Thread-unsafe coding in ecpg
@ 2019-01-21 06:07 Tsunakawa, Takayuki <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 47+ messages in thread
From: Tsunakawa, Takayuki @ 2019-01-21 06:07 UTC (permalink / raw)
To: 'Tom Lane' <[email protected]>; +Cc: Michael Meskes <[email protected]>; [email protected] <[email protected]>
From: Tom Lane [mailto:[email protected]]
> Hm. Well, I suppose we can figure that the buildfarm should tell us if
> there's anything too old that we still care about.
>
> So like this ...
How quick! Thank you. I've reviewed the code for both Unix and Windows, and it looks OK. I haven't built the patch, but expect the buildfarm to do the test.
Regards
Takayuki Tsunakawa
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-21 17:09 Tom Lane <[email protected]>
parent: Tsunakawa, Takayuki <[email protected]>
0 siblings, 3 replies; 47+ messages in thread
From: Tom Lane @ 2019-01-21 17:09 UTC (permalink / raw)
To: Tsunakawa, Takayuki <[email protected]>; +Cc: Michael Meskes <[email protected]>; [email protected] <[email protected]>
"Tsunakawa, Takayuki" <[email protected]> writes:
> From: Tom Lane [mailto:[email protected]]
>> So like this ...
> How quick! Thank you. I've reviewed the code for both Unix and Windows, and it looks OK. I haven't built the patch, but expect the buildfarm to do the test.
Thanks for reviewing! I've pushed this now (to HEAD only for the moment),
we'll see what the buildfarm thinks.
BTW, I found another spot in descriptor.c where ecpglib is using
setlocale() for the same purpose. Perhaps that one's not reachable
in threaded apps, but I didn't see any obvious reason to think so,
so I changed it too.
regards, tom lane
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-21 17:51 Michael Meskes <[email protected]>
parent: Tom Lane <[email protected]>
2 siblings, 0 replies; 47+ messages in thread
From: Michael Meskes @ 2019-01-21 17:51 UTC (permalink / raw)
To: Tom Lane <[email protected]>; Tsunakawa, Takayuki <[email protected]>; +Cc: [email protected] <[email protected]>
> Thanks for reviewing! I've pushed this now (to HEAD only for the
> moment),
> we'll see what the buildfarm thinks.
>
> BTW, I found another spot in descriptor.c where ecpglib is using
> setlocale() for the same purpose. Perhaps that one's not reachable
> in threaded apps, but I didn't see any obvious reason to think so,
> so I changed it too.
Thanks Tom.
Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-21 19:35 Andres Freund <[email protected]>
parent: Tom Lane <[email protected]>
2 siblings, 1 reply; 47+ messages in thread
From: Andres Freund @ 2019-01-21 19:35 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Tsunakawa, Takayuki <[email protected]>; Michael Meskes <[email protected]>; [email protected] <[email protected]>
On 2019-01-21 12:09:30 -0500, Tom Lane wrote:
> "Tsunakawa, Takayuki" <[email protected]> writes:
> > From: Tom Lane [mailto:[email protected]]
> >> So like this ...
>
> > How quick! Thank you. I've reviewed the code for both Unix and Windows, and it looks OK. I haven't built the patch, but expect the buildfarm to do the test.
>
> Thanks for reviewing! I've pushed this now (to HEAD only for the moment),
> we'll see what the buildfarm thinks.
>
> BTW, I found another spot in descriptor.c where ecpglib is using
> setlocale() for the same purpose. Perhaps that one's not reachable
> in threaded apps, but I didn't see any obvious reason to think so,
> so I changed it too.
Seems jacana might not have like this change?
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2019-01-21%2019%3A01%3A28
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-21 20:05 Tom Lane <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 2 replies; 47+ messages in thread
From: Tom Lane @ 2019-01-21 20:05 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Tsunakawa, Takayuki <[email protected]>; Michael Meskes <[email protected]>; [email protected] <[email protected]>
Andres Freund <[email protected]> writes:
> Seems jacana might not have like this change?
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2019-01-21%2019%3A01%3A28
Hmm. So mingw doesn't provide access to _configthreadlocale().
That's unfortunate, at least if we think that mingw is still a viable
production platform, because it means we can't make ecpg thread-safe
on that platform.
Is there a newer version of mingw that does have this functionality?
I'm not sure whether to install a version check or just assume that
it's never there.
regards, tom lane
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-21 20:18 Joshua D. Drake <[email protected]>
parent: Tom Lane <[email protected]>
1 sibling, 1 reply; 47+ messages in thread
From: Joshua D. Drake @ 2019-01-21 20:18 UTC (permalink / raw)
To: Tom Lane <[email protected]>; Andres Freund <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Tsunakawa, Takayuki <[email protected]>; Michael Meskes <[email protected]>; [email protected] <[email protected]>
On 1/21/19 12:05 PM, Tom Lane wrote:
> Andres Freund <[email protected]> writes:
>> Seems jacana might not have like this change?
>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2019-01-21%2019%3A01%3A28
> Hmm. So mingw doesn't provide access to _configthreadlocale().
> That's unfortunate, at least if we think that mingw is still a viable
> production platform, because it means we can't make ecpg thread-safe
> on that platform.
>
> Is there a newer version of mingw that does have this functionality?
> I'm not sure whether to install a version check or just assume that
> it's never there.
Apparently this can be done with thee 64bit version:
https://stackoverflow.com/questions/33647271/how-to-use-configthreadlocale-in-mingw
JD
>
> regards, tom lane
>
--
Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc
PostgreSQL centered full stack support, consulting and development.
Advocate: @amplifypostgres || Learn and Network: https://postgresconf.org
*** A fault and talent of mine is to tell it exactly how it is. ***
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-21 20:21 Andres Freund <[email protected]>
parent: Tom Lane <[email protected]>
1 sibling, 1 reply; 47+ messages in thread
From: Andres Freund @ 2019-01-21 20:21 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Tsunakawa, Takayuki <[email protected]>; Michael Meskes <[email protected]>; [email protected] <[email protected]>
Hi,
On 2019-01-21 15:05:23 -0500, Tom Lane wrote:
> Andres Freund <[email protected]> writes:
> > Seems jacana might not have like this change?
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2019-01-21%2019%3A01%3A28
>
> Hmm. So mingw doesn't provide access to _configthreadlocale().
> That's unfortunate, at least if we think that mingw is still a viable
> production platform, because it means we can't make ecpg thread-safe
> on that platform.
>
> Is there a newer version of mingw that does have this functionality?
> I'm not sure whether to install a version check or just assume that
> it's never there.
It does seem like newer versions do have it:
https://sourceforge.net/p/mingw-w64/mailman/message/34765722/
https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/mingw-w64-crt/misc/_configthreadlocale....
We could just refuse to support thread safety on mingw if that's not
supported? Or is that too aggressive?
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-21 20:25 Tom Lane <[email protected]>
parent: Joshua D. Drake <[email protected]>
0 siblings, 1 reply; 47+ messages in thread
From: Tom Lane @ 2019-01-21 20:25 UTC (permalink / raw)
To: Joshua D. Drake <[email protected]>; +Cc: Andres Freund <[email protected]>; Andrew Dunstan <[email protected]>; Tsunakawa, Takayuki <[email protected]>; Michael Meskes <[email protected]>; [email protected] <[email protected]>
"Joshua D. Drake" <[email protected]> writes:
> On 1/21/19 12:05 PM, Tom Lane wrote:
>> Is there a newer version of mingw that does have this functionality?
> Apparently this can be done with thee 64bit version:
> https://stackoverflow.com/questions/33647271/how-to-use-configthreadlocale-in-mingw
Hmm, the followup question makes it sound like it still didn't work :-(.
However, since the mingw build is autoconf-based, seems like we can
install a configure check instead of guessing. Will make it so.
Task for somebody else: run a MinGW64 buildfarm member.
regards, tom lane
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-21 20:45 Tom Lane <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 0 replies; 47+ messages in thread
From: Tom Lane @ 2019-01-21 20:45 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Tsunakawa, Takayuki <[email protected]>; Michael Meskes <[email protected]>; [email protected] <[email protected]>
Andres Freund <[email protected]> writes:
> We could just refuse to support thread safety on mingw if that's not
> supported? Or is that too aggressive?
Nah, we already had that discussion upthread. Given the lack of
prior complaints, we shouldn't break cases that are working today.
For instance, as long as setlocale() isn't actively thread-unsafe
and you are running with LC_NUMERIC=C as the prevailing locale,
the existing code doesn't pose a hazard even in a threaded app.
Forcing users for whom that's true to --disable-thread-safety
would turn an OK situation into a broken one.
regards, tom lane
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-22 03:00 Andrew Dunstan <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 47+ messages in thread
From: Andrew Dunstan @ 2019-01-22 03:00 UTC (permalink / raw)
To: Tom Lane <[email protected]>; Joshua D. Drake <[email protected]>; +Cc: Andres Freund <[email protected]>; Tsunakawa, Takayuki <[email protected]>; Michael Meskes <[email protected]>; [email protected] <[email protected]>
On 1/21/19 3:25 PM, Tom Lane wrote:
> "Joshua D. Drake" <[email protected]> writes:
>> On 1/21/19 12:05 PM, Tom Lane wrote:
>>> Is there a newer version of mingw that does have this functionality?
>> Apparently this can be done with thee 64bit version:
>> https://stackoverflow.com/questions/33647271/how-to-use-configthreadlocale-in-mingw
> Hmm, the followup question makes it sound like it still didn't work :-(.
>
> However, since the mingw build is autoconf-based, seems like we can
> install a configure check instead of guessing. Will make it so.
>
> Task for somebody else: run a MinGW64 buildfarm member.
>
>
I could set that up in just about two shakes of a lamb's tail - I have a
script to do so all tested on vagrant/aws within the last few months.
What I don't have is resources. My Windows resources are pretty much
tapped out. I would need either some modest hardware or a Windows VM
somewhere in the cloud - could be anywhere but I'm most at home on AWS.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 47+ messages in thread
* RE: Thread-unsafe coding in ecpg
@ 2019-01-22 06:57 Tsunakawa, Takayuki <[email protected]>
parent: Tom Lane <[email protected]>
2 siblings, 0 replies; 47+ messages in thread
From: Tsunakawa, Takayuki @ 2019-01-22 06:57 UTC (permalink / raw)
To: 'Tom Lane' <[email protected]>; +Cc: Michael Meskes <[email protected]>; [email protected] <[email protected]>
From: Tom Lane [mailto:[email protected]]
> BTW, I found another spot in descriptor.c where ecpglib is using
> setlocale() for the same purpose. Perhaps that one's not reachable
> in threaded apps, but I didn't see any obvious reason to think so,
> so I changed it too.
Ouch, thanks. And I'm sorry to annoy you by pointing out a trivial thing: in v3 patch, _configthreadlocale() is not called to restore the original value when setlocale() or ecpg_strdup() fails. I hope this is fixed in v4.
+ #ifdef WIN32
+ stmt->oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
+ if (stmt->oldthreadlocale == -1)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
+ #endif
stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
if (stmt->oldlocale == NULL)
{
Regards
Takayuki Tsunakawa
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-22 17:50 Andrew Dunstan <[email protected]>
parent: Andrew Dunstan <[email protected]>
0 siblings, 2 replies; 47+ messages in thread
From: Andrew Dunstan @ 2019-01-22 17:50 UTC (permalink / raw)
To: Tom Lane <[email protected]>; Joshua D. Drake <[email protected]>; +Cc: Andres Freund <[email protected]>; Tsunakawa, Takayuki <[email protected]>; Michael Meskes <[email protected]>; [email protected] <[email protected]>
On 1/21/19 10:00 PM, Andrew Dunstan wrote:
> On 1/21/19 3:25 PM, Tom Lane wrote:
>> "Joshua D. Drake" <[email protected]> writes:
>>> On 1/21/19 12:05 PM, Tom Lane wrote:
>>>> Is there a newer version of mingw that does have this functionality?
>>> Apparently this can be done with thee 64bit version:
>>> https://stackoverflow.com/questions/33647271/how-to-use-configthreadlocale-in-mingw
>> Hmm, the followup question makes it sound like it still didn't work :-(.
>>
>> However, since the mingw build is autoconf-based, seems like we can
>> install a configure check instead of guessing. Will make it so.
>>
>> Task for somebody else: run a MinGW64 buildfarm member.
>>
>>
>
> I could set that up in just about two shakes of a lamb's tail - I have a
> script to do so all tested on vagrant/aws within the last few months.
>
>
> What I don't have is resources. My Windows resources are pretty much
> tapped out. I would need either some modest hardware or a Windows VM
> somewhere in the cloud - could be anywhere but I'm most at home on AWS.
>
Incidentally, jacana *is* running mingw64, but possibly not a young
enough version. I just ran a test on an up to date version and it found
the config setting happily, and passed the make stage.
I can look at upgrading jacana to a more modern compiler. The version
it's running is 4.9.1, apparently built around Oct 2014, so it's not
that old.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-22 17:56 Andres Freund <[email protected]>
parent: Andrew Dunstan <[email protected]>
1 sibling, 0 replies; 47+ messages in thread
From: Andres Freund @ 2019-01-22 17:56 UTC (permalink / raw)
To: Andrew Dunstan <[email protected]>; Tom Lane <[email protected]>; Joshua D. Drake <[email protected]>; +Cc: Tsunakawa, Takayuki <[email protected]>; Michael Meskes <[email protected]>; [email protected] <[email protected]>
On January 22, 2019 9:50:19 AM PST, Andrew Dunstan <[email protected]> wrote:
>
>On 1/21/19 10:00 PM, Andrew Dunstan wrote:
>> On 1/21/19 3:25 PM, Tom Lane wrote:
>>> "Joshua D. Drake" <[email protected]> writes:
>>>> On 1/21/19 12:05 PM, Tom Lane wrote:
>>>>> Is there a newer version of mingw that does have this
>functionality?
>>>> Apparently this can be done with thee 64bit version:
>>>>
>https://stackoverflow.com/questions/33647271/how-to-use-configthreadlocale-in-mingw
>>> Hmm, the followup question makes it sound like it still didn't work
>:-(.
>>>
>>> However, since the mingw build is autoconf-based, seems like we can
>>> install a configure check instead of guessing. Will make it so.
>>>
>>> Task for somebody else: run a MinGW64 buildfarm member.
>>>
>>>
>>
>> I could set that up in just about two shakes of a lamb's tail - I
>have a
>> script to do so all tested on vagrant/aws within the last few months.
>>
>>
>> What I don't have is resources. My Windows resources are pretty much
>> tapped out. I would need either some modest hardware or a Windows VM
>> somewhere in the cloud - could be anywhere but I'm most at home on
>AWS.
>>
>
>
>Incidentally, jacana *is* running mingw64, but possibly not a young
>enough version. I just ran a test on an up to date version and it found
>the config setting happily, and passed the make stage.
>
>
>I can look at upgrading jacana to a more modern compiler. The version
>it's running is 4.9.1, apparently built around Oct 2014, so it's not
>that old.
The thread about the introduction is from 2016, so that's kind of expected.
Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-23 22:37 Andrew Dunstan <[email protected]>
parent: Andrew Dunstan <[email protected]>
1 sibling, 1 reply; 47+ messages in thread
From: Andrew Dunstan @ 2019-01-23 22:37 UTC (permalink / raw)
To: Tom Lane <[email protected]>; Joshua D. Drake <[email protected]>; +Cc: Andres Freund <[email protected]>; Tsunakawa, Takayuki <[email protected]>; Michael Meskes <[email protected]>; [email protected] <[email protected]>
On 1/22/19 12:50 PM, Andrew Dunstan wrote:
> On 1/21/19 10:00 PM, Andrew Dunstan wrote:
>> On 1/21/19 3:25 PM, Tom Lane wrote:
>>> "Joshua D. Drake" <[email protected]> writes:
>>>> On 1/21/19 12:05 PM, Tom Lane wrote:
>>>>> Is there a newer version of mingw that does have this functionality?
>>>> Apparently this can be done with thee 64bit version:
>>>> https://stackoverflow.com/questions/33647271/how-to-use-configthreadlocale-in-mingw
>>> Hmm, the followup question makes it sound like it still didn't work :-(.
>>>
>>> However, since the mingw build is autoconf-based, seems like we can
>>> install a configure check instead of guessing. Will make it so.
>>>
>>> Task for somebody else: run a MinGW64 buildfarm member.
>>>
>>>
>> I could set that up in just about two shakes of a lamb's tail - I have a
>> script to do so all tested on vagrant/aws within the last few months.
>>
>>
>> What I don't have is resources. My Windows resources are pretty much
>> tapped out. I would need either some modest hardware or a Windows VM
>> somewhere in the cloud - could be anywhere but I'm most at home on AWS.
>>
>
> Incidentally, jacana *is* running mingw64, but possibly not a young
> enough version. I just ran a test on an up to date version and it found
> the config setting happily, and passed the make stage.
>
>
> I can look at upgrading jacana to a more modern compiler. The version
> it's running is 4.9.1, apparently built around Oct 2014, so it's not
> that old.
>
>
I have just spent a large amount of time testing the committed fix with
a number of versions of gcc. It blows up on any compiler modern enough
to know about _configthreadlocale
Essentially what happens is this:
============== running regression test queries ==============
test compat_informix/dec_test ... ok
test compat_informix/charfuncs ... ok
test compat_informix/rfmtdate ... ok
test compat_informix/rfmtlong ... ok
test compat_informix/rnull ... stdout stderr FAILED
test compat_informix/sqlda ... stdout stderr FAILED (test
process exited with exit code 1)
test compat_informix/describe ... stdout stderr FAILED (test
process exited with exit code 1)
test compat_informix/test_informix ...
At this point the regression tests hang and the disk starts filling up
rapidly.
This has been tested with versions 5.4.0, 6.4.0, 7.3.0, 8.1.0 and 8.2.1.
The first 4 are downloaded direct from the Mingw64 repo, the last is
from Msys2's mingw64 toolchain.
I'm trying to find out more info, but what we have right now is pretty
broken.
Reverting commits ee27584c4a and 8eb4a9312 cures[*] the problem.
cheers
andrew
[*] FSVO "cure". I am still getting intermittent errors but no hangs.
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-23 23:01 Tom Lane <[email protected]>
parent: Andrew Dunstan <[email protected]>
0 siblings, 1 reply; 47+ messages in thread
From: Tom Lane @ 2019-01-23 23:01 UTC (permalink / raw)
To: Andrew Dunstan <[email protected]>; +Cc: Joshua D. Drake <[email protected]>; Andres Freund <[email protected]>; Tsunakawa, Takayuki <[email protected]>; Michael Meskes <[email protected]>; [email protected] <[email protected]>
Andrew Dunstan <[email protected]> writes:
> I have just spent a large amount of time testing the committed fix with
> a number of versions of gcc. It blows up on any compiler modern enough
> to know about _configthreadlocale
Bleah. Since the regular Windows buildfarm members seem happy, this
evidently means that MinGW's _configthreadlocale is broken in some way.
I suppose we could just remove the autoconf test and build without
_configthreadlocale on MinGW, but that's kind of sad ...
Perhaps there's some sort of setup that MinGW's version needs that
we're not doing?
regards, tom lane
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-24 00:28 Andrew Dunstan <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 47+ messages in thread
From: Andrew Dunstan @ 2019-01-24 00:28 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Joshua D. Drake <[email protected]>; Andres Freund <[email protected]>; Tsunakawa, Takayuki <[email protected]>; Michael Meskes <[email protected]>; [email protected] <[email protected]>
On 1/23/19 6:01 PM, Tom Lane wrote:
> Andrew Dunstan <[email protected]> writes:
>> I have just spent a large amount of time testing the committed fix with
>> a number of versions of gcc. It blows up on any compiler modern enough
>> to know about _configthreadlocale
> Bleah. Since the regular Windows buildfarm members seem happy, this
> evidently means that MinGW's _configthreadlocale is broken in some way.
>
> I suppose we could just remove the autoconf test and build without
> _configthreadlocale on MinGW, but that's kind of sad ...
>
> Perhaps there's some sort of setup that MinGW's version needs that
> we're not doing?
This seems to suggest something worse: https://reviews.llvm.org/D40181
Not sure I fully understand what's happening here, though.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-24 03:10 Tom Lane <[email protected]>
parent: Andrew Dunstan <[email protected]>
0 siblings, 1 reply; 47+ messages in thread
From: Tom Lane @ 2019-01-24 03:10 UTC (permalink / raw)
To: Andrew Dunstan <[email protected]>; +Cc: Joshua D. Drake <[email protected]>; Andres Freund <[email protected]>; Tsunakawa, Takayuki <[email protected]>; Michael Meskes <[email protected]>; [email protected] <[email protected]>
Andrew Dunstan <[email protected]> writes:
> On 1/23/19 6:01 PM, Tom Lane wrote:
>> Perhaps there's some sort of setup that MinGW's version needs that
>> we're not doing?
> This seems to suggest something worse: https://reviews.llvm.org/D40181
> Not sure I fully understand what's happening here, though.
Me either, but I noted a couple of interesting extracts from that page:
Normal mingw that uses msvcrt.dll doesn't have per-thread locales so
it won't really work in any case (but I think it does define some sort
of dummy functions that at least will allow it to build). Nowadays,
mingw can be built to target ucrtbase.dll as well though, and there it
should be possible to make it work just like for MSVC although it
might need some patches.
... Looked into MinGW-w64 sources and this is indeed the
case. _configthreadlocale will return -1 and will not do anything.
This suggests that, rather than throwing up our hands if the initial
_configthreadlocale call returns -1, we should act as though the function
doesn't exist, and just soldier on the same as before. The code in there
assumes that -1 is a can't-happen case and doesn't try to recover,
but apparently that's over-optimistic.
regards, tom lane
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-24 03:53 Tom Lane <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 4 replies; 47+ messages in thread
From: Tom Lane @ 2019-01-24 03:53 UTC (permalink / raw)
To: Andrew Dunstan <[email protected]>; +Cc: Joshua D. Drake <[email protected]>; Andres Freund <[email protected]>; Tsunakawa, Takayuki <[email protected]>; Michael Meskes <[email protected]>; [email protected] <[email protected]>
I wrote:
> This suggests that, rather than throwing up our hands if the initial
> _configthreadlocale call returns -1, we should act as though the function
> doesn't exist, and just soldier on the same as before. The code in there
> assumes that -1 is a can't-happen case and doesn't try to recover,
> but apparently that's over-optimistic.
I pushed a patch to fix that.
It looks to me like the reason that the ecpg tests went into an infinite
loop is that compat_informix/test_informix.pgc has not considered the
possibility of repeated statement failures:
while (1)
{
$fetch forward c into :i, :j, :c;
if (sqlca.sqlcode == 100) break;
else if (sqlca.sqlcode != 0) printf ("Error: %ld\n", sqlca.sqlcode);
if (risnull(CDECIMALTYPE, (char *)&j))
printf("%d NULL\n", i);
else
{
int a;
dectoint(&j, &a);
printf("%d %d \"%s\"\n", i, a, c);
}
}
I know zip about ecpg coding practices, but wouldn't it be a better idea
to break out of the loop on seeing an error?
regards, tom lane
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-24 05:23 Tom Lane <[email protected]>
parent: Tom Lane <[email protected]>
3 siblings, 0 replies; 47+ messages in thread
From: Tom Lane @ 2019-01-24 05:23 UTC (permalink / raw)
To: Michael Meskes <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Joshua D. Drake <[email protected]>; Andres Freund <[email protected]>; Tsunakawa, Takayuki <[email protected]>; [email protected] <[email protected]>
Oh, I just noticed something else: several of the ecpg test programs
contain
#ifdef WIN32
#ifdef _MSC_VER /* requires MSVC */
_configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
#endif
#endif
Surely this is a kluge that we could now remove? We've protected the
only setlocale calls that the ecpg tests could reach in threaded mode.
If there is still a problem, it'd behoove us to find it, because
ecpglib should not expect that the calling app has done this for it.
regards, tom lane
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-24 16:17 Andrew Dunstan <[email protected]>
parent: Tom Lane <[email protected]>
3 siblings, 0 replies; 47+ messages in thread
From: Andrew Dunstan @ 2019-01-24 16:17 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Joshua D. Drake <[email protected]>; Andres Freund <[email protected]>; Tsunakawa, Takayuki <[email protected]>; Michael Meskes <[email protected]>; [email protected] <[email protected]>
On 1/23/19 10:53 PM, Tom Lane wrote:
> I wrote:
>> This suggests that, rather than throwing up our hands if the initial
>> _configthreadlocale call returns -1, we should act as though the function
>> doesn't exist, and just soldier on the same as before. The code in there
>> assumes that -1 is a can't-happen case and doesn't try to recover,
>> but apparently that's over-optimistic.
> I pushed a patch to fix that.
jacana has been upgraded to gcc 8.1.0 so it knows about
_configthreadlocale() and it's now happy.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-26 04:25 Michael Meskes <[email protected]>
parent: Tom Lane <[email protected]>
3 siblings, 0 replies; 47+ messages in thread
From: Michael Meskes @ 2019-01-26 04:25 UTC (permalink / raw)
To: Tom Lane <[email protected]>; Andrew Dunstan <[email protected]>; +Cc: Joshua D. Drake <[email protected]>; Andres Freund <[email protected]>; Tsunakawa, Takayuki <[email protected]>; [email protected] <[email protected]>
> It looks to me like the reason that the ecpg tests went into an
> infinite
> loop is that compat_informix/test_informix.pgc has not considered the
> possibility of repeated statement failures:
> ...
Correct, this was missing a safeguard.
> I know zip about ecpg coding practices, but wouldn't it be a better
> idea
> to break out of the loop on seeing an error?
I wonder if it would be better to make the test cases use the proper
whenever command instead. That would give us a slightly better
functionality testing I'd say.
Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-26 14:57 Tom Lane <[email protected]>
parent: Tom Lane <[email protected]>
3 siblings, 1 reply; 47+ messages in thread
From: Tom Lane @ 2019-01-26 14:57 UTC (permalink / raw)
To: Michael Meskes <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Joshua D. Drake <[email protected]>; Andres Freund <[email protected]>; Tsunakawa, Takayuki <[email protected]>; [email protected] <[email protected]>
Michael Meskes <[email protected]> writes:
> I wonder if it would be better to make the test cases use the proper
> whenever command instead. That would give us a slightly better
> functionality testing I'd say.
I like having a hard limit on the number of loop iterations;
that should ensure that the test terminates no matter how confused
ecpglib is.
But certainly we could have more of the tests using "whenever"
as the intended method of getting out of the loop.
regards, tom lane
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Thread-unsafe coding in ecpg
@ 2019-01-29 11:58 Michael Meskes <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 0 replies; 47+ messages in thread
From: Michael Meskes @ 2019-01-29 11:58 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Joshua D. Drake <[email protected]>; Andres Freund <[email protected]>; Tsunakawa, Takayuki <[email protected]>; [email protected] <[email protected]>
> I like having a hard limit on the number of loop iterations;
> that should ensure that the test terminates no matter how confused
> ecpglib is.
I get your point and thus will only clean up the tests a little bit.
Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL
^ permalink raw reply [nested|flat] 47+ messages in thread
* [PATCH 4/6] Allow dsm to use on postmaster.
@ 2019-02-21 03:42 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 47+ messages in thread
From: Kyotaro Horiguchi @ 2019-02-21 03:42 UTC (permalink / raw)
DSM is inhibited to be used on postmaster. Shared memory baesd stats
collector needs it to work on postmaster and no problem found to do
that. Just allow it.
---
src/backend/storage/ipc/dsm.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/backend/storage/ipc/dsm.c b/src/backend/storage/ipc/dsm.c
index 23ccc59f13..d30a876bb0 100644
--- a/src/backend/storage/ipc/dsm.c
+++ b/src/backend/storage/ipc/dsm.c
@@ -440,8 +440,7 @@ dsm_create(Size size, int flags)
uint32 i;
uint32 nitems;
- /* Unsafe in postmaster (and pointless in a stand-alone backend). */
- Assert(IsUnderPostmaster);
+ Assert(dsm_control != NULL);
if (!dsm_init_done)
dsm_backend_startup();
@@ -537,8 +536,7 @@ dsm_attach(dsm_handle h)
uint32 i;
uint32 nitems;
- /* Unsafe in postmaster (and pointless in a stand-alone backend). */
- Assert(IsUnderPostmaster);
+ Assert(dsm_control != NULL);
if (!dsm_init_done)
dsm_backend_startup();
--
2.16.3
----Next_Part(Mon_Feb_25_13_52_14_2019_191)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v17-0005-Shared-memory-based-stats-collector.patch"
^ permalink raw reply [nested|flat] 47+ messages in thread
* [PATCH 4/6] Allow dsm to use on postmaster.
@ 2019-02-21 03:42 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 47+ messages in thread
From: Kyotaro Horiguchi @ 2019-02-21 03:42 UTC (permalink / raw)
DSM is inhibited to be used on postmaster. Shared memory baesd stats
collector needs it to work on postmaster and no problem found to do
that. Just allow it.
---
src/backend/storage/ipc/dsm.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/backend/storage/ipc/dsm.c b/src/backend/storage/ipc/dsm.c
index 23ccc59f13..d30a876bb0 100644
--- a/src/backend/storage/ipc/dsm.c
+++ b/src/backend/storage/ipc/dsm.c
@@ -440,8 +440,7 @@ dsm_create(Size size, int flags)
uint32 i;
uint32 nitems;
- /* Unsafe in postmaster (and pointless in a stand-alone backend). */
- Assert(IsUnderPostmaster);
+ Assert(dsm_control != NULL);
if (!dsm_init_done)
dsm_backend_startup();
@@ -537,8 +536,7 @@ dsm_attach(dsm_handle h)
uint32 i;
uint32 nitems;
- /* Unsafe in postmaster (and pointless in a stand-alone backend). */
- Assert(IsUnderPostmaster);
+ Assert(dsm_control != NULL);
if (!dsm_init_done)
dsm_backend_startup();
--
2.16.3
----Next_Part(Thu_Feb_21_16_05_55_2019_560)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v16-0005-Shared-memory-based-stats-collector.patch"
^ permalink raw reply [nested|flat] 47+ messages in thread
* [PATCH v6 1/7] Row pattern recognition patch for raw parser.
@ 2023-09-12 05:22 Tatsuo Ishii <[email protected]>
0 siblings, 0 replies; 47+ messages in thread
From: Tatsuo Ishii @ 2023-09-12 05:22 UTC (permalink / raw)
---
src/backend/parser/gram.y | 216 +++++++++++++++++++++++++++++---
src/include/nodes/parsenodes.h | 56 +++++++++
src/include/parser/kwlist.h | 8 ++
src/include/parser/parse_node.h | 1 +
4 files changed, 267 insertions(+), 14 deletions(-)
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 7d2032885e..70409cdc9a 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -251,6 +251,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
DefElem *defelt;
SortBy *sortby;
WindowDef *windef;
+ RPCommonSyntax *rpcom;
+ RPSubsetItem *rpsubset;
JoinExpr *jexpr;
IndexElem *ielem;
StatsElem *selem;
@@ -453,8 +455,12 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
TriggerTransitions TriggerReferencing
vacuum_relation_list opt_vacuum_relation_list
drop_option_list pub_obj_list
-
-%type <node> opt_routine_body
+ row_pattern_measure_list row_pattern_definition_list
+ opt_row_pattern_subset_clause
+ row_pattern_subset_list row_pattern_subset_rhs
+ row_pattern
+%type <rpsubset> row_pattern_subset_item
+%type <node> opt_routine_body row_pattern_term
%type <groupclause> group_clause
%type <list> group_by_list
%type <node> group_by_item empty_grouping_set rollup_clause cube_clause
@@ -551,6 +557,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type <range> relation_expr_opt_alias
%type <node> tablesample_clause opt_repeatable_clause
%type <target> target_el set_target insert_column_item
+ row_pattern_measure_item row_pattern_definition
%type <str> generic_option_name
%type <node> generic_option_arg
@@ -633,6 +640,9 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type <list> window_clause window_definition_list opt_partition_clause
%type <windef> window_definition over_clause window_specification
opt_frame_clause frame_extent frame_bound
+%type <rpcom> opt_row_pattern_common_syntax opt_row_pattern_skip_to
+%type <boolean> opt_row_pattern_initial_or_seek
+%type <list> opt_row_pattern_measures
%type <ival> opt_window_exclusion_clause
%type <str> opt_existing_window_name
%type <boolean> opt_if_not_exists
@@ -659,7 +669,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
json_object_constructor_null_clause_opt
json_array_constructor_null_clause_opt
-
/*
* Non-keyword token types. These are hard-wired into the "flex" lexer.
* They must be listed first so that their numeric codes do not depend on
@@ -702,7 +711,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
CURRENT_TIME CURRENT_TIMESTAMP CURRENT_USER CURSOR CYCLE
DATA_P DATABASE DAY_P DEALLOCATE DEC DECIMAL_P DECLARE DEFAULT DEFAULTS
- DEFERRABLE DEFERRED DEFINER DELETE_P DELIMITER DELIMITERS DEPENDS DEPTH DESC
+ DEFERRABLE DEFERRED DEFINE DEFINER DELETE_P DELIMITER DELIMITERS DEPENDS DEPTH DESC
DETACH DICTIONARY DISABLE_P DISCARD DISTINCT DO DOCUMENT_P DOMAIN_P
DOUBLE_P DROP
@@ -718,7 +727,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
HANDLER HAVING HEADER_P HOLD HOUR_P
IDENTITY_P IF_P ILIKE IMMEDIATE IMMUTABLE IMPLICIT_P IMPORT_P IN_P INCLUDE
- INCLUDING INCREMENT INDENT INDEX INDEXES INHERIT INHERITS INITIALLY INLINE_P
+ INCLUDING INCREMENT INDENT INDEX INDEXES INHERIT INHERITS INITIAL INITIALLY INLINE_P
INNER_P INOUT INPUT_P INSENSITIVE INSERT INSTEAD INT_P INTEGER
INTERSECT INTERVAL INTO INVOKER IS ISNULL ISOLATION
@@ -731,7 +740,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
LEADING LEAKPROOF LEAST LEFT LEVEL LIKE LIMIT LISTEN LOAD LOCAL
LOCALTIME LOCALTIMESTAMP LOCATION LOCK_P LOCKED LOGGED
- MAPPING MATCH MATCHED MATERIALIZED MAXVALUE MERGE METHOD
+ MAPPING MATCH MATCHED MATERIALIZED MAXVALUE MEASURES MERGE METHOD
MINUTE_P MINVALUE MODE MONTH_P MOVE
NAME_P NAMES NATIONAL NATURAL NCHAR NEW NEXT NFC NFD NFKC NFKD NO NONE
@@ -743,8 +752,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
ORDER ORDINALITY OTHERS OUT_P OUTER_P
OVER OVERLAPS OVERLAY OVERRIDING OWNED OWNER
- PARALLEL PARAMETER PARSER PARTIAL PARTITION PASSING PASSWORD
- PLACING PLANS POLICY
+ PARALLEL PARAMETER PARSER PARTIAL PARTITION PASSING PASSWORD PAST
+ PATTERN_P PERMUTE PLACING PLANS POLICY
POSITION PRECEDING PRECISION PRESERVE PREPARE PREPARED PRIMARY
PRIOR PRIVILEGES PROCEDURAL PROCEDURE PROCEDURES PROGRAM PUBLICATION
@@ -755,12 +764,13 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
RESET RESTART RESTRICT RETURN RETURNING RETURNS REVOKE RIGHT ROLE ROLLBACK ROLLUP
ROUTINE ROUTINES ROW ROWS RULE
- SAVEPOINT SCALAR SCHEMA SCHEMAS SCROLL SEARCH SECOND_P SECURITY SELECT
+ SAVEPOINT SCALAR SCHEMA SCHEMAS SCROLL SEARCH SECOND_P SECURITY SEEK SELECT
SEQUENCE SEQUENCES
+
SERIALIZABLE SERVER SESSION SESSION_USER SET SETS SETOF SHARE SHOW
SIMILAR SIMPLE SKIP SMALLINT SNAPSHOT SOME SQL_P STABLE STANDALONE_P
START STATEMENT STATISTICS STDIN STDOUT STORAGE STORED STRICT_P STRIP_P
- SUBSCRIPTION SUBSTRING SUPPORT SYMMETRIC SYSID SYSTEM_P SYSTEM_USER
+ SUBSCRIPTION SUBSET SUBSTRING SUPPORT SYMMETRIC SYSID SYSTEM_P SYSTEM_USER
TABLE TABLES TABLESAMPLE TABLESPACE TEMP TEMPLATE TEMPORARY TEXT_P THEN
TIES TIME TIMESTAMP TO TRAILING TRANSACTION TRANSFORM
@@ -853,6 +863,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
*/
%nonassoc UNBOUNDED /* ideally would have same precedence as IDENT */
%nonassoc IDENT PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP
+%nonassoc MEASURES AFTER INITIAL SEEK PATTERN_P
%left Op OPERATOR /* multi-character ops and user-defined operators */
%left '+' '-'
%left '*' '/' '%'
@@ -15857,7 +15868,8 @@ over_clause: OVER window_specification
;
window_specification: '(' opt_existing_window_name opt_partition_clause
- opt_sort_clause opt_frame_clause ')'
+ opt_sort_clause opt_row_pattern_measures opt_frame_clause
+ opt_row_pattern_common_syntax ')'
{
WindowDef *n = makeNode(WindowDef);
@@ -15865,10 +15877,12 @@ window_specification: '(' opt_existing_window_name opt_partition_clause
n->refname = $2;
n->partitionClause = $3;
n->orderClause = $4;
+ n->rowPatternMeasures = $5;
/* copy relevant fields of opt_frame_clause */
- n->frameOptions = $5->frameOptions;
- n->startOffset = $5->startOffset;
- n->endOffset = $5->endOffset;
+ n->frameOptions = $6->frameOptions;
+ n->startOffset = $6->startOffset;
+ n->endOffset = $6->endOffset;
+ n->rpCommonSyntax = $7;
n->location = @1;
$$ = n;
}
@@ -15892,6 +15906,31 @@ opt_partition_clause: PARTITION BY expr_list { $$ = $3; }
| /*EMPTY*/ { $$ = NIL; }
;
+/*
+ * ROW PATTERN_P MEASURES
+ */
+opt_row_pattern_measures: MEASURES row_pattern_measure_list { $$ = $2; }
+ | /*EMPTY*/ { $$ = NIL; }
+ ;
+
+row_pattern_measure_list:
+ row_pattern_measure_item
+ { $$ = list_make1($1); }
+ | row_pattern_measure_list ',' row_pattern_measure_item
+ { $$ = lappend($1, $3); }
+ ;
+
+row_pattern_measure_item:
+ a_expr AS ColLabel
+ {
+ $$ = makeNode(ResTarget);
+ $$->name = $3;
+ $$->indirection = NIL;
+ $$->val = (Node *) $1;
+ $$->location = @1;
+ }
+ ;
+
/*
* For frame clauses, we return a WindowDef, but only some fields are used:
* frameOptions, startOffset, and endOffset.
@@ -16051,6 +16090,139 @@ opt_window_exclusion_clause:
| /*EMPTY*/ { $$ = 0; }
;
+opt_row_pattern_common_syntax:
+opt_row_pattern_skip_to opt_row_pattern_initial_or_seek
+ PATTERN_P '(' row_pattern ')'
+ opt_row_pattern_subset_clause
+ DEFINE row_pattern_definition_list
+ {
+ RPCommonSyntax *n = makeNode(RPCommonSyntax);
+ n->rpSkipTo = $1->rpSkipTo;
+ n->rpSkipVariable = $1->rpSkipVariable;
+ n->initial = $2;
+ n->rpPatterns = $5;
+ n->rpSubsetClause = $7;
+ n->rpDefs = $9;
+ $$ = n;
+ }
+ | /*EMPTY*/ { $$ = NULL; }
+ ;
+
+opt_row_pattern_skip_to:
+ AFTER MATCH SKIP TO NEXT ROW
+ {
+ RPCommonSyntax *n = makeNode(RPCommonSyntax);
+ n->rpSkipTo = ST_NEXT_ROW;
+ n->rpSkipVariable = NULL;
+ $$ = n;
+ }
+ | AFTER MATCH SKIP PAST LAST_P ROW
+ {
+ RPCommonSyntax *n = makeNode(RPCommonSyntax);
+ n->rpSkipTo = ST_PAST_LAST_ROW;
+ n->rpSkipVariable = NULL;
+ $$ = n;
+ }
+/*
+ | AFTER MATCH SKIP TO FIRST_P ColId %prec FIRST_P
+ {
+ RPCommonSyntax *n = makeNode(RPCommonSyntax);
+ n->rpSkipTo = ST_FIRST_VARIABLE;
+ n->rpSkipVariable = $6;
+ $$ = n;
+ }
+ | AFTER MATCH SKIP TO LAST_P ColId %prec LAST_P
+ {
+ RPCommonSyntax *n = makeNode(RPCommonSyntax);
+ n->rpSkipTo = ST_LAST_VARIABLE;
+ n->rpSkipVariable = $6;
+ $$ = n;
+ }
+ * Shift/reduce
+ | AFTER MATCH SKIP TO ColId
+ {
+ RPCommonSyntax *n = makeNode(RPCommonSyntax);
+ n->rpSkipTo = ST_VARIABLE;
+ n->rpSkipVariable = $5;
+ $$ = n;
+ }
+*/
+ | /*EMPTY*/
+ {
+ RPCommonSyntax *n = makeNode(RPCommonSyntax);
+ /* temporary set default to ST_NEXT_ROW */
+ n->rpSkipTo = ST_PAST_LAST_ROW;
+ n->rpSkipVariable = NULL;
+ $$ = n;
+ }
+ ;
+
+opt_row_pattern_initial_or_seek:
+ INITIAL { $$ = true; }
+ | SEEK
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("SEEK is not supported"),
+ errhint("Use INITIAL."),
+ parser_errposition(@1)));
+ }
+ | /*EMPTY*/ { $$ = true; }
+ ;
+
+row_pattern:
+ row_pattern_term { $$ = list_make1($1); }
+ | row_pattern row_pattern_term { $$ = lappend($1, $2); }
+ ;
+
+row_pattern_term:
+ ColId { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "", (Node *)makeString($1), NULL, @1); }
+ | ColId '*' { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "*", (Node *)makeString($1), NULL, @1); }
+ | ColId '+' { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "+", (Node *)makeString($1), NULL, @1); }
+ | ColId '?' { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "?", (Node *)makeString($1), NULL, @1); }
+ ;
+
+opt_row_pattern_subset_clause:
+ SUBSET row_pattern_subset_list { $$ = $2; }
+ | /*EMPTY*/ { $$ = NIL; }
+ ;
+
+row_pattern_subset_list:
+ row_pattern_subset_item { $$ = list_make1($1); }
+ | row_pattern_subset_list ',' row_pattern_subset_item { $$ = lappend($1, $3); }
+ | /*EMPTY*/ { $$ = NIL; }
+ ;
+
+row_pattern_subset_item: ColId '=' '(' row_pattern_subset_rhs ')'
+ {
+ RPSubsetItem *n = makeNode(RPSubsetItem);
+ n->name = $1;
+ n->rhsVariable = $4;
+ $$ = n;
+ }
+ ;
+
+row_pattern_subset_rhs:
+ ColId { $$ = list_make1(makeStringConst($1, @1)); }
+ | row_pattern_subset_rhs ',' ColId { $$ = lappend($1, makeStringConst($3, @1)); }
+ | /*EMPTY*/ { $$ = NIL; }
+ ;
+
+row_pattern_definition_list:
+ row_pattern_definition { $$ = list_make1($1); }
+ | row_pattern_definition_list ',' row_pattern_definition { $$ = lappend($1, $3); }
+ ;
+
+row_pattern_definition:
+ ColId AS a_expr
+ {
+ $$ = makeNode(ResTarget);
+ $$->name = $1;
+ $$->indirection = NIL;
+ $$->val = (Node *) $3;
+ $$->location = @1;
+ }
+ ;
/*
* Supporting nonterminals for expressions.
@@ -17146,6 +17318,7 @@ unreserved_keyword:
| INDEXES
| INHERIT
| INHERITS
+ | INITIAL
| INLINE_P
| INPUT_P
| INSENSITIVE
@@ -17173,6 +17346,7 @@ unreserved_keyword:
| MATCHED
| MATERIALIZED
| MAXVALUE
+ | MEASURES
| MERGE
| METHOD
| MINUTE_P
@@ -17215,6 +17389,9 @@ unreserved_keyword:
| PARTITION
| PASSING
| PASSWORD
+ | PAST
+ | PATTERN_P
+ | PERMUTE
| PLANS
| POLICY
| PRECEDING
@@ -17265,6 +17442,7 @@ unreserved_keyword:
| SEARCH
| SECOND_P
| SECURITY
+ | SEEK
| SEQUENCE
| SEQUENCES
| SERIALIZABLE
@@ -17290,6 +17468,7 @@ unreserved_keyword:
| STRICT_P
| STRIP_P
| SUBSCRIPTION
+ | SUBSET
| SUPPORT
| SYSID
| SYSTEM_P
@@ -17477,6 +17656,7 @@ reserved_keyword:
| CURRENT_USER
| DEFAULT
| DEFERRABLE
+ | DEFINE
| DESC
| DISTINCT
| DO
@@ -17639,6 +17819,7 @@ bare_label_keyword:
| DEFAULTS
| DEFERRABLE
| DEFERRED
+ | DEFINE
| DEFINER
| DELETE_P
| DELIMITER
@@ -17714,6 +17895,7 @@ bare_label_keyword:
| INDEXES
| INHERIT
| INHERITS
+ | INITIAL
| INITIALLY
| INLINE_P
| INNER_P
@@ -17763,6 +17945,7 @@ bare_label_keyword:
| MATCHED
| MATERIALIZED
| MAXVALUE
+ | MEASURES
| MERGE
| METHOD
| MINVALUE
@@ -17816,6 +17999,9 @@ bare_label_keyword:
| PARTITION
| PASSING
| PASSWORD
+ | PAST
+ | PATTERN_P
+ | PERMUTE
| PLACING
| PLANS
| POLICY
@@ -17872,6 +18058,7 @@ bare_label_keyword:
| SCROLL
| SEARCH
| SECURITY
+ | SEEK
| SELECT
| SEQUENCE
| SEQUENCES
@@ -17903,6 +18090,7 @@ bare_label_keyword:
| STRICT_P
| STRIP_P
| SUBSCRIPTION
+ | SUBSET
| SUBSTRING
| SUPPORT
| SYMMETRIC
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index fef4c714b8..657651df1d 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -547,6 +547,44 @@ typedef struct SortBy
int location; /* operator location, or -1 if none/unknown */
} SortBy;
+/*
+ * AFTER MATCH row pattern skip to types in row pattern common syntax
+ */
+typedef enum RPSkipTo
+{
+ ST_NONE, /* AFTER MATCH omitted */
+ ST_NEXT_ROW, /* SKIP TO NEXT ROW */
+ ST_PAST_LAST_ROW, /* SKIP TO PAST LAST ROW */
+ ST_FIRST_VARIABLE, /* SKIP TO FIRST variable name */
+ ST_LAST_VARIABLE, /* SKIP TO LAST variable name */
+ ST_VARIABLE /* SKIP TO variable name */
+} RPSkipTo;
+
+/*
+ * Row Pattern SUBSET clause item
+ */
+typedef struct RPSubsetItem
+{
+ NodeTag type;
+ char *name; /* Row Pattern SUBSET clause variable name */
+ List *rhsVariable; /* Row Pattern SUBSET rhs variables (list of char *string) */
+} RPSubsetItem;
+
+/*
+ * RowPatternCommonSyntax - raw representation of row pattern common syntax
+ *
+ */
+typedef struct RPCommonSyntax
+{
+ NodeTag type;
+ RPSkipTo rpSkipTo; /* Row Pattern AFTER MATCH SKIP type */
+ char *rpSkipVariable; /* Row Pattern Skip To variable name, if any */
+ bool initial; /* true if <row pattern initial or seek> is initial */
+ List *rpPatterns; /* PATTERN variables (list of A_Expr) */
+ List *rpSubsetClause; /* row pattern subset clause (list of RPSubsetItem), if any */
+ List *rpDefs; /* row pattern definitions clause (list of ResTarget) */
+} RPCommonSyntax;
+
/*
* WindowDef - raw representation of WINDOW and OVER clauses
*
@@ -562,6 +600,8 @@ typedef struct WindowDef
char *refname; /* referenced window name, if any */
List *partitionClause; /* PARTITION BY expression list */
List *orderClause; /* ORDER BY (list of SortBy) */
+ List *rowPatternMeasures; /* row pattern measures (list of ResTarget) */
+ RPCommonSyntax *rpCommonSyntax; /* row pattern common syntax */
int frameOptions; /* frame_clause options, see below */
Node *startOffset; /* expression for starting bound, if any */
Node *endOffset; /* expression for ending bound, if any */
@@ -1483,6 +1523,11 @@ typedef struct GroupingSet
* the orderClause might or might not be copied (see copiedOrder); the framing
* options are never copied, per spec.
*
+ * "defineClause" is Row Pattern Recognition DEFINE clause (list of
+ * TargetEntry). TargetEntry.resname represents row pattern definition
+ * variable name. "patternVariable" and "patternRegexp" represents PATTERN
+ * clause.
+ *
* The information relevant for the query jumbling is the partition clause
* type and its bounds.
*/
@@ -1514,6 +1559,17 @@ typedef struct WindowClause
Index winref; /* ID referenced by window functions */
/* did we copy orderClause from refname? */
bool copiedOrder pg_node_attr(query_jumble_ignore);
+ /* Row Pattern AFTER MACH SKIP clause */
+ RPSkipTo rpSkipTo; /* Row Pattern Skip To type */
+ bool initial; /* true if <row pattern initial or seek> is initial */
+ /* Row Pattern DEFINE clause (list of TargetEntry) */
+ List *defineClause;
+ /* Row Pattern DEFINE variable initial names (list of String) */
+ List *defineInitial;
+ /* Row Pattern PATTERN variable name (list of String) */
+ List *patternVariable;
+ /* Row Pattern PATTERN regular expression quantifier ('+' or ''. list of String) */
+ List *patternRegexp;
} WindowClause;
/*
diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h
index 5984dcfa4b..2804333b53 100644
--- a/src/include/parser/kwlist.h
+++ b/src/include/parser/kwlist.h
@@ -128,6 +128,7 @@ PG_KEYWORD("default", DEFAULT, RESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("defaults", DEFAULTS, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("deferrable", DEFERRABLE, RESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("deferred", DEFERRED, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("define", DEFINE, RESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("definer", DEFINER, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("delete", DELETE_P, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("delimiter", DELIMITER, UNRESERVED_KEYWORD, BARE_LABEL)
@@ -212,6 +213,7 @@ PG_KEYWORD("index", INDEX, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("indexes", INDEXES, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("inherit", INHERIT, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("inherits", INHERITS, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("initial", INITIAL, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("initially", INITIALLY, RESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("inline", INLINE_P, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("inner", INNER_P, TYPE_FUNC_NAME_KEYWORD, BARE_LABEL)
@@ -265,6 +267,7 @@ PG_KEYWORD("match", MATCH, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("matched", MATCHED, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("materialized", MATERIALIZED, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("maxvalue", MAXVALUE, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("measures", MEASURES, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("merge", MERGE, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("method", METHOD, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("minute", MINUTE_P, UNRESERVED_KEYWORD, AS_LABEL)
@@ -326,6 +329,9 @@ PG_KEYWORD("partial", PARTIAL, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("partition", PARTITION, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("passing", PASSING, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("password", PASSWORD, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("past", PAST, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("pattern", PATTERN_P, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("permute", PERMUTE, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("placing", PLACING, RESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("plans", PLANS, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("policy", POLICY, UNRESERVED_KEYWORD, BARE_LABEL)
@@ -385,6 +391,7 @@ PG_KEYWORD("scroll", SCROLL, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("search", SEARCH, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("second", SECOND_P, UNRESERVED_KEYWORD, AS_LABEL)
PG_KEYWORD("security", SECURITY, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("seek", SEEK, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("select", SELECT, RESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("sequence", SEQUENCE, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("sequences", SEQUENCES, UNRESERVED_KEYWORD, BARE_LABEL)
@@ -416,6 +423,7 @@ PG_KEYWORD("stored", STORED, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("strict", STRICT_P, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("strip", STRIP_P, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("subscription", SUBSCRIPTION, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("subset", SUBSET, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("substring", SUBSTRING, COL_NAME_KEYWORD, BARE_LABEL)
PG_KEYWORD("support", SUPPORT, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("symmetric", SYMMETRIC, RESERVED_KEYWORD, BARE_LABEL)
diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h
index f589112d5e..6640090910 100644
--- a/src/include/parser/parse_node.h
+++ b/src/include/parser/parse_node.h
@@ -51,6 +51,7 @@ typedef enum ParseExprKind
EXPR_KIND_WINDOW_FRAME_RANGE, /* window frame clause with RANGE */
EXPR_KIND_WINDOW_FRAME_ROWS, /* window frame clause with ROWS */
EXPR_KIND_WINDOW_FRAME_GROUPS, /* window frame clause with GROUPS */
+ EXPR_KIND_RPR_DEFINE, /* DEFINE */
EXPR_KIND_SELECT_TARGET, /* SELECT target list item */
EXPR_KIND_INSERT_TARGET, /* INSERT target list item */
EXPR_KIND_UPDATE_SOURCE, /* UPDATE assignment source item */
--
2.25.1
----Next_Part(Tue_Sep_12_15_18_43_2023_359)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v6-0002-Row-pattern-recognition-patch-parse-analysis.patch"
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: pg_basebackup and error messages dependent on the order of the arguments
@ 2024-10-02 10:00 Daniel Westermann (DWE) <[email protected]>
0 siblings, 1 reply; 47+ messages in thread
From: Daniel Westermann (DWE) @ 2024-10-02 10:00 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: pgsql-hackers
>> My point was not so much about --compress but rather giving a good error message.
>Right, and my point was that the issue is bigger than --compress.
>For example, you get exactly the same misbehavior with
>$ pg_basebackup --checkpoint=fast --format=t -d --pgdata=/var/tmp/dummy
>pg_basebackup: error: must specify output directory or backup target
>pg_basebackup: hint: Try "pg_basebackup --help" for more information.
>I'm not sure how to solve the problem once you consider this larger
>scope. I don't think we could forbid arguments beginning with "-" for
>all of these switches.
It is not dependent on short or long switches:
$ pg_basebackup --checkpoint=fast --format=t -p --pgdata=/var/tmp/dummy
pg_basebackup: error: must specify output directory or backup target
pg_basebackup: hint: Try "pg_basebackup --help" for more information.
$ pg_basebackup --checkpoint=fast --format=t --port --pgdata=/var/tmp/dummy
pg_basebackup: error: must specify output directory or backup target
pg_basebackup: hint: Try "pg_basebackup --help" for more information.
Maybe checking if a valid "-D" or "--pgdata" was given and return a more generic error message would be an option?
Regards
Daniel
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: pg_basebackup and error messages dependent on the order of the arguments
@ 2024-10-03 19:19 Robert Haas <[email protected]>
parent: Daniel Westermann (DWE) <[email protected]>
0 siblings, 1 reply; 47+ messages in thread
From: Robert Haas @ 2024-10-03 19:19 UTC (permalink / raw)
To: Daniel Westermann (DWE) <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers
On Wed, Oct 2, 2024 at 6:00 AM Daniel Westermann (DWE)
<[email protected]> wrote:
> Maybe checking if a valid "-D" or "--pgdata" was given and return a more generic error message would be an option?
It doesn't really seem reasonable to me to make the tools guess
whether somebody left out the argument to an option that requires an
argument. Consider these equivalent cases:
$ pg_dump -t -Ft
pg_dump: error: no matching tables were found
$ initdb -c --text-search-config=english -D x
<lots of output>
running bootstrap script ... 2024-10-03 18:56:51.372 GMT [16909] LOG:
syntax error in file "/Users/robert.haas/pgsql/x/postgresql.conf" line
843, near token "-"
2024-10-03 18:56:51.372 GMT [16909] FATAL: configuration file
"/Users/robert.haas/pgsql/x/postgresql.conf" contains errors
child process exited with exit code 1
initdb: removing data directory "x"
$ dropuser -h -e bob
dropuser: error: could not translate host name "-e" to address:
nodename nor servname provided, or not known
I assume there are similar cases that don't involve PostgreSQL at all.
I think that this kind of problem is basically normal and the code is
working as designed. It's true that in some cases we could maybe make
an intelligent guess that the user has messed up, but that's always
got to be based on knowing that something that is formally an option
to an argument looks more like a separate argument. And I'm just not
very enthusiastic about inserting heuristics like that all over the
place. It seems like a lot of work and hard to get right, and it could
easily end up being more annoying than useful, if say we accidentally
reject something that was truly intended to be an argument to the
switch rather than a second argument.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: pg_basebackup and error messages dependent on the order of the arguments
@ 2024-10-03 19:32 Tom Lane <[email protected]>
parent: Robert Haas <[email protected]>
0 siblings, 0 replies; 47+ messages in thread
From: Tom Lane @ 2024-10-03 19:32 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Daniel Westermann (DWE) <[email protected]>; pgsql-hackers
Robert Haas <[email protected]> writes:
> On Wed, Oct 2, 2024 at 6:00 AM Daniel Westermann (DWE)
> <[email protected]> wrote:
>> Maybe checking if a valid "-D" or "--pgdata" was given and return a more generic error message would be an option?
> It doesn't really seem reasonable to me to make the tools guess
> whether somebody left out the argument to an option that requires an
> argument. Consider these equivalent cases:
> ...
> I assume there are similar cases that don't involve PostgreSQL at all.
Yeah. This has to be a standard problem for anything that uses getopt
or getopt_long at all. Unless there's a standard approach (which I've
not heard of) to resolving these ambiguities, I'm not sure that we
should try to outsmart everybody else.
In the case of getopt_long there's an additional problem, which is
that that function itself may contain heuristics that rearrange the
argument order based on what looks like a switch or not. It's likely
that anything we did on top of that would behave differently depending
on which version of getopt_long it is.
regards, tom lane
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Introduce new multi insert Table AM and improve performance of various SQL commands with it for Heap AM
@ 2025-03-09 11:27 Daniil Davydov <[email protected]>
0 siblings, 1 reply; 47+ messages in thread
From: Daniil Davydov @ 2025-03-09 11:27 UTC (permalink / raw)
To: Jingtang Zhang <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; [email protected]; pgsql-hackers
Hi,
A few days ago I came up with an idea to implement multi insert
optimization wherever possible. I prepared a raw patch
and it showed a great performance gain (up to 4 times during INSERT
... INTO ... in the best case).
Then I was very happy to find this thread. You did a great job and I
want to help you to bring the matter to an end.
On Thu, Oct 31, 2024 at 11:17 AM Jingtang Zhang <[email protected]> wrote:
> I did some performance test these days, and I have some findings.
> HEAD:
> 12.29% postgres [.] pg_checksum_block
> 6.33% postgres [.] GetPrivateRefCountEntry
> 5.40% postgres [.] pg_comp_crc32c_sse42
> 4.54% [kernel] [k] copy_user_enhanced_fast_string
> 2.69% postgres [.] BufferIsValid
> 1.52% postgres [.] XLogRecordAssemble
>
> Patched:
> 11.75% postgres [.] tts_virtual_materialize
> 8.87% postgres [.] pg_checksum_block
> 8.17% postgres [.] slot_deform_heap_tuple
> 8.09% postgres [.] heap_compute_data_size
> 6.17% postgres [.] fill_val
> 3.81% postgres [.] heap_fill_tuple
> 3.37% postgres [.] tts_virtual_copyslot
> 2.62% [kernel] [k] copy_user_enhanced_fast_string
I applied v25 patches on the master branch and made some measurements
to find out what is the bottleneck in this case. The 'time' utility
showed that without a patch, this query will run 1.5 times slower. I
also made a few flamegraphs for this test. Most of the time is spent
calling
these two functions : tts_virtual_copyslot and heap_form_tuple.
All tests were run in virtual machine with these CPU characteristics:
Architecture: x86_64
CPU(s): 2
On-line CPU(s) list: 0,1
Virtualization features:
Virtualization: AMD-V
Hypervisor vendor: KVM
Virtualization type: full
Caches (sum of all):
L1d: 128 KiB (2 instances)
L1i: 128 KiB (2 instances)
L2: 1 MiB (2 instances)
L3: 32 MiB (2 instances)
NUMA:
NUMA node(s): 1
NUMA node0 CPU(s): 0,1
In my implementation, I used Tuplestore functionality to store tuples.
In order to get rid of getting stuck in the above mentioned functions,
I crossed it with the current implementation (v25 patches) and got a
10% increase in performance (for the test above). I also set up v22
patches to
compare performance (with/without tuplestore) for INSERT ... INTO ...
queries (with -j 4 -c 10 parameters for pgbech), and there also was an
increase in TPS (about 3-4%).
I attach a patch that adds Tuplestore to v25. What do you think about this idea?
--
Best regards,
Daniil Davydov
Attachments:
[text/x-patch] 0001-Replace-holding-tuples-in-virtual-slots-with-tuplest.patch (5.9K, ../../CAJDiXggcx+v7eKruvvBK-mpyf3Y3e8vgBJhcZwhkm4p6907edw@mail.gmail.com/2-0001-Replace-holding-tuples-in-virtual-slots-with-tuplest.patch)
download | inline diff:
From a59cfcbb05bb07c94a4c0ad6531baa5e531629ae Mon Sep 17 00:00:00 2001
From: Daniil Davidov <[email protected]>
Date: Sun, 9 Mar 2025 16:37:44 +0700
Subject: [PATCH] Replace holding tuples in virtual slots with tuplestorage
During performance testing, it was found out that in the current
implementation a lot of the program's time is spent calling two functions :
tts_virtual_copyslot and heap_fill_tuple. Calls to these functions are related
to the fact that tuples are stored in virtual_tts, so I propose to replace this
logic with Tuplestore functionality.
Discussion: https://www.postgresql.org/message-id/9F9326B4-8AD9-4858-B1C1-559FC64E6E93%40gmail.com
---
src/backend/access/heap/heapam.c | 67 +++++++++++++++-----------------
src/include/access/heapam.h | 9 ++++-
2 files changed, 38 insertions(+), 38 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index acdce1a4b4..276480213a 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -2665,7 +2665,6 @@ void
heap_modify_buffer_insert(TableModifyState *state,
TupleTableSlot *slot)
{
- TupleTableSlot *dstslot;
HeapInsertState *istate;
HeapMultiInsertState *mistate;
MemoryContext oldcontext;
@@ -2682,8 +2681,10 @@ heap_modify_buffer_insert(TableModifyState *state,
mistate =
(HeapMultiInsertState *) palloc(sizeof(HeapMultiInsertState));
mistate->slots =
- (TupleTableSlot **) palloc0(sizeof(TupleTableSlot *) * HEAP_MAX_BUFFERED_SLOTS);
- mistate->cur_slots = 0;
+ (TupleTableSlot **) palloc0(sizeof(void *) * HEAP_MAX_BUFFERED_SLOTS);
+ mistate->tstore = tuplestore_begin_heap(false, false, work_mem);
+ mistate->nused = 0;
+
istate->mistate = mistate;
/*
@@ -2702,36 +2703,11 @@ heap_modify_buffer_insert(TableModifyState *state,
istate = (HeapInsertState *) state->data;
Assert(istate->mistate != NULL);
mistate = istate->mistate;
- dstslot = mistate->slots[mistate->cur_slots];
-
- if (dstslot == NULL)
- {
- /*
- * We use virtual tuple slots buffered slots for leveraging the
- * optimization it provides to minimize physical data copying. The
- * virtual slot gets materialized when we copy (via below
- * ExecCopySlot) the tuples from the source slot which can be of any
- * type. This way, it is ensured that the tuple storage doesn't depend
- * on external memory, because all the datums that aren't passed by
- * value are copied into the slot's memory context.
- */
- dstslot = MakeTupleTableSlot(RelationGetDescr(state->rel),
- &TTSOpsVirtual);
-
- mistate->slots[mistate->cur_slots] = dstslot;
- }
-
- Assert(TTS_IS_VIRTUAL(dstslot));
-
- /*
- * Note that the copy clears the previous destination slot contents, so no
- * need to explicitly ExecClearTuple() here.
- */
- ExecCopySlot(dstslot, slot);
- mistate->cur_slots++;
+ tuplestore_puttupleslot(mistate->tstore, slot);
+ mistate->nused += 1;
- if (mistate->cur_slots >= HEAP_MAX_BUFFERED_SLOTS)
+ if (mistate->nused >= HEAP_MAX_BUFFERED_SLOTS)
heap_modify_buffer_flush(state);
MemoryContextSwitchTo(oldcontext);
@@ -2746,19 +2722,35 @@ heap_modify_buffer_flush(TableModifyState *state)
HeapInsertState *istate;
HeapMultiInsertState *mistate;
MemoryContext oldcontext;
+ TupleDesc tupdesc;
/* Quick exit if we haven't inserted anything yet */
if (state->data == NULL)
return;
+ tupdesc = RelationGetDescr(state->rel);
istate = (HeapInsertState *) state->data;
Assert(istate->mistate != NULL);
mistate = istate->mistate;
/* Quick exit if we have flushed already */
- if (mistate->cur_slots == 0)
+ if (mistate->nused == 0)
return;
+ for (int i = 0; i < mistate->nused; i++)
+ {
+ bool ok;
+
+ if (istate->mistate->slots[i] == NULL)
+ {
+ istate->mistate->slots[i] =
+ MakeSingleTupleTableSlot(tupdesc, &TTSOpsMinimalTuple);
+ }
+ ok = tuplestore_gettupleslot(mistate->tstore, true, false,
+ istate->mistate->slots[i]);
+ Assert(ok);
+ }
+
/*
* heap_multi_insert() can leak memory, so switch to short-lived memory
* context before calling it.
@@ -2766,7 +2758,7 @@ heap_modify_buffer_flush(TableModifyState *state)
oldcontext = MemoryContextSwitchTo(mistate->mem_ctx);
heap_multi_insert(state->rel,
mistate->slots,
- mistate->cur_slots,
+ mistate->nused,
state->cid,
state->options,
istate->bistate);
@@ -2779,14 +2771,15 @@ heap_modify_buffer_flush(TableModifyState *state)
*/
if (state->buffer_flush_cb != NULL)
{
- for (int i = 0; i < mistate->cur_slots; i++)
+ for (int i = 0; i < mistate->nused; i++)
{
state->buffer_flush_cb(state->buffer_flush_ctx,
mistate->slots[i]);
}
}
- mistate->cur_slots = 0;
+ tuplestore_clear(mistate->tstore);
+ mistate->nused = 0;
}
/*
@@ -2811,11 +2804,13 @@ heap_modify_insert_end(TableModifyState *state)
heap_modify_buffer_flush(state);
- Assert(mistate->cur_slots == 0);
+ Assert(mistate->nused== 0);
for (int i = 0; i < HEAP_MAX_BUFFERED_SLOTS && mistate->slots[i] != NULL; i++)
ExecDropSingleTupleTableSlot(mistate->slots[i]);
+ tuplestore_end(mistate->tstore);
+
MemoryContextDelete(mistate->mem_ctx);
}
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index fdbbf9b8e8..5d8e672059 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -27,8 +27,10 @@
#include "storage/lockdefs.h"
#include "storage/read_stream.h"
#include "storage/shm_toc.h"
+#include "tcop/dest.h"
#include "utils/relcache.h"
#include "utils/snapshot.h"
+#include "utils/tuplestore.h"
/* "options" flag bits for heap_insert */
@@ -285,8 +287,11 @@ typedef struct HeapMultiInsertState
/* Array of buffered slots */
TupleTableSlot **slots;
- /* Number of buffered slots currently held */
- int cur_slots;
+ /* Holds the tuple set */
+ Tuplestorestate *tstore;
+
+ /* Number of buffered tuples currently held */
+ int nused;
/* Memory context for dealing with multi inserts */
MemoryContext mem_ctx;
--
2.43.0
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Introduce new multi insert Table AM and improve performance of various SQL commands with it for Heap AM
@ 2025-03-17 04:49 Daniil Davydov <[email protected]>
parent: Daniil Davydov <[email protected]>
0 siblings, 1 reply; 47+ messages in thread
From: Daniil Davydov @ 2025-03-17 04:49 UTC (permalink / raw)
To: Jingtang Zhang <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; [email protected]; pgsql-hackers
Hi,
Recently I took more careful measurements of the performance. I
compared three branches with each other: HEAD, Patched and Patched
with tuplestore.
Here are the results :
1)
Test case : matview creation test attached in the email from Jingtang Zhang.
10 measurements for each branch.
Result in wall clock execution time :
HEAD
30.532 +- 0.59 seconds elapsed
Patched
20.454 +- 0.114 seconds elapsed
Patched with tuplestore
19.653 +- 0.111 seconds elapsed
2)
-- init.sql
drop table test_insert;
vacuum;
checkpoint;
create table test_insert(i int, f float);
-- iowrite.sql
insert into test_insert select g, (g % 100) / 100.0 from
generate_series(1, 1000000) as g;
Test case :
pgbench -f iowrite.sql -n -j 4 -c 10 -T 40
5 measurements for each branch.
Result in tps :
HEAD
1.025 +- 0.009
Patched
2.923 +- 0.032
Patched with tuplestore
2.987 +- 0.011
P.S.
I cannot find a commitfest entry for this patch. Should we add it there?
--
Best regards,
Daniil Davydov
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Introduce new multi insert Table AM and improve performance of various SQL commands with it for Heap AM
@ 2025-04-07 06:26 Daniil Davydov <[email protected]>
parent: Daniil Davydov <[email protected]>
0 siblings, 1 reply; 47+ messages in thread
From: Daniil Davydov @ 2025-04-07 06:26 UTC (permalink / raw)
To: Jingtang Zhang <[email protected]>; +Cc: [email protected]; pgsql-hackers; Bharath Rupireddy <[email protected]>
Hi,
On Sun, Apr 6, 2025 at 8:55 PM Jingtang Zhang <[email protected]> wrote:
>
> It was quite a while since I last looked at the patch. I've tested it again,
> and still get regression on patched version where a table has many columns.
> And it is totally CPU-bounded on tts_virtual_copyslot.
>
> Unpatched version:
> 1 col:
> Time: 8909.714 ms (00:08.910)
> Time: 8803.579 ms (00:08.804)
> Time: 8600.415 ms (00:08.600)
> 32 cols:
> Time: 12911.699 ms (00:12.912)
> Time: 13543.491 ms (00:13.543)
> Time: 13325.368 ms (00:13.325)
>
> Patched version:
> 1 col:
> Time: 3532.841 ms (00:03.533)
> Time: 3598.223 ms (00:03.598)
> Time: 3515.858 ms (00:03.516)
> 32 cols:
> Time: 35647.724 ms (00:35.648)
> Time: 35596.233 ms (00:35.596)
> Time: 35669.106 ms (00:35.669)
>
Hm, maybe I didn't choose the best way to measure performance. Can you
please share how you do it?
> I've tested your patch with tuplestore and found the regression does not exist
> anymore, but I haven't look deep inside it.
>
> Patched version (with tuplestore):
> 1 col:
> Time: 3500.502 ms (00:03.501)
> Time: 3486.886 ms (00:03.487)
> Time: 3514.233 ms (00:03.514)
> 32 cols:
> Time: 10375.391 ms (00:10.375)
> Time: 10248.256 ms (00:10.248)
> Time: 10248.289 ms (00:10.248)
>
> It seems to be a good idea if there is no other issue with your patch.
As far as I understand, the use of multi inserts for queries like
"INSERT INTO ... SELECT FROM" is not discussed here anymore due to the
fact that in such cases we will have to take into account the volatile
functions and ROW triggers.
I've been thinking about this for a while and made a patch as an
experiment. The principles that the patch works on are listed below.
1)
Since performance decreases for single INSERTs (within a multi inserts
mechanism), I designed this feature as an option for the table. Thus,
if the user knows that he will perform a lot of inserts on the table,
he can specify "WITH (append_optimized=true)".
2)
The availability of volatile functions is monitored during the
construction of a subtree for a ModifyTable node. I'm not that
familiar with the query plan construction mechanism, but it seems to
me that this way we can track any occurrence of volatile functions.
Of course, most volatile functions don't stop us from using multi
inserts, but checking each such function would take a very long time,
so the very fact of having a volatile function is enough for us to
abandon multi-inserts.
3)
Default expressions of the target table are also checked for volatile
functions. The same rules apply to them as in (2). As an exception, I
allowed the use of SERIAL in the column data type, since this is a
fairly common use case.
4)
If the target table contains any ROW triggers, we don't use multi insert.
5)
Patch also contains a regression test. This is a "sandbox" where you
can do some experiments with append-optimized tables.
I hope that patch (targeted on 'master' branch,
2c7bd2ba507e273f2d7fe1b2f6d30775ed4f3c09) will be useful for this
thread.
--
Best regards,
Daniil Davydov
Attachments:
[text/x-patch] v1-0001-Meet-append-optimized-tables.patch (42.0K, ../../CAJDiXgiD4G0rBwWk6=0TTy9AYHi4LmPL6AS5VpMLf8OEbAxEhw@mail.gmail.com/2-v1-0001-Meet-append-optimized-tables.patch)
download | inline diff:
From 224378c11d270aabe28bdd32efacd37ed1984bd1 Mon Sep 17 00:00:00 2001
From: Daniil Davidov <[email protected]>
Date: Mon, 7 Apr 2025 12:55:50 +0700
Subject: [PATCH v1] Meet append optimized tables
---
src/backend/access/common/reloptions.c | 11 +
src/backend/access/heap/heapam.c | 205 ++++++++++++++++++
src/backend/access/heap/heapam_handler.c | 5 +
src/backend/access/table/tableamapi.c | 5 +
src/backend/commands/explain.c | 5 +-
src/backend/executor/execExpr.c | 17 +-
src/backend/executor/execProcnode.c | 9 +
src/backend/executor/nodeModifyTable.c | 194 ++++++++++++++++-
src/backend/optimizer/plan/createplan.c | 1 +
src/backend/optimizer/util/clauses.c | 28 ++-
src/include/access/heapam.h | 41 ++++
src/include/access/tableam.h | 84 +++++++
src/include/nodes/execnodes.h | 6 +
src/include/nodes/plannodes.h | 2 +
src/include/optimizer/optimizer.h | 3 +
src/include/utils/rel.h | 10 +
.../regress/expected/append_optimized.out | 161 ++++++++++++++
src/test/regress/parallel_schedule | 2 +
src/test/regress/sql/append_optimized.sql | 105 +++++++++
19 files changed, 879 insertions(+), 15 deletions(-)
create mode 100644 src/test/regress/expected/append_optimized.out
create mode 100644 src/test/regress/sql/append_optimized.sql
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 46c1dce222d..9652cf4179b 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -166,6 +166,15 @@ static relopt_bool boolRelOpts[] =
},
true
},
+ {
+ {
+ "append_optimized",
+ "Enables using batching for insertion algorithm whenever it possible",
+ RELOPT_KIND_HEAP,
+ AccessExclusiveLock
+ },
+ false
+ },
/* list terminator */
{{NULL}}
};
@@ -1905,6 +1914,8 @@ default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
offsetof(StdRdOptions, vacuum_index_cleanup)},
{"vacuum_truncate", RELOPT_TYPE_BOOL,
offsetof(StdRdOptions, vacuum_truncate), offsetof(StdRdOptions, vacuum_truncate_set)},
+ {"append_optimized", RELOPT_TYPE_BOOL,
+ offsetof(StdRdOptions, append_optimized)},
{"vacuum_max_eager_freeze_failure_rate", RELOPT_TYPE_REAL,
offsetof(StdRdOptions, vacuum_max_eager_freeze_failure_rate)}
};
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index ed2e3021799..415eef4c35d 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -51,6 +51,7 @@
#include "utils/datum.h"
#include "utils/injection_point.h"
#include "utils/inval.h"
+#include "utils/memutils.h"
#include "utils/spccache.h"
#include "utils/syscache.h"
@@ -106,6 +107,7 @@ static XLogRecPtr log_heap_new_cid(Relation relation, HeapTuple tup);
static HeapTuple ExtractReplicaIdentity(Relation relation, HeapTuple tp, bool key_required,
bool *copy);
+static void heap_modify_insert_end(TableModifyState *state);
/*
* Each tuple lock mode has a corresponding heavyweight lock, and one or two
@@ -2674,6 +2676,209 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
pgstat_count_heap_insert(relation, ntuples);
}
+/*
+ * Initialize heap modify state.
+ */
+TableModifyState *
+heap_modify_begin(Relation rel,
+ CommandId cid,
+ int options,
+ TableModifyBufferFlushCb buffer_flush_cb,
+ void *buffer_flush_ctx)
+{
+ TableModifyState *state;
+ MemoryContext context;
+ MemoryContext oldcontext;
+
+ Assert(RelationIsAppendOptimized(rel));
+ context = AllocSetContextCreate(TopTransactionContext,
+ "heap_modify memory context",
+ ALLOCSET_DEFAULT_SIZES);
+
+ oldcontext = MemoryContextSwitchTo(context);
+ state = palloc(sizeof(TableModifyState));
+ state->rel = rel;
+ state->cid = cid;
+ state->options = options;
+ state->mem_ctx = context;
+ state->buffer_flush_cb = buffer_flush_cb;
+ state->buffer_flush_ctx = buffer_flush_ctx;
+ state->data = NULL; /* To be set lazily */
+ MemoryContextSwitchTo(oldcontext);
+
+ return state;
+}
+
+/*
+ * Store passed-in tuple into in-memory buffered slots. When full, insert
+ * multiple tuples from the buffers into heap.
+ */
+void
+heap_modify_buffer_insert(TableModifyState *state,
+ TupleTableSlot *slot)
+{
+ HeapInsertState *istate;
+ HeapMultiInsertState *mistate;
+ MemoryContext oldcontext;
+
+ Assert(RelationIsAppendOptimized(state->rel));
+ oldcontext = MemoryContextSwitchTo(state->mem_ctx);
+
+ /* First time through, initialize heap insert state */
+ if (state->data == NULL)
+ {
+ istate = (HeapInsertState *) palloc(sizeof(HeapInsertState));
+ istate->bistate = NULL;
+ istate->mistate = NULL;
+ state->data = istate;
+ mistate =
+ (HeapMultiInsertState *) palloc(sizeof(HeapMultiInsertState));
+ mistate->slots =
+ (TupleTableSlot **) palloc0(sizeof(void *) * HEAP_MAX_BUFFERED_SLOTS);
+ mistate->tstore = tuplestore_begin_heap(false, false, work_mem);
+ mistate->nused = 0;
+ istate->mistate = mistate;
+
+ /*
+ * heap_multi_insert() can leak memory. So switch to this memory
+ * context before every heap_multi_insert() call and reset when
+ * finished.
+ */
+ mistate->mem_ctx = AllocSetContextCreate(CurrentMemoryContext,
+ "heap_multi_insert memory context",
+ ALLOCSET_DEFAULT_SIZES);
+ istate->bistate = GetBulkInsertState();
+ }
+
+ istate = (HeapInsertState *) state->data;
+ Assert(istate->mistate != NULL);
+ mistate = istate->mistate;
+
+ tuplestore_puttupleslot(mistate->tstore, slot);
+ mistate->nused += 1;
+
+ if (mistate->nused >= HEAP_MAX_BUFFERED_SLOTS)
+ heap_modify_buffer_flush(state);
+
+ MemoryContextSwitchTo(oldcontext);
+}
+
+/*
+ * Insert multiple tuples from in-memory buffered slots into heap.
+ */
+void
+heap_modify_buffer_flush(TableModifyState *state)
+{
+ HeapInsertState *istate;
+ HeapMultiInsertState *mistate;
+ MemoryContext oldcontext;
+ TupleDesc tupdesc;
+
+ Assert(RelationIsAppendOptimized(state->rel));
+
+ /* Quick exit if we haven't inserted anything yet */
+ if (state->data == NULL)
+ return;
+
+ tupdesc = RelationGetDescr(state->rel);
+ istate = (HeapInsertState *) state->data;
+ Assert(istate->mistate != NULL);
+ mistate = istate->mistate;
+
+ /* Quick exit if we have flushed already */
+ if (mistate->nused == 0)
+ return;
+
+ for (int i = 0; i < mistate->nused; i++)
+ {
+ bool ok;
+
+ if (istate->mistate->slots[i] == NULL)
+ {
+ istate->mistate->slots[i] =
+ MakeSingleTupleTableSlot(tupdesc, &TTSOpsMinimalTuple);
+ }
+ ok = tuplestore_gettupleslot(mistate->tstore, true, false,
+ istate->mistate->slots[i]);
+ Assert(ok);
+ }
+
+ /*
+ * heap_multi_insert() can leak memory, so switch to short-lived memory
+ * context before calling it.
+ */
+ oldcontext = MemoryContextSwitchTo(mistate->mem_ctx);
+ heap_multi_insert(state->rel,
+ mistate->slots,
+ mistate->nused,
+ state->cid,
+ state->options,
+ istate->bistate);
+ MemoryContextSwitchTo(oldcontext);
+ MemoryContextReset(mistate->mem_ctx);
+
+ /*
+ * Invoke caller-supplied buffer flush callback after inserting rows from
+ * the buffers to heap.
+ */
+ if (state->buffer_flush_cb != NULL)
+ {
+ for (int i = 0; i < mistate->nused; i++)
+ {
+ state->buffer_flush_cb(state->buffer_flush_ctx,
+ mistate->slots[i]);
+ }
+ }
+
+ tuplestore_clear(mistate->tstore);
+ mistate->nused = 0;
+}
+
+/*
+ * Heap insert specific function used for performing work at the end like
+ * flushing remaining buffered tuples, cleaning up the insert state and tuple
+ * table slots used for buffered tuples etc.
+ */
+static void
+heap_modify_insert_end(TableModifyState *state)
+{
+ HeapInsertState *istate;
+
+ /* Quick exit if we haven't inserted anything yet */
+ if (state->data == NULL)
+ return;
+
+ istate = (HeapInsertState *) state->data;
+
+ if (istate->mistate != NULL)
+ {
+ HeapMultiInsertState *mistate = istate->mistate;
+
+ heap_modify_buffer_flush(state);
+
+ Assert(mistate->nused == 0);
+
+ for (int i = 0; i < HEAP_MAX_BUFFERED_SLOTS && mistate->slots[i] != NULL; i++)
+ ExecDropSingleTupleTableSlot(mistate->slots[i]);
+
+ tuplestore_end(mistate->tstore);
+ MemoryContextDelete(mistate->mem_ctx);
+ }
+
+ if (istate->bistate != NULL)
+ FreeBulkInsertState(istate->bistate);
+}
+
+/*
+ * Clean heap modify state.
+ */
+void
+heap_modify_end(TableModifyState *state)
+{
+ heap_modify_insert_end(state);
+ MemoryContextDelete(state->mem_ctx);
+}
+
/*
* simple_heap_insert - insert a tuple
*
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index ac082fefa77..56880165ed0 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2643,6 +2643,11 @@ static const TableAmRoutine heapam_methods = {
.tuple_update = heapam_tuple_update,
.tuple_lock = heapam_tuple_lock,
+ .tuple_modify_begin = heap_modify_begin,
+ .tuple_modify_buffer_insert = heap_modify_buffer_insert,
+ .tuple_modify_buffer_flush = heap_modify_buffer_flush,
+ .tuple_modify_end = heap_modify_end,
+
.tuple_fetch_row_version = heapam_fetch_row_version,
.tuple_get_latest_tid = heap_get_latest_tid,
.tuple_tid_valid = heapam_tuple_tid_valid,
diff --git a/src/backend/access/table/tableamapi.c b/src/backend/access/table/tableamapi.c
index 476663b66aa..ae30c5a21a8 100644
--- a/src/backend/access/table/tableamapi.c
+++ b/src/backend/access/table/tableamapi.c
@@ -94,6 +94,11 @@ GetTableAmRoutine(Oid amhandler)
Assert(routine->scan_sample_next_block != NULL);
Assert(routine->scan_sample_next_tuple != NULL);
+ Assert(routine->tuple_modify_begin != NULL);
+ Assert(routine->tuple_modify_buffer_insert != NULL);
+ Assert(routine->tuple_modify_buffer_flush != NULL);
+ Assert(routine->tuple_modify_end != NULL);
+
return routine;
}
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index ef8aa489af8..31ce1fa7acb 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -1399,7 +1399,10 @@ ExplainNode(PlanState *planstate, List *ancestors,
switch (((ModifyTable *) plan)->operation)
{
case CMD_INSERT:
- pname = operation = "Insert";
+ if (((ModifyTable *) plan)->canUseBatching)
+ pname = operation = "MultiInsert";
+ else
+ pname = operation = "Insert";
break;
case CMD_UPDATE:
pname = operation = "Update";
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index f1569879b52..f2d3a236fbc 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -103,7 +103,11 @@ static void ExecInitJsonCoercion(ExprState *state, JsonReturning *returning,
ErrorSaveContext *escontext, bool omit_quotes,
bool exists_coerce,
Datum *resv, bool *resnull);
-
+/*
+ * Every time when we find volatile function during expresstion evaluating, we
+ * must set this flag, so higher level code can process it appropriately.
+ */
+static bool volatile_func_flag = false;
/*
* ExecInitExpr: prepare an expression tree for execution
@@ -264,6 +268,9 @@ ExecInitQual(List *qual, PlanState *parent)
scratch.resvalue = &state->resvalue;
scratch.resnull = &state->resnull;
+ /* Reset flag indicating the presence of volatile functions in qual */
+ volatile_func_flag = false;
+
foreach_ptr(Expr, node, qual)
{
/* first evaluate expression */
@@ -276,6 +283,10 @@ ExecInitQual(List *qual, PlanState *parent)
state->steps_len - 1);
}
+ /* Possibly update information about batch-insert-capability */
+ if (parent && !parent->has_volatile)
+ parent->has_volatile = volatile_func_flag;
+
/* adjust jump targets */
foreach_int(jump, adjust_jumps)
{
@@ -1193,6 +1204,10 @@ ExecInitExprRec(Expr *node, ExprState *state,
{
FuncExpr *func = (FuncExpr *) node;
+ /* Higher level code will handle it */
+ if (func_volatile(func->funcid))
+ volatile_func_flag = true;
+
ExecInitFunc(&scratch, node,
func->args, func->funcid, func->inputcollid,
state);
diff --git a/src/backend/executor/execProcnode.c b/src/backend/executor/execProcnode.c
index f5f9cfbeead..2383ef7ea4b 100644
--- a/src/backend/executor/execProcnode.c
+++ b/src/backend/executor/execProcnode.c
@@ -416,6 +416,15 @@ ExecInitNode(Plan *node, EState *estate, int eflags)
result->instrument = InstrAlloc(1, estate->es_instrument,
result->async_capable);
+ /* Check whether some nodes below has volatile functions */
+ if ((outerPlanState(result) != NULL &&
+ outerPlanState(result)->has_volatile) ||
+ (innerPlanState(result) != NULL &&
+ innerPlanState(result)->has_volatile))
+ {
+ result->has_volatile = true;
+ }
+
return result;
}
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 309e27f8b5f..bbaf91bcbac 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -55,6 +55,7 @@
#include "access/htup_details.h"
#include "access/tableam.h"
#include "access/xact.h"
+#include "catalog/pg_proc.h"
#include "commands/trigger.h"
#include "executor/execPartition.h"
#include "executor/executor.h"
@@ -67,6 +68,8 @@
#include "storage/lmgr.h"
#include "utils/builtins.h"
#include "utils/datum.h"
+#include "utils/fmgroids.h"
+#include "utils/lsyscache.h"
#include "utils/rel.h"
#include "utils/snapmgr.h"
@@ -130,6 +133,18 @@ typedef struct UpdateContext
LockTupleMode lockmode;
} UpdateContext;
+typedef struct InsertModifyBufferFlushContext
+{
+ ResultRelInfo *resultRelInfo;
+ EState *estate;
+ ModifyTableState *mtstate;
+} InsertModifyBufferFlushContext;
+
+static InsertModifyBufferFlushContext *insert_modify_buffer_flush_context = NULL;
+static TableModifyState *table_modify_state = NULL;
+
+static void InsertModifyBufferFlushCallback(void *context,
+ TupleTableSlot *slot);
static void ExecBatchInsert(ModifyTableState *mtstate,
ResultRelInfo *resultRelInfo,
@@ -174,6 +189,8 @@ static TupleTableSlot *ExecMergeNotMatched(ModifyTableContext *context,
ResultRelInfo *resultRelInfo,
bool canSetTag);
+static bool ContainVolatileFunctionsChecker(Oid func_id, void *context);
+static bool IsMultiInsertCapable(ModifyTableState *mtstate);
/*
* Verify that the tuples to be produced by INSERT match the
@@ -806,6 +823,31 @@ ExecGetUpdateNewTuple(ResultRelInfo *relinfo,
return ExecProject(newProj);
}
+static void
+InsertModifyBufferFlushCallback(void *context, TupleTableSlot *slot)
+{
+ InsertModifyBufferFlushContext *ctx = (InsertModifyBufferFlushContext *) context;
+ ResultRelInfo *resultRelInfo = ctx->resultRelInfo;
+ EState *estate = ctx->estate;
+
+ /* Caller must take care of opening and closing the indices */
+
+ /*
+ * If there are any indexes, update them for all the inserted tuples, and
+ * run AFTER ROW INSERT triggers.
+ */
+ if (resultRelInfo->ri_NumIndices > 0)
+ {
+ List *recheckIndexes;
+
+ recheckIndexes =
+ ExecInsertIndexTuples(resultRelInfo,
+ slot, estate, false,
+ false, NULL, NIL, false);
+ list_free(recheckIndexes);
+ }
+}
+
/* ----------------------------------------------------------------
* ExecInsert
*
@@ -1209,17 +1251,22 @@ ExecInsert(ModifyTableContext *context,
}
else
{
- /* insert the tuple normally */
- table_tuple_insert(resultRelationDesc, slot,
- estate->es_output_cid,
- 0, NULL);
-
- /* insert index entries for tuple */
- if (resultRelInfo->ri_NumIndices > 0)
- recheckIndexes = ExecInsertIndexTuples(resultRelInfo,
- slot, estate, false,
- false, NULL, NIL,
- false);
+ if (table_modify_state != NULL)
+ table_modify_buffer_insert(table_modify_state, slot);
+ else
+ {
+ /* insert the tuple normally */
+ table_tuple_insert(resultRelationDesc, slot,
+ estate->es_output_cid,
+ 0, NULL);
+
+ /* insert index entries for tuple */
+ if (resultRelInfo->ri_NumIndices > 0)
+ recheckIndexes = ExecInsertIndexTuples(resultRelInfo,
+ slot, estate, false,
+ false, NULL, NIL,
+ false);
+ }
}
}
@@ -4586,6 +4633,13 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
mtstate->mt_mergeActionLists = mergeActionLists;
mtstate->mt_mergeJoinConditions = mergeJoinConditions;
+ /*
+ * Previous ModifyTable node execution (if any) should have released
+ * these resources.
+ */
+ Assert(insert_modify_buffer_flush_context == NULL &&
+ table_modify_state == NULL);
+
/*----------
* Resolve the target relation. This is the same as:
*
@@ -4999,6 +5053,8 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
*
* We only do this for INSERT, so that for UPDATE/DELETE the batch size
* remains set to 0.
+ *
+ * Also determine whether we can use batching for this INSERT command.
*/
if (operation == CMD_INSERT)
{
@@ -5016,6 +5072,27 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
}
else
resultRelInfo->ri_BatchSize = 1;
+
+ if (IsMultiInsertCapable(mtstate))
+ {
+ insert_modify_buffer_flush_context =
+ (InsertModifyBufferFlushContext *) palloc0(sizeof(InsertModifyBufferFlushContext));
+ insert_modify_buffer_flush_context->resultRelInfo = resultRelInfo;
+ insert_modify_buffer_flush_context->estate = estate;
+ insert_modify_buffer_flush_context->mtstate = mtstate;
+
+ Assert(estate->es_output_cid != InvalidCommandId);
+
+ table_modify_state =
+ table_modify_begin(resultRelInfo->ri_RelationDesc,
+ estate->es_output_cid,
+ 0,
+ InsertModifyBufferFlushCallback,
+ insert_modify_buffer_flush_context);
+
+ /* For more accurate EXPLAIN output */
+ node->canUseBatching = true;
+ }
}
/*
@@ -5034,6 +5111,90 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
return mtstate;
}
+/*
+ * Returns true if batch insert can be performed in table whithin current query.
+ * We impose the following rules:
+ * 1) Batching is supported only for ordinary tables without ROW triggers
+ * and with append_optimized option set.
+ * 2) Batching is not supported for queries, containing RETURNING clause.
+ * 3) Batching is not supported for queries, containing any volatile
+ * functions in plan tree.
+ * 4) Batching is supported only for tables, that hasn't volatile default
+ * expressions.
+ */
+static bool
+IsMultiInsertCapable(ModifyTableState *mtstate)
+{
+ ResultRelInfo *relinfo = mtstate->resultRelInfo;
+ TupleDesc tdesc = RelationGetDescr(relinfo->ri_RelationDesc);
+ bool has_row_triggers;
+
+ Assert(mtstate->operation == CMD_INSERT);
+
+ has_row_triggers =
+ (relinfo->ri_TrigDesc != NULL &&
+ (relinfo->ri_TrigDesc->trig_insert_after_row ||
+ relinfo->ri_TrigDesc->trig_insert_before_row ||
+ relinfo->ri_TrigDesc->trig_insert_instead_row));
+
+ /* Check (1) - (3) conditions. */
+ if (!RelationIsAppendOptimized(relinfo->ri_RelationDesc) ||
+ relinfo->ri_projectReturning ||
+ has_row_triggers)
+ {
+ return false;
+ }
+
+ /* Check last condition. */
+
+ /*
+ * By default, this variable is calculated in the end of ExecInitNode
+ * processing, but we need it now.
+ */
+ if ((outerPlanState(mtstate) != NULL &&
+ outerPlanState(mtstate)->has_volatile) ||
+ (innerPlanState(mtstate) != NULL &&
+ innerPlanState(mtstate)->has_volatile))
+ {
+ mtstate->ps.has_volatile = true;
+ return false;
+ }
+
+ for (AttrNumber i = 0; i < tdesc->natts; i++)
+ {
+ Node *defexpr;
+ if (!TupleDescAttr(tdesc, i)->atthasdef)
+ continue;
+
+ defexpr = TupleDescGetDefault(tdesc, i + 1);
+ if (contain_volatile_functions_extended(defexpr,
+ ContainVolatileFunctionsChecker))
+ {
+ return false;
+ }
+ }
+
+ /* All conditions are met - we can perform batch insert on table. */
+ return true;
+}
+
+/*
+ * Supportive function for IsMultiInsertCapable.
+ *
+ * To decide whether we can use batching, we should iterate across all default
+ * expressions in target table and check if they contain any volatile functions.
+ *
+ * But not all functions are considered dangerous in terms of batching. We can
+ * allow some volatile functions to appear in default expressions. For now, we
+ * only allow to use nextval (in order not to dismiss batching if target table
+ * has SERIAL filed).
+ */
+static bool ContainVolatileFunctionsChecker(Oid func_id, void *context)
+{
+ return (func_volatile(func_id) == PROVOLATILE_VOLATILE &&
+ func_id != F_NEXTVAL);
+}
+
/* ----------------------------------------------------------------
* ExecEndModifyTable
*
@@ -5047,6 +5208,17 @@ ExecEndModifyTable(ModifyTableState *node)
{
int i;
+ if (table_modify_state != NULL)
+ {
+ Assert(node->operation == CMD_INSERT);
+
+ table_modify_end(table_modify_state);
+ table_modify_state = NULL;
+
+ pfree(insert_modify_buffer_flush_context);
+ insert_modify_buffer_flush_context = NULL;
+ }
+
/*
* Allow any FDWs to shut down
*/
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index a8f22a8c154..7bf13de1e93 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -7133,6 +7133,7 @@ make_modifytable(PlannerInfo *root, Plan *subplan,
node->operation = operation;
node->canSetTag = canSetTag;
+ node->canUseBatching = false;
node->nominalRelation = nominalRelation;
node->rootRelation = rootRelation;
node->partColsUpdated = partColsUpdated;
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 26a3e050086..91ee85e9157 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -157,6 +157,14 @@ static Node *substitute_actual_srf_parameters_mutator(Node *node,
substitute_actual_srf_parameters_context *context);
static bool pull_paramids_walker(Node *node, Bitmapset **context);
+/*
+ * Allow user to supply specific checker for "contain_volatile_functions" call.
+ * In general it is not used, but for example append-optimized tables needs to
+ * ignore some types of volatile functions during default expressions check.
+ */
+
+static bool contain_volatile_functions_checker(Oid func_id, void *context);
+static check_function_callback checker = contain_volatile_functions_checker;
/*****************************************************************************
* Aggregate-function clause manipulation
@@ -541,6 +549,23 @@ contain_volatile_functions(Node *clause)
return contain_volatile_functions_walker(clause, NULL);
}
+/*
+ * Same as above, but allows to specify user-defined check_function_callback.
+ */
+bool
+contain_volatile_functions_extended(Node *clause,
+ check_function_callback ud_checker)
+{
+ bool res;
+ check_function_callback prev_checker = checker;
+
+ checker = ud_checker;
+ res = contain_volatile_functions_walker(clause, NULL);
+ checker = prev_checker;
+
+ return res;
+}
+
static bool
contain_volatile_functions_checker(Oid func_id, void *context)
{
@@ -553,8 +578,7 @@ contain_volatile_functions_walker(Node *node, void *context)
if (node == NULL)
return false;
/* Check for volatile functions in node itself */
- if (check_functions_in_node(node, contain_volatile_functions_checker,
- context))
+ if (check_functions_in_node(node, checker, context))
return true;
if (IsA(node, NextValueExpr))
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index e48fe434cd3..96b9e925e66 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -30,6 +30,7 @@
#include "storage/shm_toc.h"
#include "utils/relcache.h"
#include "utils/snapshot.h"
+#include "utils/tuplestore.h"
/* "options" flag bits for heap_insert */
@@ -270,6 +271,35 @@ typedef enum
PRUNE_VACUUM_CLEANUP, /* VACUUM 2nd heap pass */
} PruneReason;
+/*
+ * Maximum number of slots that multi-insert buffers can hold.
+ *
+ * Caution: Don't make this too big, as we could end up with this many tuples
+ * stored in multi insert buffer.
+ */
+#define HEAP_MAX_BUFFERED_SLOTS 1000
+
+typedef struct HeapMultiInsertState
+{
+ /* Array of buffered slots */
+ TupleTableSlot **slots;
+
+ /* Holds the tuple set */
+ Tuplestorestate *tstore;
+
+ /* Number of buffered tuples currently held */
+ int nused;
+
+ /* Memory context for dealing with multi inserts */
+ MemoryContext mem_ctx;
+} HeapMultiInsertState;
+
+typedef struct HeapInsertState
+{
+ struct BulkInsertStateData *bistate;
+ HeapMultiInsertState *mistate;
+} HeapInsertState;
+
/* ----------------
* function prototypes for heap access method
*
@@ -320,6 +350,17 @@ extern void heap_insert(Relation relation, HeapTuple tup, CommandId cid,
extern void heap_multi_insert(Relation relation, struct TupleTableSlot **slots,
int ntuples, CommandId cid, int options,
BulkInsertState bistate);
+
+extern TableModifyState *heap_modify_begin(Relation rel,
+ CommandId cid,
+ int options,
+ TableModifyBufferFlushCb buffer_flush_cb,
+ void *buffer_flush_ctx);
+extern void heap_modify_buffer_insert(TableModifyState *state,
+ TupleTableSlot *slot);
+extern void heap_modify_buffer_flush(TableModifyState *state);
+extern void heap_modify_end(TableModifyState *state);
+
extern TM_Result heap_delete(Relation relation, ItemPointer tid,
CommandId cid, Snapshot crosscheck, bool wait,
struct TM_FailureData *tmfd, bool changingPart);
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 8713e12cbfb..3942463b715 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -248,12 +248,44 @@ typedef struct TM_IndexDeleteOp
TM_IndexStatus *status;
} TM_IndexDeleteOp;
+struct TableModifyState;
+
+/* Callback invoked upon flushing each buffered tuple */
+typedef void (*TableModifyBufferFlushCb) (void *context,
+ TupleTableSlot *slot);
+
+/* Holds table modify state */
+typedef struct TableModifyState
+{
+ /* These fields are used for inserts for now */
+
+ Relation rel; /* Relation to insert to */
+ CommandId cid; /* Command ID for insert */
+ int options; /* TABLE_INSERT options */
+
+ /* Memory context for dealing with modify state variables */
+ MemoryContext mem_ctx;
+
+ /* Flush callback and its context used for multi inserts */
+ TableModifyBufferFlushCb buffer_flush_cb;
+ void *buffer_flush_ctx;
+
+ /* Table AM specific data */
+ void *data;
+} TableModifyState;
+
/* "options" flag bits for table_tuple_insert */
/* TABLE_INSERT_SKIP_WAL was 0x0001; RelationNeedsWAL() now governs */
#define TABLE_INSERT_SKIP_FSM 0x0002
#define TABLE_INSERT_FROZEN 0x0004
#define TABLE_INSERT_NO_LOGICAL 0x0008
+/*
+ * Use BAS_BULKWRITE buffer access strategy. 0x0010 is for
+ * HEAP_INSERT_SPECULATIVE.
+ */
+#define TABLE_INSERT_BAS_BULKWRITE 0x0020
+
/* flag bits for table_tuple_lock */
/* Follow tuples whose update is in progress if lock modes don't conflict */
#define TUPLE_LOCK_FLAG_LOCK_UPDATE_IN_PROGRESS (1 << 0)
@@ -571,6 +603,21 @@ typedef struct TableAmRoutine
void (*finish_bulk_insert) (Relation rel, int options);
+ /* ------------------------------------------------------------------------
+ * Table Modify related functions.
+ * ------------------------------------------------------------------------
+ */
+ TableModifyState *(*tuple_modify_begin) (Relation rel,
+ CommandId cid,
+ int options,
+ TableModifyBufferFlushCb buffer_flush_cb,
+ void *buffer_flush_ctx);
+ void (*tuple_modify_buffer_insert) (TableModifyState *state,
+ TupleTableSlot *slot);
+ void (*tuple_modify_buffer_flush) (TableModifyState *state);
+ void (*tuple_modify_end) (TableModifyState *state);
+
+
/* ------------------------------------------------------------------------
* DDL related functionality.
* ------------------------------------------------------------------------
@@ -1560,6 +1607,43 @@ table_finish_bulk_insert(Relation rel, int options)
}
+/* ------------------------------------------------------------------------
+ * Table Modify related functions.
+ * ------------------------------------------------------------------------
+ */
+static inline TableModifyState *
+table_modify_begin(Relation rel,
+ CommandId cid,
+ int options,
+ TableModifyBufferFlushCb buffer_flush_cb,
+ void *buffer_flush_ctx)
+{
+ return rel->rd_tableam->tuple_modify_begin(rel,
+ cid,
+ options,
+ buffer_flush_cb,
+ buffer_flush_ctx);
+}
+
+static inline void
+table_modify_buffer_insert(TableModifyState *state, TupleTableSlot *slot)
+{
+ state->rel->rd_tableam->tuple_modify_buffer_insert(state, slot);
+}
+
+static inline void
+table_modify_buffer_flush(TableModifyState *state)
+{
+ state->rel->rd_tableam->tuple_modify_buffer_flush(state);
+}
+
+static inline void
+table_modify_end(TableModifyState *state)
+{
+ state->rel->rd_tableam->tuple_modify_end(state);
+}
+
+
/* ------------------------------------------------------------------------
* DDL related functionality.
* ------------------------------------------------------------------------
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 5b6cadb5a6c..cbd798187eb 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1200,6 +1200,12 @@ typedef struct PlanState
bool async_capable; /* true if node is async-capable */
+ /*
+ * Qual of current node or any qual of nodes lower down the plan tree has
+ * at least one volatile function.
+ */
+ bool has_volatile;
+
/*
* Scanslot's descriptor if known. This is a bit of a hack, but otherwise
* it's hard for expression compilation to optimize based on the
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 658d76225e4..3a38040d991 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -292,6 +292,8 @@ typedef struct ModifyTable
CmdType operation;
/* do we set the command tag/es_processed? */
bool canSetTag;
+ /* do we use batching during INSERT? */
+ bool canUseBatching;
/* Parent RT index for use of EXPLAIN */
Index nominalRelation;
/* Root RT index, if partitioned/inherited */
diff --git a/src/include/optimizer/optimizer.h b/src/include/optimizer/optimizer.h
index 546828b54bd..9bda34d21bc 100644
--- a/src/include/optimizer/optimizer.h
+++ b/src/include/optimizer/optimizer.h
@@ -22,6 +22,7 @@
#ifndef OPTIMIZER_H
#define OPTIMIZER_H
+#include "nodes/nodeFuncs.h"
#include "nodes/parsenodes.h"
/*
@@ -142,6 +143,8 @@ extern Expr *canonicalize_qual(Expr *qual, bool is_check);
extern bool contain_mutable_functions(Node *clause);
extern bool contain_mutable_functions_after_planning(Expr *expr);
extern bool contain_volatile_functions(Node *clause);
+extern bool contain_volatile_functions_extended(Node *clause,
+ check_function_callback ud_checker);
extern bool contain_volatile_functions_after_planning(Expr *expr);
extern bool contain_volatile_functions_not_nextval(Node *clause);
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index b552359915f..e548954d81d 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -348,6 +348,7 @@ typedef struct StdRdOptions
StdRdOptIndexCleanup vacuum_index_cleanup; /* controls index vacuuming */
bool vacuum_truncate; /* enables vacuum to truncate a relation */
bool vacuum_truncate_set; /* whether vacuum_truncate is set */
+ bool append_optimized; /* use optimized insertion algorithm */
/*
* Fraction of pages in a relation that vacuum can eagerly scan and fail
@@ -367,6 +368,15 @@ typedef struct StdRdOptions
((relation)->rd_options ? \
((StdRdOptions *) (relation)->rd_options)->toast_tuple_target : (defaulttarg))
+/*
+ * RelationIsAppendOptimized
+ * Check whether relation can use batching for insertion
+ */
+ #define RelationIsAppendOptimized(relation) \
+ (AssertMacro(RelationIsValid(relation)), \
+ (relation)->rd_options ? \
+ ((StdRdOptions *) (relation)->rd_options)->append_optimized : false)
+
/*
* RelationGetFillFactor
* Returns the relation's fillfactor. Note multiple eval of argument!
diff --git a/src/test/regress/expected/append_optimized.out b/src/test/regress/expected/append_optimized.out
new file mode 100644
index 00000000000..57b45a20e61
--- /dev/null
+++ b/src/test/regress/expected/append_optimized.out
@@ -0,0 +1,161 @@
+-- Not all INSERT queries are suitable for using batching. All conditions are
+-- listed in nodeModifyTable.c
+-- In this test we want to check whether append_optimized table correcly
+-- determines when to use batching.
+CREATE TABLE optimized_tbl (
+ int_data INT DEFAULT random()
+) WITH (append_optimized=true);
+CREATE TABLE rows_source (int_data INT);
+INSERT INTO rows_source SELECT generate_series(1, 10);
+-- Must not use batching here, because optimized_tbl has volatile function
+-- whithin default expression.
+EXPLAIN INSERT INTO optimized_tbl
+ SELECT int_data FROM rows_source;
+ QUERY PLAN
+---------------------------------------------------------------------
+ Insert on optimized_tbl (cost=0.00..35.50 rows=0 width=0)
+ -> Seq Scan on rows_source (cost=0.00..35.50 rows=2550 width=4)
+(2 rows)
+
+-- Now default expression not prevent us from using batching.
+ALTER TABLE optimized_tbl ALTER COLUMN int_data SET DEFAULT 0;
+EXPLAIN INSERT INTO optimized_tbl
+ SELECT int_data FROM rows_source;
+ QUERY PLAN
+---------------------------------------------------------------------
+ MultiInsert on optimized_tbl (cost=0.00..35.50 rows=0 width=0)
+ -> Seq Scan on rows_source (cost=0.00..35.50 rows=2550 width=4)
+(2 rows)
+
+-- Must not use batching here, because WHERE clause contains volatile function.
+EXPLAIN INSERT INTO optimized_tbl
+ SELECT int_data FROM rows_source
+ WHERE int_data > random();
+ QUERY PLAN
+--------------------------------------------------------------------
+ Insert on optimized_tbl (cost=0.00..54.63 rows=0 width=0)
+ -> Seq Scan on rows_source (cost=0.00..54.63 rows=850 width=4)
+ Filter: ((int_data)::double precision > random())
+(3 rows)
+
+-- Now WHERE clause not prevent us from using batching.
+EXPLAIN INSERT INTO optimized_tbl
+ SELECT int_data FROM rows_source
+ WHERE int_data > 2;
+ QUERY PLAN
+--------------------------------------------------------------------
+ MultiInsert on optimized_tbl (cost=0.00..41.88 rows=0 width=0)
+ -> Seq Scan on rows_source (cost=0.00..41.88 rows=850 width=4)
+ Filter: (int_data > 2)
+(3 rows)
+
+-- Create ROW trigger on optimized_tbl.
+CREATE OR REPLACE FUNCTION my_trigger_function()
+RETURNS TRIGGER AS $$
+BEGIN
+ NEW.int_data := NEW.int_data * 10;
+ RETURN NEW;
+END;
+$$ LANGUAGE plpgsql;
+CREATE TRIGGER my_row_trigger
+BEFORE INSERT ON optimized_tbl
+FOR EACH ROW
+EXECUTE FUNCTION my_trigger_function();
+-- Must not use batching here, because optimized_tbl has ROW trigger.
+EXPLAIN INSERT INTO optimized_tbl
+ SELECT int_data FROM rows_source;
+ QUERY PLAN
+---------------------------------------------------------------------
+ Insert on optimized_tbl (cost=0.00..35.50 rows=0 width=0)
+ -> Seq Scan on rows_source (cost=0.00..35.50 rows=2550 width=4)
+(2 rows)
+
+DROP TRIGGER my_row_trigger ON optimized_tbl;
+DROP FUNCTION my_trigger_function();
+-- Must not use batching here, because RETURNING clause is specified.
+EXPLAIN INSERT INTO optimized_tbl VALUES (100) RETURNING int_data;
+ QUERY PLAN
+-----------------------------------------------------------
+ Insert on optimized_tbl (cost=0.00..0.01 rows=1 width=4)
+ -> Result (cost=0.00..0.01 rows=1 width=4)
+(2 rows)
+
+-- Now RETURNING not prevent us from using batching.
+EXPLAIN INSERT INTO optimized_tbl VALUES (100);
+ QUERY PLAN
+----------------------------------------------------------------
+ MultiInsert on optimized_tbl (cost=0.00..0.01 rows=0 width=0)
+ -> Result (cost=0.00..0.01 rows=1 width=4)
+(2 rows)
+
+TRUNCATE optimized_tbl;
+CREATE INDEX idx_test_int_data ON optimized_tbl (int_data);
+-- Fill source table with more data, so there will be several buffers flushs
+-- during INSERT opration.
+INSERT INTO rows_source SELECT generate_series(11, 10000);
+-- It is OK to use batching.
+EXPLAIN INSERT INTO optimized_tbl
+ SELECT int_data FROM rows_source;
+ QUERY PLAN
+-----------------------------------------------------------------------
+ MultiInsert on optimized_tbl (cost=0.00..159.75 rows=0 width=0)
+ -> Seq Scan on rows_source (cost=0.00..159.75 rows=11475 width=4)
+(2 rows)
+
+INSERT INTO optimized_tbl
+SELECT int_data FROM rows_source;
+-- Check whether both index and table contains all inserted rows.
+SELECT COUNT(*) FROM optimized_tbl;
+ count
+-------
+ 10000
+(1 row)
+
+ANALYZE optimized_tbl;
+SELECT c.relname, c.reltuples
+FROM pg_class c
+JOIN pg_index i ON c.oid = i.indexrelid
+WHERE i.indrelid = 'optimized_tbl'::regclass;
+ relname | reltuples
+-------------------+-----------
+ idx_test_int_data | 10000
+(1 row)
+
+-- We allow to use SERIAL field in append_optimized table. Check whether such
+-- fields behave correctly.
+CREATE TABLE test_serial(
+ id SERIAL,
+ int_data INT
+) WITH (append_optimized=true);
+CREATE TABLE small_source(int_data INT);
+INSERT INTO small_source SELECT generate_series(1, 10);
+EXPLAIN INSERT INTO test_serial(int_data)
+ SELECT int_data FROM small_source;
+ QUERY PLAN
+----------------------------------------------------------------------
+ MultiInsert on test_serial (cost=0.00..48.25 rows=0 width=0)
+ -> Seq Scan on small_source (cost=0.00..48.25 rows=2550 width=8)
+(2 rows)
+
+INSERT INTO test_serial(int_data)
+SELECT int_data FROM small_source;
+SELECT * FROM test_serial;
+ id | int_data
+----+----------
+ 1 | 1
+ 2 | 2
+ 3 | 3
+ 4 | 4
+ 5 | 5
+ 6 | 6
+ 7 | 7
+ 8 | 8
+ 9 | 9
+ 10 | 10
+(10 rows)
+
+-- Cleanup
+DROP TABLE optimized_tbl;
+DROP TABLE rows_source;
+DROP TABLE test_serial;
+DROP TABLE small_source;
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index 0a35f2f8f6a..0cda71a358d 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -136,3 +136,5 @@ test: fast_default
# run tablespace test at the end because it drops the tablespace created during
# setup that other tests may use.
test: tablespace
+
+test: append_optimized
diff --git a/src/test/regress/sql/append_optimized.sql b/src/test/regress/sql/append_optimized.sql
new file mode 100644
index 00000000000..ce3ffab2d52
--- /dev/null
+++ b/src/test/regress/sql/append_optimized.sql
@@ -0,0 +1,105 @@
+-- Not all INSERT queries are suitable for using batching. All conditions are
+-- listed in nodeModifyTable.c
+-- In this test we want to check whether append_optimized table correcly
+-- determines when to use batching.
+
+CREATE TABLE optimized_tbl (
+ int_data INT DEFAULT random()
+) WITH (append_optimized=true);
+
+CREATE TABLE rows_source (int_data INT);
+INSERT INTO rows_source SELECT generate_series(1, 10);
+
+-- Must not use batching here, because optimized_tbl has volatile function
+-- whithin default expression.
+EXPLAIN INSERT INTO optimized_tbl
+ SELECT int_data FROM rows_source;
+
+-- Now default expression not prevent us from using batching.
+ALTER TABLE optimized_tbl ALTER COLUMN int_data SET DEFAULT 0;
+EXPLAIN INSERT INTO optimized_tbl
+ SELECT int_data FROM rows_source;
+
+-- Must not use batching here, because WHERE clause contains volatile function.
+EXPLAIN INSERT INTO optimized_tbl
+ SELECT int_data FROM rows_source
+ WHERE int_data > random();
+
+-- Now WHERE clause not prevent us from using batching.
+EXPLAIN INSERT INTO optimized_tbl
+ SELECT int_data FROM rows_source
+ WHERE int_data > 2;
+
+-- Create ROW trigger on optimized_tbl.
+CREATE OR REPLACE FUNCTION my_trigger_function()
+RETURNS TRIGGER AS $$
+BEGIN
+ NEW.int_data := NEW.int_data * 10;
+ RETURN NEW;
+END;
+$$ LANGUAGE plpgsql;
+
+CREATE TRIGGER my_row_trigger
+BEFORE INSERT ON optimized_tbl
+FOR EACH ROW
+EXECUTE FUNCTION my_trigger_function();
+
+-- Must not use batching here, because optimized_tbl has ROW trigger.
+EXPLAIN INSERT INTO optimized_tbl
+ SELECT int_data FROM rows_source;
+
+DROP TRIGGER my_row_trigger ON optimized_tbl;
+DROP FUNCTION my_trigger_function();
+
+-- Must not use batching here, because RETURNING clause is specified.
+EXPLAIN INSERT INTO optimized_tbl VALUES (100) RETURNING int_data;
+
+-- Now RETURNING not prevent us from using batching.
+EXPLAIN INSERT INTO optimized_tbl VALUES (100);
+
+TRUNCATE optimized_tbl;
+CREATE INDEX idx_test_int_data ON optimized_tbl (int_data);
+
+-- Fill source table with more data, so there will be several buffers flushs
+-- during INSERT opration.
+INSERT INTO rows_source SELECT generate_series(11, 10000);
+
+-- It is OK to use batching.
+EXPLAIN INSERT INTO optimized_tbl
+ SELECT int_data FROM rows_source;
+
+INSERT INTO optimized_tbl
+SELECT int_data FROM rows_source;
+
+-- Check whether both index and table contains all inserted rows.
+SELECT COUNT(*) FROM optimized_tbl;
+ANALYZE optimized_tbl;
+
+SELECT c.relname, c.reltuples
+FROM pg_class c
+JOIN pg_index i ON c.oid = i.indexrelid
+WHERE i.indrelid = 'optimized_tbl'::regclass;
+
+-- We allow to use SERIAL field in append_optimized table. Check whether such
+-- fields behave correctly.
+CREATE TABLE test_serial(
+ id SERIAL,
+ int_data INT
+) WITH (append_optimized=true);
+
+CREATE TABLE small_source(int_data INT);
+INSERT INTO small_source SELECT generate_series(1, 10);
+
+EXPLAIN INSERT INTO test_serial(int_data)
+ SELECT int_data FROM small_source;
+
+INSERT INTO test_serial(int_data)
+SELECT int_data FROM small_source;
+
+SELECT * FROM test_serial;
+
+-- Cleanup
+DROP TABLE optimized_tbl;
+DROP TABLE rows_source;
+DROP TABLE test_serial;
+DROP TABLE small_source;
--
2.43.0
^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: Introduce new multi insert Table AM and improve performance of various SQL commands with it for Heap AM
@ 2026-04-28 04:27 Haibo Yan <[email protected]>
parent: Daniil Davydov <[email protected]>
0 siblings, 0 replies; 47+ messages in thread
From: Haibo Yan @ 2026-04-28 04:27 UTC (permalink / raw)
To: Daniil Davydov <[email protected]>; +Cc: Jingtang Zhang <[email protected]>; [email protected]; pgsql-hackers; Bharath Rupireddy <[email protected]>
On Sun, Apr 26, 2026 at 2:56 PM Daniil Davydov <[email protected]> wrote:
> Hi,
>
> On Sun, Apr 6, 2025 at 8:55 PM Jingtang Zhang <[email protected]>
> wrote:
> >
> > It was quite a while since I last looked at the patch. I've tested it
> again,
> > and still get regression on patched version where a table has many
> columns.
> > And it is totally CPU-bounded on tts_virtual_copyslot.
> >
> > Unpatched version:
> > 1 col:
> > Time: 8909.714 ms (00:08.910)
> > Time: 8803.579 ms (00:08.804)
> > Time: 8600.415 ms (00:08.600)
> > 32 cols:
> > Time: 12911.699 ms (00:12.912)
> > Time: 13543.491 ms (00:13.543)
> > Time: 13325.368 ms (00:13.325)
> >
> > Patched version:
> > 1 col:
> > Time: 3532.841 ms (00:03.533)
> > Time: 3598.223 ms (00:03.598)
> > Time: 3515.858 ms (00:03.516)
> > 32 cols:
> > Time: 35647.724 ms (00:35.648)
> > Time: 35596.233 ms (00:35.596)
> > Time: 35669.106 ms (00:35.669)
> >
>
> Hm, maybe I didn't choose the best way to measure performance. Can you
> please share how you do it?
>
> > I've tested your patch with tuplestore and found the regression does not
> exist
> > anymore, but I haven't look deep inside it.
> >
> > Patched version (with tuplestore):
> > 1 col:
> > Time: 3500.502 ms (00:03.501)
> > Time: 3486.886 ms (00:03.487)
> > Time: 3514.233 ms (00:03.514)
> > 32 cols:
> > Time: 10375.391 ms (00:10.375)
> > Time: 10248.256 ms (00:10.248)
> > Time: 10248.289 ms (00:10.248)
> >
> > It seems to be a good idea if there is no other issue with your patch.
>
> As far as I understand, the use of multi inserts for queries like
> "INSERT INTO ... SELECT FROM" is not discussed here anymore due to the
> fact that in such cases we will have to take into account the volatile
> functions and ROW triggers.
> I've been thinking about this for a while and made a patch as an
> experiment. The principles that the patch works on are listed below.
> 1)
> Since performance decreases for single INSERTs (within a multi inserts
> mechanism), I designed this feature as an option for the table. Thus,
> if the user knows that he will perform a lot of inserts on the table,
> he can specify "WITH (append_optimized=true)".
> 2)
> The availability of volatile functions is monitored during the
> construction of a subtree for a ModifyTable node. I'm not that
> familiar with the query plan construction mechanism, but it seems to
> me that this way we can track any occurrence of volatile functions.
> Of course, most volatile functions don't stop us from using multi
> inserts, but checking each such function would take a very long time,
> so the very fact of having a volatile function is enough for us to
> abandon multi-inserts.
> 3)
> Default expressions of the target table are also checked for volatile
> functions. The same rules apply to them as in (2). As an exception, I
> allowed the use of SERIAL in the column data type, since this is a
> fairly common use case.
> 4)
> If the target table contains any ROW triggers, we don't use multi insert.
> 5)
> Patch also contains a regression test. This is a "sandbox" where you
> can do some experiments with append-optimized tables.
>
> I hope that patch (targeted on 'master' branch,
> 2c7bd2ba507e273f2d7fe1b2f6d30775ed4f3c09) will be useful for this
> thread.
>
> --
> Best regards,
> Daniil Davydov
>
Hi all,
I picked this work up again and implemented the full 5-patch series.
The series is structured as follows:
- 0001 adds the buffered-insert lifecycle API in tableam/heapam and
provides the heap implementation.
- 0002 adopts the API for CTAS.
- 0003 adopts the API for CREATE MATERIALIZED VIEW and REFRESH
MATERIALIZED VIEW.
- 0004 adopts the API for COPY FROM.
- 0005 adopts the API for a restricted first step of INSERT INTO … SELECT
.
I also reran performance testing locally on my machine:
- Hardware: MacBook Pro M4, 36GB RAM
- shared_buffers: 128MB
I compared the unpatched baseline against the current patched series for
CTAS, CMV, RMV, and INSERT INTO ... SELECT.
Table 1 — Median (ms)
Workload
10K Before
10K After
10K Improv
100K Before
100K After
100K Improv
1M Before
1M After
1M Improv
CTAS
1.60
1.17
+26.9%
9.19
5.70
+38.0%
105.61
73.28
+30.6%
CMV
2.11
1.77
+16.1%
10.28
6.20
+39.7%
110.10
79.64
+27.7%
RMV
1.62
1.19
+26.5%
9.91
5.43
+45.2%
106.04
69.57
+34.4%
INSERT … SELECT
1.72
0.89
+48.3%
15.39
7.46
+51.5%
228.24
84.66
+62.9%
Table 2 — Average (ms)
Workload
10K Before
10K After
10K Improv
100K Before
100K After
100K Improv
1M Before
1M After
1M Improv
CTAS
1.65
1.26
+24.1%
9.37
5.82
+37.9%
104.81
74.31
+29.1%
CMV
2.34
1.82
+22.2%
10.32
6.25
+39.4%
110.32
80.50
+27.0%
RMV
1.92
1.21
+36.9%
9.86
5.49
+44.3%
106.79
69.53
+34.9%
INSERT … SELECT
1.69
0.90
+46.8%
15.45
7.39
+52.2%
210.62
85.19
+59.6%
These numbers look ok to me to continue the discussion with the current
design and implementation.
One point worth calling out explicitly: the INSERT INTO ... SELECT support
in patch 5 is intentionally limited, as described above. It is not intended
to claim broad executor coverage yet.
One further improvement still seems possible: making the heap
implementation cache raw HeapTuple bytes directly instead of maintaining
buffered slot arrays. I looked at that direction, but did not include it in
this series because it felt like a larger scope change than what I wanted
for v1.
At this point, I think the current series is in reasonable shape and I’d
really appreciate review on both the API shape and the caller adoptions.
Thanks,
Haibo
Attachments:
[application/octet-stream] v1-0001-tableam-heapam-add-buffered-insert-lifecycle-API-.patch (41.2K, ../../CABXr29ExTDUoWZsiA0w+mkrjkJWKhPdiseM7vHJgHY-Mwq9PNg@mail.gmail.com/3-v1-0001-tableam-heapam-add-buffered-insert-lifecycle-API-.patch)
download | inline diff:
From ee069b0d3c9b15ad8d4b2ab0bbd73f1392ed02c3 Mon Sep 17 00:00:00 2001
From: Haibo Yan <[email protected]>
Date: Thu, 23 Apr 2026 11:14:36 -0700
Subject: [PATCH v1 1/5] tableam/heapam: add buffered-insert lifecycle API for
heap
Add an optional Table AM buffered-insert lifecycle API with
begin/put/flush/end callbacks and tableam wrappers.
Implement the API for heap only, including internal buffering,
heap_multi_insert()-based flushes, optional per-tuple flush callbacks,
and explicit end-of-session cleanup.
Add a small Table AM-level test module for lifecycle validation.
No caller adoption is included in this patch.
---
src/backend/access/heap/heapam.c | 321 +++++++++++++++++-
src/backend/access/heap/heapam_handler.c | 7 +-
src/backend/access/table/tableamapi.c | 12 +
src/include/access/heapam.h | 12 +
src/include/access/tableam.h | 159 +++++++++
src/test/modules/Makefile | 1 +
src/test/modules/meson.build | 1 +
.../modules/test_buffered_insert/Makefile | 23 ++
.../expected/test_buffered_insert.out | 108 ++++++
.../modules/test_buffered_insert/meson.build | 33 ++
.../sql/test_buffered_insert.sql | 41 +++
.../test_buffered_insert--1.0.sql | 30 ++
.../test_buffered_insert.c | 233 +++++++++++++
.../test_buffered_insert.control | 4 +
src/tools/pgindent/typedefs.list | 4 +
15 files changed, 970 insertions(+), 19 deletions(-)
create mode 100644 src/test/modules/test_buffered_insert/Makefile
create mode 100644 src/test/modules/test_buffered_insert/expected/test_buffered_insert.out
create mode 100644 src/test/modules/test_buffered_insert/meson.build
create mode 100644 src/test/modules/test_buffered_insert/sql/test_buffered_insert.sql
create mode 100644 src/test/modules/test_buffered_insert/test_buffered_insert--1.0.sql
create mode 100644 src/test/modules/test_buffered_insert/test_buffered_insert.c
create mode 100644 src/test/modules/test_buffered_insert/test_buffered_insert.control
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index abfd8e8970a..ae994dd202d 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -53,6 +53,7 @@
#include "utils/datum.h"
#include "utils/injection_point.h"
#include "utils/inval.h"
+#include "utils/memutils.h"
#include "utils/spccache.h"
#include "utils/syscache.h"
@@ -1982,6 +1983,263 @@ ReleaseBulkInsertStatePin(BulkInsertState bistate)
}
+/* ------------------------------------------------------------------------
+ * Heap buffered-insert lifecycle implementation.
+ * ------------------------------------------------------------------------
+ */
+
+/*
+ * Private state for the heap buffered-insert session.
+ *
+ * Memory-context hierarchy (all destroyed by MemoryContextDelete(state_ctx)):
+ *
+ * state_ctx ("HeapBufferedInsert") -- long-lived, owns stable state
+ * ├── batch_ctx ("HeapBufInsBatch") -- per-batch HeapTuples
+ * └── mi_ctx ("HeapBufInsFlush") -- per-flush transient allocations
+ * └── heap_multi_insert scratch + toasted tuple copies
+ *
+ * state_ctx owns: this struct, the HeapTuple pointer array, bistate,
+ * scratch_slot (callback-only).
+ * batch_ctx owns: HeapTuples produced by ExecCopySlotHeapTuple() during
+ * put(). It is reset (not deleted) after each flush, which bulk-frees all
+ * per-batch allocations.
+ * mi_ctx owns: heap_multi_insert's transient allocations (including any
+ * toasted-tuple copies produced by heap_prepare_insert). It is reset after
+ * each flush once the flush callback (if any) has finished reading the
+ * post-prepare tuples.
+ */
+typedef struct HeapBufferedInsertState
+{
+ TableBufferedInsertStateData base;
+ CommandId cid;
+ int options;
+ BulkInsertState bistate;
+ TableBufferedInsertFlushCb flush_cb;
+ void *flush_ctx;
+ HeapTuple *tuples; /* buffered HeapTuples (in batch_ctx) */
+ int ntuples;
+ int max_tuples;
+ Size buffered_bytes; /* sum of tuples[i]->t_len */
+ TupleTableSlot *scratch_slot; /* callback-only; HeapTuple-backed */
+ MemoryContext batch_ctx; /* per-batch HeapTuples; reset after flush */
+ MemoryContext mi_ctx; /* reset after each heap_multi_insert */
+ MemoryContext state_ctx; /* long-lived; owns stable state */
+} HeapBufferedInsertState;
+
+#define HEAP_BUFFERED_INSERT_MAX_TUPLES 1000
+#define HEAP_BUFFERED_INSERT_MAX_BYTES 65535
+
+static void heap_buffered_insert_do_flush(HeapBufferedInsertState *hstate);
+static void heap_buffered_insert_finish_bulkinsert(HeapBufferedInsertState *hstate);
+
+/* heap_multi_insert core, takes HeapTuples directly (defined near heap_multi_insert) */
+static void heap_multi_insert_raw(Relation relation, HeapTuple *heaptuples,
+ int ntuples, CommandId cid, uint32 options,
+ BulkInsertState bistate);
+
+TableBufferedInsertState
+heap_buffered_insert_begin(Relation rel, CommandId cid, int options,
+ TableBufferedInsertFlushCb flush_cb,
+ void *flush_ctx)
+{
+ HeapBufferedInsertState *hstate;
+ MemoryContext state_ctx;
+ MemoryContext old_ctx;
+
+ state_ctx = AllocSetContextCreate(CurrentMemoryContext,
+ "HeapBufferedInsert",
+ ALLOCSET_DEFAULT_SIZES);
+ old_ctx = MemoryContextSwitchTo(state_ctx);
+
+ hstate = palloc0(sizeof(HeapBufferedInsertState));
+ hstate->base.rel = rel;
+ hstate->cid = cid;
+ hstate->options = options;
+ hstate->flush_cb = flush_cb;
+ hstate->flush_ctx = flush_ctx;
+ hstate->state_ctx = state_ctx;
+
+ hstate->max_tuples = HEAP_BUFFERED_INSERT_MAX_TUPLES;
+ hstate->tuples = palloc_array(HeapTuple, hstate->max_tuples);
+ hstate->ntuples = 0;
+ hstate->buffered_bytes = 0;
+
+ if (options & TABLE_INSERT_BAS_BULKWRITE)
+ hstate->bistate = GetBulkInsertState();
+ else
+ hstate->bistate = NULL;
+
+ hstate->batch_ctx = AllocSetContextCreate(state_ctx,
+ "HeapBufInsBatch",
+ ALLOCSET_DEFAULT_SIZES);
+
+ hstate->mi_ctx = AllocSetContextCreate(state_ctx,
+ "HeapBufInsFlush",
+ ALLOCSET_DEFAULT_SIZES);
+
+ /*
+ * Allocate the scratch slot only when a flush callback is present.
+ * No-callback callers (CTAS, CMV, RMV) pay zero slot overhead.
+ */
+ if (flush_cb != NULL)
+ hstate->scratch_slot = MakeSingleTupleTableSlot(RelationGetDescr(rel),
+ &TTSOpsHeapTuple);
+ else
+ hstate->scratch_slot = NULL;
+
+ MemoryContextSwitchTo(old_ctx);
+
+ return &hstate->base;
+}
+
+void
+heap_buffered_insert_put(TableBufferedInsertState state, TupleTableSlot *slot)
+{
+ HeapBufferedInsertState *hstate = (HeapBufferedInsertState *) state;
+ MemoryContext old_ctx;
+ HeapTuple htup;
+
+ /* Auto-flush if tuple count is at capacity. */
+ if (hstate->ntuples >= hstate->max_tuples)
+ heap_buffered_insert_do_flush(hstate);
+
+ /*
+ * Produce a self-contained HeapTuple in batch_ctx. ExecCopySlotHeapTuple
+ * invokes the slot-type's copy_heap_tuple method, which for every
+ * built-in slot type allocates a new HeapTuple in CurrentMemoryContext.
+ * This is the *only* per-row materialization -- the flush path consumes
+ * the HeapTuple directly via heap_multi_insert_raw() with no further
+ * slot conversion or copy.
+ */
+ old_ctx = MemoryContextSwitchTo(hstate->batch_ctx);
+ htup = ExecCopySlotHeapTuple(slot);
+ MemoryContextSwitchTo(old_ctx);
+
+ hstate->tuples[hstate->ntuples++] = htup;
+ hstate->buffered_bytes += htup->t_len;
+
+ /* Byte-threshold flush: exact tracking from the HeapTuple t_len. */
+ if (hstate->buffered_bytes > HEAP_BUFFERED_INSERT_MAX_BYTES)
+ heap_buffered_insert_do_flush(hstate);
+}
+
+void
+heap_buffered_insert_flush(TableBufferedInsertState state)
+{
+ HeapBufferedInsertState *hstate = (HeapBufferedInsertState *) state;
+
+ if (hstate->ntuples > 0)
+ heap_buffered_insert_do_flush(hstate);
+}
+
+void
+heap_buffered_insert_end(TableBufferedInsertState state)
+{
+ HeapBufferedInsertState *hstate = (HeapBufferedInsertState *) state;
+
+ /* Flush any remaining tuples */
+ if (hstate->ntuples > 0)
+ heap_buffered_insert_do_flush(hstate);
+
+ /* Clean up the scratch slot used for flush callback */
+ if (hstate->scratch_slot != NULL)
+ ExecDropSingleTupleTableSlot(hstate->scratch_slot);
+
+ /* Perform finish-bulk-insert cleanup (subsumes table_finish_bulk_insert) */
+ heap_buffered_insert_finish_bulkinsert(hstate);
+
+ /*
+ * Release all memory owned by the state, including batch_ctx and mi_ctx
+ * (both are children of state_ctx).
+ */
+ MemoryContextDelete(hstate->state_ctx);
+}
+
+/*
+ * Internal: flush all buffered HeapTuples via heap_multi_insert_raw, then
+ * invoke the flush callback (if any) once per written tuple. Resets both
+ * per-batch and per-flush contexts so no payload survives across cycles.
+ *
+ * Callback-path isolation: the scratch_slot and tuple-to-slot conversion
+ * only execute when flush_cb is non-NULL, so no-callback callers (CTAS,
+ * CMV, RMV) pay zero callback overhead per tuple.
+ */
+static void
+heap_buffered_insert_do_flush(HeapBufferedInsertState *hstate)
+{
+ MemoryContext old_ctx;
+ int ntuples = hstate->ntuples;
+
+ Assert(ntuples > 0);
+
+ /*
+ * Switch to the per-flush memory context. heap_multi_insert_raw's
+ * transient allocations (including any toasted-tuple copies from
+ * heap_prepare_insert) land here and are bulk-freed after the callback.
+ */
+ old_ctx = MemoryContextSwitchTo(hstate->mi_ctx);
+
+ heap_multi_insert_raw(hstate->base.rel,
+ hstate->tuples,
+ ntuples,
+ hstate->cid,
+ hstate->options,
+ hstate->bistate);
+
+ MemoryContextSwitchTo(old_ctx);
+
+ /*
+ * Invoke the flush callback once per flushed tuple, in insertion order.
+ * After heap_multi_insert_raw, each tuples[i]->t_self holds the stored
+ * TID, and tuples[i] points to the post-prepare (possibly toasted) tuple
+ * in mi_ctx (if toasted) or the original in batch_ctx (if not). Both
+ * contexts are still alive here.
+ */
+ if (hstate->flush_cb != NULL)
+ {
+ for (int i = 0; i < ntuples; i++)
+ {
+ ExecStoreHeapTuple(hstate->tuples[i], hstate->scratch_slot, false);
+ hstate->scratch_slot->tts_tid = hstate->tuples[i]->t_self;
+ hstate->scratch_slot->tts_tableOid = hstate->tuples[i]->t_tableOid;
+ hstate->flush_cb(hstate->flush_ctx, hstate->scratch_slot);
+ ExecClearTuple(hstate->scratch_slot);
+ }
+ }
+
+ /* Reset both contexts now that the callback is done. */
+ MemoryContextReset(hstate->mi_ctx);
+ MemoryContextReset(hstate->batch_ctx);
+
+ hstate->ntuples = 0;
+ hstate->buffered_bytes = 0;
+}
+
+/*
+ * Perform heap-specific finish-bulk-insert cleanup.
+ *
+ * This is the buffered-insert equivalent of what callers of the non-buffered
+ * path achieve by calling FreeBulkInsertState() + table_finish_bulk_insert()
+ * at teardown. end() must subsume both.
+ *
+ * For the current in-tree heap AM, the cleanup consists of:
+ *
+ * 1. Release the BulkInsertState (buffer pin + bulk-write access strategy).
+ *
+ * 2. Any action that heap's finish_bulk_insert AM callback would perform.
+ * Heap does not currently register that callback (the slot in
+ * heapam_methods is NULL), so no additional action is required.
+ */
+static void
+heap_buffered_insert_finish_bulkinsert(HeapBufferedInsertState *hstate)
+{
+ if (hstate->bistate != NULL)
+ FreeBulkInsertState(hstate->bistate);
+
+ /* Heap does not currently register a finish_bulk_insert AM callback. */
+}
+
+
/*
* heap_insert - insert tuple into a heap
*
@@ -2268,22 +2526,24 @@ heap_multi_insert_pages(HeapTuple *heaptuples, int done, int ntuples, Size saveF
}
/*
- * heap_multi_insert - insert multiple tuples into a heap
+ * heap_multi_insert_raw - core multi-insert for a HeapTuple array
*
- * This is like heap_insert(), but inserts multiple tuples in one operation.
- * That's faster than calling heap_insert() in a loop, because when multiple
- * tuples can be inserted on a single page, we can write just a single WAL
- * record covering all of them, and only need to lock/unlock the page once.
+ * Takes an array of pre-formed HeapTuples, runs heap_prepare_insert on each
+ * (toast + header setup), and inserts them into heap pages. The heaptuples
+ * array is updated in-place: after return, each entry points to the prepared
+ * (possibly toasted) tuple with t_self set to the stored TID.
+ *
+ * This is the shared core for heap_multi_insert (slot-based callers) and
+ * heap_buffered_insert_do_flush (HeapTuple-based callers).
*
* Note: this leaks memory into the current memory context. You can create a
* temporary context before calling this, if that's a problem.
*/
-void
-heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
- CommandId cid, uint32 options, BulkInsertState bistate)
+static void
+heap_multi_insert_raw(Relation relation, HeapTuple *heaptuples, int ntuples,
+ CommandId cid, uint32 options, BulkInsertState bistate)
{
TransactionId xid = GetCurrentTransactionId();
- HeapTuple *heaptuples;
int i;
int ndone;
PGAlignedBlock scratch;
@@ -2306,16 +2566,11 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
saveFreeSpace = RelationGetTargetPageFreeSpace(relation,
HEAP_DEFAULT_FILLFACTOR);
- /* Toast and set header data in all the slots */
- heaptuples = palloc(ntuples * sizeof(HeapTuple));
+ /* Toast and set header data in all the tuples */
for (i = 0; i < ntuples; i++)
{
- HeapTuple tuple;
-
- tuple = ExecFetchSlotHeapTuple(slots[i], true, NULL);
- slots[i]->tts_tableOid = RelationGetRelid(relation);
- tuple->t_tableOid = slots[i]->tts_tableOid;
- heaptuples[i] = heap_prepare_insert(relation, tuple, xid, cid,
+ heaptuples[i]->t_tableOid = RelationGetRelid(relation);
+ heaptuples[i] = heap_prepare_insert(relation, heaptuples[i], xid, cid,
options);
}
@@ -2639,11 +2894,41 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
CacheInvalidateHeapTuple(relation, heaptuples[i], NULL);
}
+ pgstat_count_heap_insert(relation, ntuples);
+}
+
+/*
+ * heap_multi_insert - insert multiple tuples into a heap
+ *
+ * This is like heap_insert(), but inserts multiple tuples in one operation.
+ * That's faster than calling heap_insert() in a loop, because when multiple
+ * tuples can be inserted on a single page, we can write just a single WAL
+ * record covering all of them, and only need to lock/unlock the page once.
+ *
+ * Note: this leaks memory into the current memory context. You can create a
+ * temporary context before calling this, if that's a problem.
+ */
+void
+heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
+ CommandId cid, uint32 options, BulkInsertState bistate)
+{
+ HeapTuple *heaptuples;
+ int i;
+
+ heaptuples = palloc(ntuples * sizeof(HeapTuple));
+ for (i = 0; i < ntuples; i++)
+ {
+ heaptuples[i] = ExecFetchSlotHeapTuple(slots[i], true, NULL);
+ slots[i]->tts_tableOid = RelationGetRelid(relation);
+ }
+
+ heap_multi_insert_raw(relation, heaptuples, ntuples, cid, options, bistate);
+
/* copy t_self fields back to the caller's slots */
for (i = 0; i < ntuples; i++)
slots[i]->tts_tid = heaptuples[i]->t_self;
- pgstat_count_heap_insert(relation, ntuples);
+ pfree(heaptuples);
}
/*
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 20d3b46e062..65226b07a5c 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2697,7 +2697,12 @@ static const TableAmRoutine heapam_methods = {
.scan_bitmap_next_tuple = heapam_scan_bitmap_next_tuple,
.scan_sample_next_block = heapam_scan_sample_next_block,
- .scan_sample_next_tuple = heapam_scan_sample_next_tuple
+ .scan_sample_next_tuple = heapam_scan_sample_next_tuple,
+
+ .buffered_insert_begin = heap_buffered_insert_begin,
+ .buffered_insert_put = heap_buffered_insert_put,
+ .buffered_insert_flush = heap_buffered_insert_flush,
+ .buffered_insert_end = heap_buffered_insert_end,
};
diff --git a/src/backend/access/table/tableamapi.c b/src/backend/access/table/tableamapi.c
index 5450a27faeb..a0ff123fa83 100644
--- a/src/backend/access/table/tableamapi.c
+++ b/src/backend/access/table/tableamapi.c
@@ -93,6 +93,18 @@ GetTableAmRoutine(Oid amhandler)
Assert(routine->scan_sample_next_block != NULL);
Assert(routine->scan_sample_next_tuple != NULL);
+ /*
+ * Buffered-insert callbacks: either all four are NULL (AM does not
+ * support buffered inserts), or all four are non-NULL. No partially
+ * populated groups are allowed.
+ */
+ Assert((routine->buffered_insert_begin == NULL) ==
+ (routine->buffered_insert_put == NULL));
+ Assert((routine->buffered_insert_begin == NULL) ==
+ (routine->buffered_insert_flush == NULL));
+ Assert((routine->buffered_insert_begin == NULL) ==
+ (routine->buffered_insert_end == NULL));
+
return routine;
}
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 5176478c295..d8b34e37627 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -379,6 +379,18 @@ extern void heap_insert(Relation relation, HeapTuple tup, CommandId cid,
extern void heap_multi_insert(Relation relation, TupleTableSlot **slots,
int ntuples, CommandId cid, uint32 options,
BulkInsertState bistate);
+
+/* heap buffered-insert lifecycle */
+extern TableBufferedInsertState heap_buffered_insert_begin(Relation rel,
+ CommandId cid,
+ int options,
+ TableBufferedInsertFlushCb flush_cb,
+ void *flush_ctx);
+extern void heap_buffered_insert_put(TableBufferedInsertState state,
+ TupleTableSlot *slot);
+extern void heap_buffered_insert_flush(TableBufferedInsertState state);
+extern void heap_buffered_insert_end(TableBufferedInsertState state);
+
extern TM_Result heap_delete(Relation relation, const ItemPointerData *tid,
CommandId cid, uint32 options, Snapshot crosscheck,
bool wait, TM_FailureData *tmfd);
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index f2c36696bca..d5a1875d93c 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -284,6 +284,7 @@ typedef struct TM_IndexDeleteOp
#define TABLE_INSERT_SKIP_FSM 0x0002
#define TABLE_INSERT_FROZEN 0x0004
#define TABLE_INSERT_NO_LOGICAL 0x0008
+#define TABLE_INSERT_BAS_BULKWRITE 0x0020
/* "options" flag bits for table_tuple_delete */
#define TABLE_DELETE_CHANGING_PARTITION (1 << 0)
@@ -307,6 +308,40 @@ typedef void (*IndexBuildCallback) (Relation index,
bool tupleIsAlive,
void *state);
+/*
+ * State for a buffered-insert session. AMs embed this as the first field of
+ * their private state struct. The base struct is minimal: only the target
+ * relation is exposed, which is needed for AM dispatch via the inline wrapper
+ * functions.
+ *
+ * AMs that do not support buffered inserts leave the buffered_insert_begin
+ * callback NULL; callers detect this via table_buffered_insert_begin()
+ * returning NULL.
+ */
+typedef struct TableBufferedInsertStateData
+{
+ Relation rel; /* target relation -- needed for AM dispatch */
+} TableBufferedInsertStateData;
+
+typedef TableBufferedInsertStateData *TableBufferedInsertState;
+
+/*
+ * Callback invoked once per flushed tuple, in insertion order, after the AM
+ * writes a batch of buffered tuples to storage.
+ *
+ * The slot is an AM-owned scratch object used solely as a handoff vehicle.
+ * It contains the stored tuple with TID (or AM-equivalent locator) set.
+ * It is valid only for the duration of this callback invocation; the AM may
+ * reuse the same scratch slot across successive invocations within a single
+ * flush. If the caller needs data beyond the callback, it must copy within
+ * the callback body.
+ *
+ * The AM must not assume the callback is cheap, side-effect-free, or
+ * non-throwing.
+ */
+typedef void (*TableBufferedInsertFlushCb)(void *context,
+ TupleTableSlot *slot);
+
/*
* API struct for a table AM. Note this must be allocated in a
* server-lifetime manner, typically as a static const struct, which then gets
@@ -614,6 +649,64 @@ typedef struct TableAmRoutine
void (*finish_bulk_insert) (Relation rel, uint32 options);
+ /* ------------------------------------------------------------------------
+ * Buffered-insert lifecycle callbacks.
+ *
+ * Optional optimization for batch inserts. All four callbacks must be
+ * either NULL together (AM does not support buffered inserts) or non-NULL
+ * together (validated at AM registration time). No partially populated
+ * groups are allowed.
+ * ------------------------------------------------------------------------
+ */
+
+ /*
+ * Begin a buffered-insert session. Returns an opaque state handle whose
+ * first bytes are a TableBufferedInsertStateData with rel set. The AM
+ * stores cid, options, flush_cb, and flush_ctx in its private extension
+ * of the state struct.
+ *
+ * flush_cb may be NULL, in which case the AM skips per-tuple notification
+ * after flushing.
+ *
+ * Optional callback -- NULL means the AM does not support buffered inserts.
+ */
+ TableBufferedInsertState (*buffered_insert_begin)(
+ Relation rel,
+ CommandId cid,
+ int options,
+ TableBufferedInsertFlushCb flush_cb,
+ void *flush_ctx);
+
+ /*
+ * Submit one tuple for buffered insertion. The AM reads the tuple data
+ * from the slot and captures it internally before returning. The caller
+ * retains ownership of the slot and may reuse it immediately.
+ *
+ * The AM may auto-flush during this call if its internal buffer is full;
+ * the caller must be prepared for the flush callback to fire during put().
+ */
+ void (*buffered_insert_put)(
+ TableBufferedInsertState state,
+ TupleTableSlot *slot);
+
+ /*
+ * Force-flush all buffered tuples to storage. Invokes the flush callback
+ * (if non-NULL) once per flushed tuple, in insertion order, using an
+ * AM-owned scratch slot. After return, the internal buffer is empty.
+ */
+ void (*buffered_insert_flush)(
+ TableBufferedInsertState state);
+
+ /*
+ * Flush remaining buffered tuples, perform finish-bulk-insert cleanup
+ * (e.g. FSM update for heap), and release all resources owned by the
+ * state. The state pointer is invalid after this call. Callers must
+ * not separately call table_finish_bulk_insert() -- end() subsumes it.
+ */
+ void (*buffered_insert_end)(
+ TableBufferedInsertState state);
+
+
/* ------------------------------------------------------------------------
* DDL related functionality.
* ------------------------------------------------------------------------
@@ -1666,6 +1759,72 @@ table_finish_bulk_insert(Relation rel, uint32 options)
}
+/* ----------------------------------------------------------------------------
+ * Buffered-insert lifecycle functions.
+ * ----------------------------------------------------------------------------
+ */
+
+/*
+ * Begin a buffered-insert session for the given relation.
+ *
+ * Returns NULL if the relation's AM does not support buffered inserts, in
+ * which case the caller should fall back to the single-row insert path.
+ *
+ * flush_cb may be NULL when no per-tuple post-insert work is needed (e.g.
+ * CTAS/CMV/RMV). When non-NULL, it is invoked once per flushed tuple in
+ * insertion order, using an AM-owned scratch slot valid only for the duration
+ * of each callback invocation.
+ */
+static inline TableBufferedInsertState
+table_buffered_insert_begin(Relation rel, CommandId cid, int options,
+ TableBufferedInsertFlushCb flush_cb,
+ void *flush_ctx)
+{
+ if (rel->rd_tableam->buffered_insert_begin == NULL)
+ return NULL;
+ return rel->rd_tableam->buffered_insert_begin(rel, cid, options,
+ flush_cb, flush_ctx);
+}
+
+/*
+ * Submit one tuple for buffered insertion. The AM reads from the slot and
+ * captures the data internally; the caller retains slot ownership and may
+ * reuse it immediately after this call returns.
+ *
+ * The AM may auto-flush during put() if its buffer is full; the flush
+ * callback (if any) may fire during this call.
+ */
+static inline void
+table_buffered_insert_put(TableBufferedInsertState state,
+ TupleTableSlot *slot)
+{
+ state->rel->rd_tableam->buffered_insert_put(state, slot);
+}
+
+/*
+ * Force-flush all buffered tuples to storage. The flush callback (if
+ * non-NULL) fires once per flushed tuple in insertion order. After return
+ * the buffer is empty.
+ */
+static inline void
+table_buffered_insert_flush(TableBufferedInsertState state)
+{
+ state->rel->rd_tableam->buffered_insert_flush(state);
+}
+
+/*
+ * Flush remaining buffered tuples, perform finish-bulk-insert cleanup
+ * (e.g. FSM update for heap), and release all resources. The state pointer
+ * is invalid after this call. Do not separately call
+ * table_finish_bulk_insert() -- end() subsumes it.
+ */
+static inline void
+table_buffered_insert_end(TableBufferedInsertState state)
+{
+ state->rel->rd_tableam->buffered_insert_end(state);
+}
+
+
/* ------------------------------------------------------------------------
* DDL related functionality.
* ------------------------------------------------------------------------
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 0a74ab5c86f..fa964043d4f 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -20,6 +20,7 @@ SUBDIRS = \
test_binaryheap \
test_bitmapset \
test_bloomfilter \
+ test_buffered_insert \
test_cloexec \
test_checksums \
test_copy_callbacks \
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index 4bca42bb370..aae892699b9 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -20,6 +20,7 @@ subdir('test_autovacuum')
subdir('test_binaryheap')
subdir('test_bitmapset')
subdir('test_bloomfilter')
+subdir('test_buffered_insert')
subdir('test_cloexec')
subdir('test_checksums')
subdir('test_copy_callbacks')
diff --git a/src/test/modules/test_buffered_insert/Makefile b/src/test/modules/test_buffered_insert/Makefile
new file mode 100644
index 00000000000..5c4926a4c3a
--- /dev/null
+++ b/src/test/modules/test_buffered_insert/Makefile
@@ -0,0 +1,23 @@
+# src/test/modules/test_buffered_insert/Makefile
+
+MODULE_big = test_buffered_insert
+OBJS = \
+ $(WIN32RES) \
+ test_buffered_insert.o
+PGFILEDESC = "test_buffered_insert - test Table AM buffered-insert lifecycle"
+
+EXTENSION = test_buffered_insert
+DATA = test_buffered_insert--1.0.sql
+
+REGRESS = test_buffered_insert
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_buffered_insert
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_buffered_insert/expected/test_buffered_insert.out b/src/test/modules/test_buffered_insert/expected/test_buffered_insert.out
new file mode 100644
index 00000000000..79163952f6f
--- /dev/null
+++ b/src/test/modules/test_buffered_insert/expected/test_buffered_insert.out
@@ -0,0 +1,108 @@
+CREATE EXTENSION test_buffered_insert;
+-- Target table: single integer column.
+CREATE TABLE buffered_insert_test (a INT);
+-- Basic test: insert 5 rows via buffered-insert with NULL flush callback.
+SELECT test_buffered_insert_basic('buffered_insert_test'::regclass, 5);
+ test_buffered_insert_basic
+----------------------------
+ 5
+(1 row)
+
+SELECT count(*) FROM buffered_insert_test;
+ count
+-------
+ 5
+(1 row)
+
+SELECT a FROM buffered_insert_test ORDER BY a;
+ a
+---
+ 1
+ 2
+ 3
+ 4
+ 5
+(5 rows)
+
+TRUNCATE buffered_insert_test;
+-- Test with flush callback: insert 5 rows, verify callback count matches.
+SELECT test_buffered_insert_with_callback('buffered_insert_test'::regclass, 5);
+ test_buffered_insert_with_callback
+------------------------------------
+ 5
+(1 row)
+
+SELECT count(*) FROM buffered_insert_test;
+ count
+-------
+ 5
+(1 row)
+
+SELECT a FROM buffered_insert_test ORDER BY a;
+ a
+---
+ 1
+ 2
+ 3
+ 4
+ 5
+(5 rows)
+
+TRUNCATE buffered_insert_test;
+-- Trigger auto-flush: insert more rows than HEAP_BUFFERED_INSERT_MAX_SLOTS
+-- (1000) to verify auto-flush during put() works correctly.
+SELECT test_buffered_insert_basic('buffered_insert_test'::regclass, 1500);
+ test_buffered_insert_basic
+----------------------------
+ 1500
+(1 row)
+
+SELECT count(*) FROM buffered_insert_test;
+ count
+-------
+ 1500
+(1 row)
+
+-- Spot-check first and last values.
+SELECT min(a), max(a) FROM buffered_insert_test;
+ min | max
+-----+------
+ 1 | 1500
+(1 row)
+
+TRUNCATE buffered_insert_test;
+-- Verify flush callback fires for all rows including auto-flushed batches.
+SELECT test_buffered_insert_with_callback('buffered_insert_test'::regclass, 1500);
+ test_buffered_insert_with_callback
+------------------------------------
+ 1500
+(1 row)
+
+SELECT count(*) FROM buffered_insert_test;
+ count
+-------
+ 1500
+(1 row)
+
+TRUNCATE buffered_insert_test;
+-- Test explicit flush() mid-session: flush after first 5 rows, insert 5 more,
+-- then end(). Callback count must equal total rows.
+SELECT test_buffered_insert_flush_mid('buffered_insert_test'::regclass, 10);
+ test_buffered_insert_flush_mid
+--------------------------------
+ 10
+(1 row)
+
+SELECT count(*) FROM buffered_insert_test;
+ count
+-------
+ 10
+(1 row)
+
+SELECT min(a), max(a) FROM buffered_insert_test;
+ min | max
+-----+-----
+ 1 | 10
+(1 row)
+
+DROP TABLE buffered_insert_test;
diff --git a/src/test/modules/test_buffered_insert/meson.build b/src/test/modules/test_buffered_insert/meson.build
new file mode 100644
index 00000000000..d738ccb1a84
--- /dev/null
+++ b/src/test/modules/test_buffered_insert/meson.build
@@ -0,0 +1,33 @@
+# Copyright (c) 2022-2026, PostgreSQL Global Development Group
+
+test_buffered_insert_sources = files(
+ 'test_buffered_insert.c',
+)
+
+if host_system == 'windows'
+ test_buffered_insert_sources += rc_lib_gen.process(win32ver_rc, extra_args: [
+ '--NAME', 'test_buffered_insert',
+ '--FILEDESC', 'test_buffered_insert - test Table AM buffered-insert lifecycle',])
+endif
+
+test_buffered_insert = shared_module('test_buffered_insert',
+ test_buffered_insert_sources,
+ kwargs: pg_test_mod_args,
+)
+test_install_libs += test_buffered_insert
+
+test_install_data += files(
+ 'test_buffered_insert.control',
+ 'test_buffered_insert--1.0.sql',
+)
+
+tests += {
+ 'name': 'test_buffered_insert',
+ 'sd': meson.current_source_dir(),
+ 'bd': meson.current_build_dir(),
+ 'regress': {
+ 'sql': [
+ 'test_buffered_insert',
+ ],
+ },
+}
diff --git a/src/test/modules/test_buffered_insert/sql/test_buffered_insert.sql b/src/test/modules/test_buffered_insert/sql/test_buffered_insert.sql
new file mode 100644
index 00000000000..23ac09cc9bc
--- /dev/null
+++ b/src/test/modules/test_buffered_insert/sql/test_buffered_insert.sql
@@ -0,0 +1,41 @@
+CREATE EXTENSION test_buffered_insert;
+
+-- Target table: single integer column.
+CREATE TABLE buffered_insert_test (a INT);
+
+-- Basic test: insert 5 rows via buffered-insert with NULL flush callback.
+SELECT test_buffered_insert_basic('buffered_insert_test'::regclass, 5);
+SELECT count(*) FROM buffered_insert_test;
+SELECT a FROM buffered_insert_test ORDER BY a;
+
+TRUNCATE buffered_insert_test;
+
+-- Test with flush callback: insert 5 rows, verify callback count matches.
+SELECT test_buffered_insert_with_callback('buffered_insert_test'::regclass, 5);
+SELECT count(*) FROM buffered_insert_test;
+SELECT a FROM buffered_insert_test ORDER BY a;
+
+TRUNCATE buffered_insert_test;
+
+-- Trigger auto-flush: insert more rows than HEAP_BUFFERED_INSERT_MAX_SLOTS
+-- (1000) to verify auto-flush during put() works correctly.
+SELECT test_buffered_insert_basic('buffered_insert_test'::regclass, 1500);
+SELECT count(*) FROM buffered_insert_test;
+-- Spot-check first and last values.
+SELECT min(a), max(a) FROM buffered_insert_test;
+
+TRUNCATE buffered_insert_test;
+
+-- Verify flush callback fires for all rows including auto-flushed batches.
+SELECT test_buffered_insert_with_callback('buffered_insert_test'::regclass, 1500);
+SELECT count(*) FROM buffered_insert_test;
+
+TRUNCATE buffered_insert_test;
+
+-- Test explicit flush() mid-session: flush after first 5 rows, insert 5 more,
+-- then end(). Callback count must equal total rows.
+SELECT test_buffered_insert_flush_mid('buffered_insert_test'::regclass, 10);
+SELECT count(*) FROM buffered_insert_test;
+SELECT min(a), max(a) FROM buffered_insert_test;
+
+DROP TABLE buffered_insert_test;
diff --git a/src/test/modules/test_buffered_insert/test_buffered_insert--1.0.sql b/src/test/modules/test_buffered_insert/test_buffered_insert--1.0.sql
new file mode 100644
index 00000000000..26aa0635240
--- /dev/null
+++ b/src/test/modules/test_buffered_insert/test_buffered_insert--1.0.sql
@@ -0,0 +1,30 @@
+/* src/test/modules/test_buffered_insert/test_buffered_insert--1.0.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION test_buffered_insert" to load this file. \quit
+
+--
+-- Insert rows through the buffered-insert lifecycle with a NULL flush
+-- callback (the CTAS/CMV/RMV pattern). Returns the number of rows put().
+--
+CREATE FUNCTION test_buffered_insert_basic(pg_catalog.regclass, pg_catalog.int4)
+ RETURNS pg_catalog.int4
+ AS 'MODULE_PATHNAME' LANGUAGE C;
+
+--
+-- Insert rows through the buffered-insert lifecycle with a flush callback
+-- that counts invocations. Returns the total number of flush-callback
+-- invocations (should equal the number of rows inserted).
+--
+CREATE FUNCTION test_buffered_insert_with_callback(pg_catalog.regclass, pg_catalog.int4)
+ RETURNS pg_catalog.int4
+ AS 'MODULE_PATHNAME' LANGUAGE C;
+
+--
+-- Exercise explicit flush() mid-session: inserts half the rows, calls
+-- flush(), inserts the other half, then calls end(). Returns the total
+-- number of flush-callback invocations (should equal nrows).
+--
+CREATE FUNCTION test_buffered_insert_flush_mid(pg_catalog.regclass, pg_catalog.int4)
+ RETURNS pg_catalog.int4
+ AS 'MODULE_PATHNAME' LANGUAGE C;
diff --git a/src/test/modules/test_buffered_insert/test_buffered_insert.c b/src/test/modules/test_buffered_insert/test_buffered_insert.c
new file mode 100644
index 00000000000..fbda7b1c70b
--- /dev/null
+++ b/src/test/modules/test_buffered_insert/test_buffered_insert.c
@@ -0,0 +1,233 @@
+/*--------------------------------------------------------------------------
+ *
+ * test_buffered_insert.c
+ * Minimal validation for the Table AM buffered-insert lifecycle API.
+ *
+ * Exercises begin/put/flush/end directly at the Table AM layer on a
+ * real heap relation, without going through any higher-level caller
+ * (CTAS, COPY, etc.). This keeps the test within Patch 0001 scope.
+ *
+ * Portions Copyright (c) 1996-2026, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * IDENTIFICATION
+ * src/test/modules/test_buffered_insert/test_buffered_insert.c
+ *
+ * -------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include "access/tableam.h"
+#include "access/table.h"
+#include "access/xact.h"
+#include "catalog/namespace.h"
+#include "executor/tuptable.h"
+#include "fmgr.h"
+#include "utils/builtins.h"
+#include "utils/lsyscache.h"
+#include "utils/rel.h"
+
+PG_MODULE_MAGIC;
+
+/*
+ * test_buffered_insert_basic(regclass, int4)
+ *
+ * Opens the given relation, inserts nrows tuples through the buffered-insert
+ * API with a NULL flush callback, and returns the number of rows put().
+ * The tuples inserted have the form (i) for a single-integer-column table.
+ */
+PG_FUNCTION_INFO_V1(test_buffered_insert_basic);
+Datum
+test_buffered_insert_basic(PG_FUNCTION_ARGS)
+{
+ Oid relid = PG_GETARG_OID(0);
+ int32 nrows = PG_GETARG_INT32(1);
+ Relation rel;
+ TableBufferedInsertState state;
+ TupleTableSlot *slot;
+ TupleDesc tupdesc;
+ int i;
+
+ rel = table_open(relid, RowExclusiveLock);
+ tupdesc = RelationGetDescr(rel);
+
+ state = table_buffered_insert_begin(rel,
+ GetCurrentCommandId(true),
+ TABLE_INSERT_BAS_BULKWRITE,
+ NULL, NULL);
+
+ if (state == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("AM does not support buffered inserts")));
+
+ slot = MakeSingleTupleTableSlot(tupdesc, &TTSOpsVirtual);
+
+ for (i = 0; i < nrows; i++)
+ {
+ ExecClearTuple(slot);
+ slot->tts_values[0] = Int32GetDatum(i + 1);
+ slot->tts_isnull[0] = false;
+ ExecStoreVirtualTuple(slot);
+
+ table_buffered_insert_put(state, slot);
+ }
+
+ table_buffered_insert_end(state);
+
+ ExecDropSingleTupleTableSlot(slot);
+ table_close(rel, RowExclusiveLock);
+
+ PG_RETURN_INT32(nrows);
+}
+
+/*
+ * Flush callback context: counts invocations and verifies each slot has
+ * a valid TID.
+ */
+typedef struct FlushCbContext
+{
+ int count;
+} FlushCbContext;
+
+static void
+test_flush_callback(void *context, TupleTableSlot *slot)
+{
+ FlushCbContext *ctx = (FlushCbContext *) context;
+
+ if (!ItemPointerIsValid(&slot->tts_tid))
+ ereport(ERROR,
+ (errcode(ERRCODE_INTERNAL_ERROR),
+ errmsg("flush callback received slot with invalid TID")));
+
+ ctx->count++;
+}
+
+/*
+ * test_buffered_insert_with_callback(regclass, int4)
+ *
+ * Same as basic, but passes a flush callback that counts invocations and
+ * validates TIDs. Returns the total flush-callback invocation count.
+ */
+PG_FUNCTION_INFO_V1(test_buffered_insert_with_callback);
+Datum
+test_buffered_insert_with_callback(PG_FUNCTION_ARGS)
+{
+ Oid relid = PG_GETARG_OID(0);
+ int32 nrows = PG_GETARG_INT32(1);
+ Relation rel;
+ TableBufferedInsertState state;
+ TupleTableSlot *slot;
+ TupleDesc tupdesc;
+ FlushCbContext ctx;
+ int i;
+
+ ctx.count = 0;
+
+ rel = table_open(relid, RowExclusiveLock);
+ tupdesc = RelationGetDescr(rel);
+
+ state = table_buffered_insert_begin(rel,
+ GetCurrentCommandId(true),
+ TABLE_INSERT_BAS_BULKWRITE,
+ test_flush_callback,
+ &ctx);
+
+ if (state == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("AM does not support buffered inserts")));
+
+ slot = MakeSingleTupleTableSlot(tupdesc, &TTSOpsVirtual);
+
+ for (i = 0; i < nrows; i++)
+ {
+ ExecClearTuple(slot);
+ slot->tts_values[0] = Int32GetDatum(i + 1);
+ slot->tts_isnull[0] = false;
+ ExecStoreVirtualTuple(slot);
+
+ table_buffered_insert_put(state, slot);
+ }
+
+ table_buffered_insert_end(state);
+
+ ExecDropSingleTupleTableSlot(slot);
+ table_close(rel, RowExclusiveLock);
+
+ PG_RETURN_INT32(ctx.count);
+}
+
+/*
+ * test_buffered_insert_flush_mid(regclass, int4)
+ *
+ * Exercises explicit flush() mid-session: inserts half the rows, calls
+ * flush(), inserts the other half, then calls end(). Returns the total
+ * flush-callback count, which should equal nrows.
+ */
+PG_FUNCTION_INFO_V1(test_buffered_insert_flush_mid);
+Datum
+test_buffered_insert_flush_mid(PG_FUNCTION_ARGS)
+{
+ Oid relid = PG_GETARG_OID(0);
+ int32 nrows = PG_GETARG_INT32(1);
+ Relation rel;
+ TableBufferedInsertState state;
+ TupleTableSlot *slot;
+ TupleDesc tupdesc;
+ FlushCbContext ctx;
+ int half = nrows / 2;
+ int i;
+
+ ctx.count = 0;
+
+ rel = table_open(relid, RowExclusiveLock);
+ tupdesc = RelationGetDescr(rel);
+
+ state = table_buffered_insert_begin(rel,
+ GetCurrentCommandId(true),
+ TABLE_INSERT_BAS_BULKWRITE,
+ test_flush_callback,
+ &ctx);
+
+ if (state == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("AM does not support buffered inserts")));
+
+ slot = MakeSingleTupleTableSlot(tupdesc, &TTSOpsVirtual);
+
+ /* First half */
+ for (i = 0; i < half; i++)
+ {
+ ExecClearTuple(slot);
+ slot->tts_values[0] = Int32GetDatum(i + 1);
+ slot->tts_isnull[0] = false;
+ ExecStoreVirtualTuple(slot);
+
+ table_buffered_insert_put(state, slot);
+ }
+
+ /* Explicit flush mid-session */
+ table_buffered_insert_flush(state);
+
+ /* Second half */
+ for (i = half; i < nrows; i++)
+ {
+ ExecClearTuple(slot);
+ slot->tts_values[0] = Int32GetDatum(i + 1);
+ slot->tts_isnull[0] = false;
+ ExecStoreVirtualTuple(slot);
+
+ table_buffered_insert_put(state, slot);
+ }
+
+ /* end() flushes the remaining tuples */
+ table_buffered_insert_end(state);
+
+ ExecDropSingleTupleTableSlot(slot);
+ table_close(rel, RowExclusiveLock);
+
+ PG_RETURN_INT32(ctx.count);
+}
diff --git a/src/test/modules/test_buffered_insert/test_buffered_insert.control b/src/test/modules/test_buffered_insert/test_buffered_insert.control
new file mode 100644
index 00000000000..3221c765820
--- /dev/null
+++ b/src/test/modules/test_buffered_insert/test_buffered_insert.control
@@ -0,0 +1,4 @@
+comment = 'Test code for Table AM buffered-insert lifecycle'
+default_version = '1.0'
+module_pathname = '$libdir/test_buffered_insert'
+relocatable = true
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 9f1dd55213d..257aee1e684 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1255,6 +1255,7 @@ HeadlineWordEntry
HeapCheckContext
HeapCheckReadStreamData
HeapPageFreeze
+HeapBufferedInsertState
HeapScanDesc
HeapScanDescData
HeapTuple
@@ -3123,6 +3124,9 @@ T_Action
T_WorkerStatus
TableAmRoutine
TableAttachInfo
+TableBufferedInsertFlushCb
+TableBufferedInsertState
+TableBufferedInsertStateData
TableDataInfo
TableFunc
TableFuncRoutine
--
2.52.0
[application/octet-stream] v1-0005-executor-adopt-buffered-insert-API-for-restricted.patch (24.3K, ../../CABXr29ExTDUoWZsiA0w+mkrjkJWKhPdiseM7vHJgHY-Mwq9PNg@mail.gmail.com/4-v1-0005-executor-adopt-buffered-insert-API-for-restricted.patch)
download | inline diff:
From f2a38d918a0cb3f4279c75f399c82eedb37c909d Mon Sep 17 00:00:00 2001
From: Haibo Yan <[email protected]>
Date: Sun, 26 Apr 2026 22:24:20 -0700
Subject: [PATCH v1 5/5] executor: adopt buffered-insert API for restricted
INSERT ... SELECT
Add a restricted first-step buffered-insert adoption for
`INSERT INTO ... SELECT` in the executor.
This patch integrates the existing buffered-insert lifecycle API into the
plain non-partitioned heap `CMD_INSERT` path in `ExecInsert()` /
`ExecModifyTable()`, using a local flush callback for post-insert work.
The buffered path is intentionally narrow. It falls back to the existing
single-row path for cases outside the restricted scope, including:
- ON CONFLICT
- RETURNING
- partitioned targets
- BEFORE ROW / INSTEAD OF triggers
- FDW targets
- MERGE / cross-partition UPDATE insert side
- volatile target-side default expressions
The flush callback performs the minimal post-insert executor work needed
for this first step:
- index maintenance via `ExecInsertIndexTuples()`
- AFTER ROW trigger firing via `ExecARInsertTriggers()`
- `es_processed` accounting
Add focused regression coverage for:
- basic bulk INSERT ... SELECT
- indexed targets
- AFTER ROW trigger behavior
- index + trigger combination
- fallback cases (ON CONFLICT, RETURNING, BEFORE ROW trigger,
partitioned target, volatile target defaults)
- zero-row insert
---
src/backend/executor/nodeModifyTable.c | 164 +++++++++++
src/include/nodes/execnodes.h | 5 +
src/test/regress/expected/insert_buffered.out | 271 ++++++++++++++++++
src/test/regress/parallel_schedule | 2 +-
src/test/regress/sql/insert_buffered.sql | 209 ++++++++++++++
5 files changed, 650 insertions(+), 1 deletion(-)
create mode 100644 src/test/regress/expected/insert_buffered.out
create mode 100644 src/test/regress/sql/insert_buffered.sql
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 4cb057ca4f9..06af0637407 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -83,6 +83,85 @@ typedef struct MTTargetRelLookup
int relationIndex; /* rel's index in resultRelInfo[] array */
} MTTargetRelLookup;
+/*
+ * Flush callback context for buffered INSERT INTO ... SELECT.
+ *
+ * These are the parameters required by the three callback operations
+ * (ExecInsertIndexTuples, ExecARInsertTriggers, es_processed++):
+ *
+ * - estate, resultRelInfo: required by ExecInsertIndexTuples/ExecARInsertTriggers.
+ * - transition_capture: 5th parameter of ExecARInsertTriggers; the non-buffered
+ * path passes mtstate->mt_transition_capture for CMD_INSERT. NULL would
+ * silently break statement-level triggers with REFERENCING NEW TABLE.
+ * - canSetTag: the non-buffered path gates es_processed++ on this after the
+ * insert; the buffered return-NULL bypasses that gate, so the callback
+ * replicates it.
+ */
+typedef struct ExecBufferedInsertFlushState
+{
+ EState *estate;
+ ResultRelInfo *resultRelInfo;
+ TransitionCaptureState *transition_capture;
+ bool canSetTag;
+} ExecBufferedInsertFlushState;
+
+/*
+ * Flush callback for buffered INSERT INTO ... SELECT.
+ *
+ * Called once per flushed tuple after heap_multi_insert() completes a batch.
+ * Performs index maintenance, AFTER ROW trigger firing, and tuple counting.
+ */
+static void
+ExecBufferedInsertFlushCb(void *context, TupleTableSlot *slot)
+{
+ ExecBufferedInsertFlushState *ctx = (ExecBufferedInsertFlushState *) context;
+ ResultRelInfo *resultRelInfo = ctx->resultRelInfo;
+ EState *estate = ctx->estate;
+ List *recheckIndexes = NIL;
+
+ if (resultRelInfo->ri_NumIndices > 0)
+ recheckIndexes = ExecInsertIndexTuples(resultRelInfo, estate, 0,
+ slot, NIL, NULL);
+
+ ExecARInsertTriggers(estate, resultRelInfo, slot, recheckIndexes,
+ ctx->transition_capture);
+
+ list_free(recheckIndexes);
+
+ if (ctx->canSetTag)
+ (estate->es_processed)++;
+}
+
+/*
+ * Check whether a relation has volatile default expressions.
+ *
+ * Conservative target-side restriction: if any column default contains a
+ * volatile function (excluding nextval), the buffered-insert path is not used.
+ * This mirrors COPY FROM's volatile_defexprs check.
+ */
+static bool
+ExecRelHasVolatileDefaults(Relation rel)
+{
+ TupleConstr *constr = RelationGetDescr(rel)->constr;
+
+ if (constr == NULL || constr->num_defval == 0)
+ return false;
+
+ for (int i = 0; i < constr->num_defval; i++)
+ {
+ Node *expr;
+
+ if (constr->defval[i].adbin == NULL)
+ continue;
+
+ expr = stringToNode(constr->defval[i].adbin);
+
+ if (contain_volatile_functions_not_nextval(expr))
+ return true;
+ }
+ return false;
+}
+
/*
* Context struct for a ModifyTable operation, containing basic execution
* state and some output variables populated by ExecUpdateAct() and
@@ -1269,6 +1348,49 @@ ExecInsert(ModifyTableContext *context,
}
else
{
+ /*
+ * Buffered-insert path: lazily open the session on first call,
+ * then submit tuples via put() instead of single-row insert.
+ * Post-insert work (indexes, triggers) fires in the flush callback.
+ */
+ if (mtstate->mt_buffered_insert_eligible &&
+ mtstate->mt_bi_state == NULL)
+ {
+ ExecBufferedInsertFlushState *flush_ctx;
+
+ flush_ctx = palloc(sizeof(ExecBufferedInsertFlushState));
+ flush_ctx->estate = estate;
+ flush_ctx->resultRelInfo = resultRelInfo;
+ flush_ctx->transition_capture = mtstate->mt_transition_capture;
+ flush_ctx->canSetTag = canSetTag;
+ mtstate->mt_bi_flush_ctx = flush_ctx;
+
+ mtstate->mt_bi_state =
+ table_buffered_insert_begin(resultRelationDesc,
+ estate->es_output_cid,
+ TABLE_INSERT_BAS_BULKWRITE,
+ ExecBufferedInsertFlushCb,
+ flush_ctx);
+ if (mtstate->mt_bi_state == NULL)
+ mtstate->mt_buffered_insert_eligible = false;
+ }
+
+ if (mtstate->mt_bi_state != NULL)
+ {
+ /*
+ * Pre-insert validation already ran in this else-branch
+ * above the ON CONFLICT test — specifically:
+ * tts_tableOid init, ExecComputeStoredGenerated,
+ * ExecWithCheckOptions (RLS), ExecConstraints,
+ * ExecPartitionCheck.
+ * This inner else-branch (no ON CONFLICT) is reached only
+ * after all of those. Submit the validated tuple to the
+ * AM buffer; post-insert work fires in the flush callback.
+ */
+ table_buffered_insert_put(mtstate->mt_bi_state, slot);
+ return NULL;
+ }
+
/* insert the tuple normally */
table_tuple_insert(resultRelationDesc, slot,
estate->es_output_cid,
@@ -5027,6 +5149,15 @@ ExecModifyTable(PlanState *pstate)
if (estate->es_insert_pending_result_relations != NIL)
ExecPendingInserts(estate);
+ /*
+ * Flush and clean up buffered-insert session if active.
+ */
+ if (node->mt_bi_state != NULL)
+ {
+ table_buffered_insert_end(node->mt_bi_state);
+ node->mt_bi_state = NULL;
+ }
+
/*
* We're done, but fire AFTER STATEMENT triggers before exiting.
*/
@@ -5773,6 +5904,30 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
resultRelInfo->ri_BatchSize = 1;
}
+ /*
+ * For CMD_INSERT without ON CONFLICT/RETURNING/partitioning/BEFORE ROW
+ * triggers, determine if the restricted buffered-insert path is eligible.
+ * AM support is resolved lazily at first ExecInsert() call.
+ */
+ if (operation == CMD_INSERT)
+ {
+ ModifyTable *mtnode = (ModifyTable *) mtstate->ps.plan;
+
+ resultRelInfo = mtstate->resultRelInfo;
+ mtstate->mt_buffered_insert_eligible =
+ (mtnode->onConflictAction == ONCONFLICT_NONE &&
+ resultRelInfo->ri_projectReturning == NULL &&
+ resultRelInfo->ri_RelationDesc->rd_rel->relkind !=
+ RELKIND_PARTITIONED_TABLE &&
+ !(resultRelInfo->ri_TrigDesc &&
+ resultRelInfo->ri_TrigDesc->trig_insert_before_row) &&
+ !(resultRelInfo->ri_TrigDesc &&
+ resultRelInfo->ri_TrigDesc->trig_insert_instead_row) &&
+ resultRelInfo->ri_FdwRoutine == NULL &&
+ mtstate->operation == CMD_INSERT &&
+ !ExecRelHasVolatileDefaults(resultRelInfo->ri_RelationDesc));
+ }
+
/*
* Lastly, if this is not the primary (canSetTag) ModifyTable node, add it
* to estate->es_auxmodifytables so that it will be run to completion by
@@ -5802,6 +5957,15 @@ ExecEndModifyTable(ModifyTableState *node)
{
int i;
+ /*
+ * Defensive: clean up buffered-insert if end() was not reached above.
+ */
+ if (node->mt_bi_state != NULL)
+ {
+ table_buffered_insert_end(node->mt_bi_state);
+ node->mt_bi_state = NULL;
+ }
+
/*
* Allow any FDWs to shut down
*/
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 13359180d25..7790bb0ba38 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1508,6 +1508,11 @@ typedef struct ModifyTableState
List *mt_updateColnosLists;
List *mt_mergeActionLists;
List *mt_mergeJoinConditions;
+
+ /* Buffered-insert state for restricted INSERT INTO ... SELECT */
+ bool mt_buffered_insert_eligible;
+ struct TableBufferedInsertStateData *mt_bi_state;
+ void *mt_bi_flush_ctx; /* private to nodeModifyTable.c */
} ModifyTableState;
/* ----------------
diff --git a/src/test/regress/expected/insert_buffered.out b/src/test/regress/expected/insert_buffered.out
new file mode 100644
index 00000000000..3ee43ae8b04
--- /dev/null
+++ b/src/test/regress/expected/insert_buffered.out
@@ -0,0 +1,271 @@
+--
+-- Tests for buffered-insert adoption in INSERT INTO ... SELECT (Patch 0005).
+-- Restricted first step: non-partitioned heap target, no ON CONFLICT,
+-- no RETURNING, no BEFORE ROW triggers.
+--
+-- ============================================================
+-- T1: Basic bulk insert (exercises multiple auto-flush cycles)
+-- ============================================================
+CREATE TABLE bi_target_basic (id int, val text);
+INSERT INTO bi_target_basic
+SELECT g, 'row-' || g FROM generate_series(1, 2000) g;
+SELECT count(*) FROM bi_target_basic;
+ count
+-------
+ 2000
+(1 row)
+
+SELECT min(id), max(id) FROM bi_target_basic;
+ min | max
+-----+------
+ 1 | 2000
+(1 row)
+
+DROP TABLE bi_target_basic;
+-- ============================================================
+-- T2: Indexed target
+-- ============================================================
+CREATE TABLE bi_target_idx (id int, val text);
+CREATE INDEX bi_target_idx_id ON bi_target_idx (id);
+INSERT INTO bi_target_idx
+SELECT g, 'row-' || g FROM generate_series(1, 500) g;
+SELECT count(*) FROM bi_target_idx;
+ count
+-------
+ 500
+(1 row)
+
+-- Verify index is usable and correct
+SET enable_seqscan = off;
+SELECT count(*) FROM bi_target_idx WHERE id BETWEEN 1 AND 500;
+ count
+-------
+ 500
+(1 row)
+
+RESET enable_seqscan;
+DROP TABLE bi_target_idx;
+-- ============================================================
+-- T3: AFTER ROW trigger
+-- ============================================================
+CREATE TABLE bi_target_trig (id int, val text);
+CREATE TABLE bi_audit (id int, val text, logged_at timestamp DEFAULT now());
+CREATE FUNCTION bi_audit_fn() RETURNS trigger
+LANGUAGE plpgsql AS $$
+BEGIN
+ INSERT INTO bi_audit (id, val) VALUES (NEW.id, NEW.val);
+ RETURN NEW;
+END;
+$$;
+CREATE TRIGGER bi_target_trig_after
+ AFTER INSERT ON bi_target_trig
+ FOR EACH ROW EXECUTE FUNCTION bi_audit_fn();
+INSERT INTO bi_target_trig
+SELECT g, 'row-' || g FROM generate_series(1, 50) g;
+SELECT count(*) FROM bi_target_trig;
+ count
+-------
+ 50
+(1 row)
+
+SELECT count(*) FROM bi_audit;
+ count
+-------
+ 50
+(1 row)
+
+-- Verify insertion order is preserved
+SELECT bool_and(t.id = a.id) AS order_preserved
+FROM (SELECT id, row_number() OVER (ORDER BY ctid) AS rn FROM bi_target_trig) t
+JOIN (SELECT id, row_number() OVER (ORDER BY ctid) AS rn FROM bi_audit) a
+ON t.rn = a.rn;
+ order_preserved
+-----------------
+ t
+(1 row)
+
+DROP TABLE bi_target_trig CASCADE;
+DROP TABLE bi_audit;
+DROP FUNCTION bi_audit_fn;
+-- ============================================================
+-- T4: Index + AFTER ROW trigger combined
+-- ============================================================
+CREATE TABLE bi_target_combo (id int, val text);
+CREATE INDEX bi_target_combo_id ON bi_target_combo (id);
+CREATE TABLE bi_audit_combo (id int, val text);
+CREATE FUNCTION bi_audit_combo_fn() RETURNS trigger
+LANGUAGE plpgsql AS $$
+BEGIN
+ INSERT INTO bi_audit_combo (id, val) VALUES (NEW.id, NEW.val);
+ RETURN NEW;
+END;
+$$;
+CREATE TRIGGER bi_target_combo_after
+ AFTER INSERT ON bi_target_combo
+ FOR EACH ROW EXECUTE FUNCTION bi_audit_combo_fn();
+INSERT INTO bi_target_combo
+SELECT g, 'row-' || g FROM generate_series(1, 100) g;
+SELECT count(*) FROM bi_target_combo;
+ count
+-------
+ 100
+(1 row)
+
+SELECT count(*) FROM bi_audit_combo;
+ count
+-------
+ 100
+(1 row)
+
+-- Verify index correctness
+SET enable_seqscan = off;
+SELECT count(*) FROM bi_target_combo WHERE id BETWEEN 1 AND 100;
+ count
+-------
+ 100
+(1 row)
+
+RESET enable_seqscan;
+DROP TABLE bi_target_combo CASCADE;
+DROP TABLE bi_audit_combo;
+DROP FUNCTION bi_audit_combo_fn;
+-- ============================================================
+-- T5: ON CONFLICT fallback (uses non-buffered path)
+-- ============================================================
+CREATE TABLE bi_target_conflict (id int PRIMARY KEY, val text);
+INSERT INTO bi_target_conflict VALUES (1, 'existing');
+INSERT INTO bi_target_conflict
+SELECT g, 'row-' || g FROM generate_series(1, 10) g
+ON CONFLICT (id) DO NOTHING;
+SELECT count(*) FROM bi_target_conflict;
+ count
+-------
+ 10
+(1 row)
+
+SELECT val FROM bi_target_conflict WHERE id = 1;
+ val
+----------
+ existing
+(1 row)
+
+DROP TABLE bi_target_conflict;
+-- ============================================================
+-- T6: RETURNING fallback (uses non-buffered path)
+-- ============================================================
+CREATE TABLE bi_target_ret (id int, val text);
+INSERT INTO bi_target_ret
+SELECT g, 'row-' || g FROM generate_series(1, 3) g
+RETURNING id, val;
+ id | val
+----+-------
+ 1 | row-1
+ 2 | row-2
+ 3 | row-3
+(3 rows)
+
+SELECT count(*) FROM bi_target_ret;
+ count
+-------
+ 3
+(1 row)
+
+DROP TABLE bi_target_ret;
+-- ============================================================
+-- T7: BEFORE ROW trigger fallback (uses non-buffered path)
+-- ============================================================
+CREATE TABLE bi_target_br (id int, val text);
+CREATE FUNCTION bi_br_fn() RETURNS trigger
+LANGUAGE plpgsql AS $$
+BEGIN
+ NEW.val := NEW.val || '-modified';
+ RETURN NEW;
+END;
+$$;
+CREATE TRIGGER bi_target_br_before
+ BEFORE INSERT ON bi_target_br
+ FOR EACH ROW EXECUTE FUNCTION bi_br_fn();
+INSERT INTO bi_target_br
+SELECT g, 'row-' || g FROM generate_series(1, 5) g;
+SELECT count(*) FROM bi_target_br;
+ count
+-------
+ 5
+(1 row)
+
+SELECT val FROM bi_target_br WHERE id = 1;
+ val
+----------------
+ row-1-modified
+(1 row)
+
+DROP TABLE bi_target_br;
+DROP FUNCTION bi_br_fn;
+-- ============================================================
+-- T8: Partitioned target fallback (uses non-buffered path)
+-- ============================================================
+CREATE TABLE bi_target_part (id int, val text) PARTITION BY RANGE (id);
+CREATE TABLE bi_target_part_1 PARTITION OF bi_target_part FOR VALUES FROM (1) TO (501);
+CREATE TABLE bi_target_part_2 PARTITION OF bi_target_part FOR VALUES FROM (501) TO (1001);
+INSERT INTO bi_target_part
+SELECT g, 'row-' || g FROM generate_series(1, 1000) g;
+SELECT count(*) FROM bi_target_part;
+ count
+-------
+ 1000
+(1 row)
+
+SELECT count(*) FROM bi_target_part_1;
+ count
+-------
+ 500
+(1 row)
+
+SELECT count(*) FROM bi_target_part_2;
+ count
+-------
+ 500
+(1 row)
+
+DROP TABLE bi_target_part;
+-- ============================================================
+-- T9: Volatile target-default fallback
+-- Expected to fall back to non-buffered path under E8.
+-- Test validates correctness; path selection is not observable
+-- from SQL output.
+-- ============================================================
+CREATE TABLE bi_target_volatile (
+ id int,
+ val text,
+ rand_val double precision DEFAULT random()
+);
+INSERT INTO bi_target_volatile (id, val)
+SELECT g, 'row-' || g FROM generate_series(1, 5) g;
+SELECT count(*) FROM bi_target_volatile;
+ count
+-------
+ 5
+(1 row)
+
+-- Verify the volatile default was evaluated (all values should be distinct)
+SELECT count(DISTINCT rand_val) = count(*) AS all_distinct
+FROM bi_target_volatile;
+ all_distinct
+--------------
+ t
+(1 row)
+
+DROP TABLE bi_target_volatile;
+-- ============================================================
+-- T10: Zero-row insert
+-- ============================================================
+CREATE TABLE bi_target_zero (id int, val text);
+INSERT INTO bi_target_zero
+SELECT g, 'row-' || g FROM generate_series(1, 100) g WHERE false;
+SELECT count(*) FROM bi_target_zero;
+ count
+-------
+ 0
+(1 row)
+
+DROP TABLE bi_target_zero;
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index 5d4f910155e..ae63c3dd73c 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -36,7 +36,7 @@ test: geometry horology tstypes regex type_sanity opr_sanity misc_sanity comment
# execute two copy tests in parallel, to check that copy itself
# is concurrent safe.
# ----------
-test: copy copyselect copydml copyencoding insert insert_conflict
+test: copy copyselect copydml copyencoding insert insert_conflict insert_buffered
# ----------
# More groups of parallel tests
diff --git a/src/test/regress/sql/insert_buffered.sql b/src/test/regress/sql/insert_buffered.sql
new file mode 100644
index 00000000000..2ca6f3525f4
--- /dev/null
+++ b/src/test/regress/sql/insert_buffered.sql
@@ -0,0 +1,209 @@
+--
+-- Tests for buffered-insert adoption in INSERT INTO ... SELECT (Patch 0005).
+-- Restricted first step: non-partitioned heap target, no ON CONFLICT,
+-- no RETURNING, no BEFORE ROW triggers.
+--
+
+-- ============================================================
+-- T1: Basic bulk insert (exercises multiple auto-flush cycles)
+-- ============================================================
+CREATE TABLE bi_target_basic (id int, val text);
+
+INSERT INTO bi_target_basic
+SELECT g, 'row-' || g FROM generate_series(1, 2000) g;
+
+SELECT count(*) FROM bi_target_basic;
+SELECT min(id), max(id) FROM bi_target_basic;
+
+DROP TABLE bi_target_basic;
+
+-- ============================================================
+-- T2: Indexed target
+-- ============================================================
+CREATE TABLE bi_target_idx (id int, val text);
+CREATE INDEX bi_target_idx_id ON bi_target_idx (id);
+
+INSERT INTO bi_target_idx
+SELECT g, 'row-' || g FROM generate_series(1, 500) g;
+
+SELECT count(*) FROM bi_target_idx;
+
+-- Verify index is usable and correct
+SET enable_seqscan = off;
+SELECT count(*) FROM bi_target_idx WHERE id BETWEEN 1 AND 500;
+RESET enable_seqscan;
+
+DROP TABLE bi_target_idx;
+
+-- ============================================================
+-- T3: AFTER ROW trigger
+-- ============================================================
+CREATE TABLE bi_target_trig (id int, val text);
+CREATE TABLE bi_audit (id int, val text, logged_at timestamp DEFAULT now());
+
+CREATE FUNCTION bi_audit_fn() RETURNS trigger
+LANGUAGE plpgsql AS $$
+BEGIN
+ INSERT INTO bi_audit (id, val) VALUES (NEW.id, NEW.val);
+ RETURN NEW;
+END;
+$$;
+
+CREATE TRIGGER bi_target_trig_after
+ AFTER INSERT ON bi_target_trig
+ FOR EACH ROW EXECUTE FUNCTION bi_audit_fn();
+
+INSERT INTO bi_target_trig
+SELECT g, 'row-' || g FROM generate_series(1, 50) g;
+
+SELECT count(*) FROM bi_target_trig;
+SELECT count(*) FROM bi_audit;
+
+-- Verify insertion order is preserved
+SELECT bool_and(t.id = a.id) AS order_preserved
+FROM (SELECT id, row_number() OVER (ORDER BY ctid) AS rn FROM bi_target_trig) t
+JOIN (SELECT id, row_number() OVER (ORDER BY ctid) AS rn FROM bi_audit) a
+ON t.rn = a.rn;
+
+DROP TABLE bi_target_trig CASCADE;
+DROP TABLE bi_audit;
+DROP FUNCTION bi_audit_fn;
+
+-- ============================================================
+-- T4: Index + AFTER ROW trigger combined
+-- ============================================================
+CREATE TABLE bi_target_combo (id int, val text);
+CREATE INDEX bi_target_combo_id ON bi_target_combo (id);
+CREATE TABLE bi_audit_combo (id int, val text);
+
+CREATE FUNCTION bi_audit_combo_fn() RETURNS trigger
+LANGUAGE plpgsql AS $$
+BEGIN
+ INSERT INTO bi_audit_combo (id, val) VALUES (NEW.id, NEW.val);
+ RETURN NEW;
+END;
+$$;
+
+CREATE TRIGGER bi_target_combo_after
+ AFTER INSERT ON bi_target_combo
+ FOR EACH ROW EXECUTE FUNCTION bi_audit_combo_fn();
+
+INSERT INTO bi_target_combo
+SELECT g, 'row-' || g FROM generate_series(1, 100) g;
+
+SELECT count(*) FROM bi_target_combo;
+SELECT count(*) FROM bi_audit_combo;
+
+-- Verify index correctness
+SET enable_seqscan = off;
+SELECT count(*) FROM bi_target_combo WHERE id BETWEEN 1 AND 100;
+RESET enable_seqscan;
+
+DROP TABLE bi_target_combo CASCADE;
+DROP TABLE bi_audit_combo;
+DROP FUNCTION bi_audit_combo_fn;
+
+-- ============================================================
+-- T5: ON CONFLICT fallback (uses non-buffered path)
+-- ============================================================
+CREATE TABLE bi_target_conflict (id int PRIMARY KEY, val text);
+
+INSERT INTO bi_target_conflict VALUES (1, 'existing');
+
+INSERT INTO bi_target_conflict
+SELECT g, 'row-' || g FROM generate_series(1, 10) g
+ON CONFLICT (id) DO NOTHING;
+
+SELECT count(*) FROM bi_target_conflict;
+SELECT val FROM bi_target_conflict WHERE id = 1;
+
+DROP TABLE bi_target_conflict;
+
+-- ============================================================
+-- T6: RETURNING fallback (uses non-buffered path)
+-- ============================================================
+CREATE TABLE bi_target_ret (id int, val text);
+
+INSERT INTO bi_target_ret
+SELECT g, 'row-' || g FROM generate_series(1, 3) g
+RETURNING id, val;
+
+SELECT count(*) FROM bi_target_ret;
+
+DROP TABLE bi_target_ret;
+
+-- ============================================================
+-- T7: BEFORE ROW trigger fallback (uses non-buffered path)
+-- ============================================================
+CREATE TABLE bi_target_br (id int, val text);
+
+CREATE FUNCTION bi_br_fn() RETURNS trigger
+LANGUAGE plpgsql AS $$
+BEGIN
+ NEW.val := NEW.val || '-modified';
+ RETURN NEW;
+END;
+$$;
+
+CREATE TRIGGER bi_target_br_before
+ BEFORE INSERT ON bi_target_br
+ FOR EACH ROW EXECUTE FUNCTION bi_br_fn();
+
+INSERT INTO bi_target_br
+SELECT g, 'row-' || g FROM generate_series(1, 5) g;
+
+SELECT count(*) FROM bi_target_br;
+SELECT val FROM bi_target_br WHERE id = 1;
+
+DROP TABLE bi_target_br;
+DROP FUNCTION bi_br_fn;
+
+-- ============================================================
+-- T8: Partitioned target fallback (uses non-buffered path)
+-- ============================================================
+CREATE TABLE bi_target_part (id int, val text) PARTITION BY RANGE (id);
+CREATE TABLE bi_target_part_1 PARTITION OF bi_target_part FOR VALUES FROM (1) TO (501);
+CREATE TABLE bi_target_part_2 PARTITION OF bi_target_part FOR VALUES FROM (501) TO (1001);
+
+INSERT INTO bi_target_part
+SELECT g, 'row-' || g FROM generate_series(1, 1000) g;
+
+SELECT count(*) FROM bi_target_part;
+SELECT count(*) FROM bi_target_part_1;
+SELECT count(*) FROM bi_target_part_2;
+
+DROP TABLE bi_target_part;
+
+-- ============================================================
+-- T9: Volatile target-default fallback
+-- Expected to fall back to non-buffered path under E8.
+-- Test validates correctness; path selection is not observable
+-- from SQL output.
+-- ============================================================
+CREATE TABLE bi_target_volatile (
+ id int,
+ val text,
+ rand_val double precision DEFAULT random()
+);
+
+INSERT INTO bi_target_volatile (id, val)
+SELECT g, 'row-' || g FROM generate_series(1, 5) g;
+
+SELECT count(*) FROM bi_target_volatile;
+-- Verify the volatile default was evaluated (all values should be distinct)
+SELECT count(DISTINCT rand_val) = count(*) AS all_distinct
+FROM bi_target_volatile;
+
+DROP TABLE bi_target_volatile;
+
+-- ============================================================
+-- T10: Zero-row insert
+-- ============================================================
+CREATE TABLE bi_target_zero (id int, val text);
+
+INSERT INTO bi_target_zero
+SELECT g, 'row-' || g FROM generate_series(1, 100) g WHERE false;
+
+SELECT count(*) FROM bi_target_zero;
+
+DROP TABLE bi_target_zero;
--
2.52.0
[application/octet-stream] v1-0002-createas-use-buffered-insert-API-for-CTAS.patch (11.6K, ../../CABXr29ExTDUoWZsiA0w+mkrjkJWKhPdiseM7vHJgHY-Mwq9PNg@mail.gmail.com/5-v1-0002-createas-use-buffered-insert-API-for-CTAS.patch)
download | inline diff:
From d6b6cb19c326d79693c2e6238cfbe96f5b0fa0e0 Mon Sep 17 00:00:00 2001
From: Haibo Yan <[email protected]>
Date: Thu, 23 Apr 2026 12:20:59 -0700
Subject: [PATCH v1 2/5] createas: use buffered-insert API for CTAS
Adopt the buffered-insert lifecycle API in the CTAS dest receiver.
CTAS uses table_buffered_insert_begin()/put()/end() with a NULL flush
callback and falls back to the existing single-row path when needed.
Retain the volatile-function check and EXECUTE-path fallback
conservatively for the initial patch series.
Add focused CTAS regression tests.
---
src/backend/commands/createas.c | 112 ++++++++++++++++++----
src/test/regress/expected/select_into.out | 98 +++++++++++++++++++
src/test/regress/sql/select_into.sql | 44 +++++++++
3 files changed, 234 insertions(+), 20 deletions(-)
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 6dbb831ca89..4ba25c9e336 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -40,6 +40,7 @@
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
#include "nodes/queryjumble.h"
+#include "optimizer/optimizer.h"
#include "parser/analyze.h"
#include "rewrite/rewriteHandler.h"
#include "tcop/tcopprot.h"
@@ -57,7 +58,9 @@ typedef struct
ObjectAddress reladdr; /* address of rel, for ExecCreateTableAs */
CommandId output_cid; /* cmin to insert in output tuples */
uint32 ti_options; /* table_tuple_insert performance options */
- BulkInsertState bistate; /* bulk insert state */
+ BulkInsertState bistate; /* bulk insert state (fallback path only) */
+ TableBufferedInsertState buffered_state; /* buffered-insert state, or NULL */
+ bool use_buffered_insert; /* true if buffered path is eligible */
} DR_intorel;
/* utility functions for CTAS definition creation */
@@ -260,7 +263,15 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
Assert(!is_matview); /* excluded by syntax */
ExecuteQuery(pstate, estmt, into, params, dest, qc);
- /* get object address that intorel_startup saved for us */
+ /*
+ * get object address that intorel_startup saved for us.
+ *
+ * Note: use_buffered_insert stays false (its palloc0 default) for the
+ * EXECUTE path. We conservatively skip the buffered-insert
+ * optimization here because the prepared statement's plan is not
+ * available for volatile-function inspection at this point. Relaxing
+ * this is outside Patch 0002 scope.
+ */
address = ((DR_intorel *) dest)->reladdr;
return address;
@@ -323,6 +334,24 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
plan = pg_plan_query(query, pstate->p_sourcetext,
CURSOR_OPT_PARALLEL_OK, params, NULL);
+ /*
+ * Conservative implementation choice: disable the buffered-insert
+ * path if the planned target list contains volatile functions.
+ *
+ * The buffered-insert API contract does not require this check — the
+ * CTAS target table is created within this statement and cannot be
+ * referenced by the source query. This guard is retained for the
+ * initial patch series as a caller-local conservatism and can be
+ * relaxed after validation without any API change.
+ */
+ {
+ DR_intorel *myState = (DR_intorel *) dest;
+
+ myState->use_buffered_insert =
+ !contain_volatile_functions_after_planning(
+ (Expr *) plan->planTree->targetlist);
+ }
+
/*
* Use a snapshot with an updated command ID to ensure this query sees
* results of any previously executed queries. (This could only
@@ -564,10 +593,45 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
* If WITH NO DATA is specified, there is no need to set up the state for
* bulk inserts as there are no tuples to insert.
*/
- if (!into->skipData)
- myState->bistate = GetBulkInsertState();
- else
+ if (into->skipData)
+ {
myState->bistate = NULL;
+ myState->buffered_state = NULL;
+ }
+ else if (myState->use_buffered_insert)
+ {
+ /*
+ * Try the buffered-insert path. Pass NULL flush callback -- CTAS
+ * has no indexes, triggers, or per-tuple post-insert work.
+ */
+ myState->ti_options |= TABLE_INSERT_BAS_BULKWRITE;
+ myState->buffered_state =
+ table_buffered_insert_begin(intoRelationDesc,
+ myState->output_cid,
+ myState->ti_options,
+ NULL, NULL);
+
+ if (myState->buffered_state != NULL)
+ {
+ /* Buffered path active; bistate is managed inside the AM */
+ myState->bistate = NULL;
+ }
+ else
+ {
+ /* AM does not support buffered inserts; fall back */
+ myState->bistate = GetBulkInsertState();
+ }
+ }
+ else
+ {
+ /*
+ * Fallback to single-row path. This is reached when the
+ * volatile-function conservative guard fired, or for CTAS via
+ * EXECUTE (where use_buffered_insert stays false by default).
+ */
+ myState->buffered_state = NULL;
+ myState->bistate = GetBulkInsertState();
+ }
/*
* Valid smgr_targblock implies something already wrote to the relation.
@@ -587,19 +651,18 @@ intorel_receive(TupleTableSlot *slot, DestReceiver *self)
/* Nothing to insert if WITH NO DATA is specified. */
if (!myState->into->skipData)
{
- /*
- * Note that the input slot might not be of the type of the target
- * relation. That's supported by table_tuple_insert(), but slightly
- * less efficient than inserting with the right slot - but the
- * alternative would be to copy into a slot of the right type, which
- * would not be cheap either. This also doesn't allow accessing per-AM
- * data (say a tuple's xmin), but since we don't do that here...
- */
- table_tuple_insert(myState->rel,
- slot,
- myState->output_cid,
- myState->ti_options,
- myState->bistate);
+ if (myState->buffered_state != NULL)
+ {
+ table_buffered_insert_put(myState->buffered_state, slot);
+ }
+ else
+ {
+ table_tuple_insert(myState->rel,
+ slot,
+ myState->output_cid,
+ myState->ti_options,
+ myState->bistate);
+ }
}
/* We know this is a newly created relation, so there are no indexes */
@@ -618,8 +681,17 @@ intorel_shutdown(DestReceiver *self)
if (!into->skipData)
{
- FreeBulkInsertState(myState->bistate);
- table_finish_bulk_insert(myState->rel, myState->ti_options);
+ if (myState->buffered_state != NULL)
+ {
+ /* end() flushes remaining tuples and subsumes finish_bulk_insert */
+ table_buffered_insert_end(myState->buffered_state);
+ myState->buffered_state = NULL;
+ }
+ else
+ {
+ FreeBulkInsertState(myState->bistate);
+ table_finish_bulk_insert(myState->rel, myState->ti_options);
+ }
}
/* close rel, but keep lock until commit */
diff --git a/src/test/regress/expected/select_into.out b/src/test/regress/expected/select_into.out
index d04ca2b1bf7..36cc783fda5 100644
--- a/src/test/regress/expected/select_into.out
+++ b/src/test/regress/expected/select_into.out
@@ -220,3 +220,101 @@ NOTICE: relation "ctas_ine_tbl" already exists, skipping
(0 rows)
DROP TABLE ctas_ine_tbl;
+--
+-- Tests for CTAS with buffered-insert path.
+--
+-- These tests verify correctness of CTAS results under both the buffered
+-- path (no volatile functions) and the fallback single-row path (volatile
+-- functions present, or EXECUTE). Path selection is not directly observable
+-- in SQL output; these tests validate that results are correct regardless.
+--
+-- Buffered path: small row count, verify contents.
+CREATE TABLE ctas_buffered_1 AS SELECT g AS a, g * 10 AS b FROM generate_series(1, 5) g;
+SELECT count(*) FROM ctas_buffered_1;
+ count
+-------
+ 5
+(1 row)
+
+SELECT * FROM ctas_buffered_1 ORDER BY a;
+ a | b
+---+----
+ 1 | 10
+ 2 | 20
+ 3 | 30
+ 4 | 40
+ 5 | 50
+(5 rows)
+
+DROP TABLE ctas_buffered_1;
+-- Buffered path: enough rows to trigger auto-flush (>1000 slot threshold).
+CREATE TABLE ctas_buffered_2 AS SELECT g AS a FROM generate_series(1, 2500) g;
+SELECT count(*) FROM ctas_buffered_2;
+ count
+-------
+ 2500
+(1 row)
+
+SELECT min(a), max(a) FROM ctas_buffered_2;
+ min | max
+-----+------
+ 1 | 2500
+(1 row)
+
+DROP TABLE ctas_buffered_2;
+-- Buffered path: wide tuples to exercise byte-threshold flushing.
+CREATE TABLE ctas_buffered_wide AS
+ SELECT g AS id,
+ repeat('x', 200) AS col1,
+ repeat('y', 200) AS col2,
+ repeat('z', 200) AS col3
+ FROM generate_series(1, 100) g;
+SELECT count(*) FROM ctas_buffered_wide;
+ count
+-------
+ 100
+(1 row)
+
+SELECT id, length(col1), length(col2), length(col3) FROM ctas_buffered_wide WHERE id IN (1, 50, 100) ORDER BY id;
+ id | length | length | length
+-----+--------+--------+--------
+ 1 | 200 | 200 | 200
+ 50 | 200 | 200 | 200
+ 100 | 200 | 200 | 200
+(3 rows)
+
+DROP TABLE ctas_buffered_wide;
+-- Fallback path: random() triggers the conservative volatile-function guard.
+-- Result must still be correct through the single-row insertion path.
+CREATE TABLE ctas_volatile AS SELECT g AS a, random() AS r FROM generate_series(1, 10) g;
+SELECT count(*) FROM ctas_volatile;
+ count
+-------
+ 10
+(1 row)
+
+SELECT a FROM ctas_volatile ORDER BY a;
+ a
+----
+ 1
+ 2
+ 3
+ 4
+ 5
+ 6
+ 7
+ 8
+ 9
+ 10
+(10 rows)
+
+DROP TABLE ctas_volatile;
+-- WITH NO DATA: no insertion path exercised; verify unchanged behavior.
+CREATE TABLE ctas_nodata AS SELECT 1 AS a WITH NO DATA;
+SELECT count(*) FROM ctas_nodata;
+ count
+-------
+ 0
+(1 row)
+
+DROP TABLE ctas_nodata;
diff --git a/src/test/regress/sql/select_into.sql b/src/test/regress/sql/select_into.sql
index f71e3940e0a..8147113cee3 100644
--- a/src/test/regress/sql/select_into.sql
+++ b/src/test/regress/sql/select_into.sql
@@ -136,3 +136,47 @@ EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
CREATE TABLE IF NOT EXISTS ctas_ine_tbl AS EXECUTE ctas_ine_query; -- ok
DROP TABLE ctas_ine_tbl;
+
+--
+-- Tests for CTAS with buffered-insert path.
+--
+-- These tests verify correctness of CTAS results under both the buffered
+-- path (no volatile functions) and the fallback single-row path (volatile
+-- functions present, or EXECUTE). Path selection is not directly observable
+-- in SQL output; these tests validate that results are correct regardless.
+--
+
+-- Buffered path: small row count, verify contents.
+CREATE TABLE ctas_buffered_1 AS SELECT g AS a, g * 10 AS b FROM generate_series(1, 5) g;
+SELECT count(*) FROM ctas_buffered_1;
+SELECT * FROM ctas_buffered_1 ORDER BY a;
+DROP TABLE ctas_buffered_1;
+
+-- Buffered path: enough rows to trigger auto-flush (>1000 slot threshold).
+CREATE TABLE ctas_buffered_2 AS SELECT g AS a FROM generate_series(1, 2500) g;
+SELECT count(*) FROM ctas_buffered_2;
+SELECT min(a), max(a) FROM ctas_buffered_2;
+DROP TABLE ctas_buffered_2;
+
+-- Buffered path: wide tuples to exercise byte-threshold flushing.
+CREATE TABLE ctas_buffered_wide AS
+ SELECT g AS id,
+ repeat('x', 200) AS col1,
+ repeat('y', 200) AS col2,
+ repeat('z', 200) AS col3
+ FROM generate_series(1, 100) g;
+SELECT count(*) FROM ctas_buffered_wide;
+SELECT id, length(col1), length(col2), length(col3) FROM ctas_buffered_wide WHERE id IN (1, 50, 100) ORDER BY id;
+DROP TABLE ctas_buffered_wide;
+
+-- Fallback path: random() triggers the conservative volatile-function guard.
+-- Result must still be correct through the single-row insertion path.
+CREATE TABLE ctas_volatile AS SELECT g AS a, random() AS r FROM generate_series(1, 10) g;
+SELECT count(*) FROM ctas_volatile;
+SELECT a FROM ctas_volatile ORDER BY a;
+DROP TABLE ctas_volatile;
+
+-- WITH NO DATA: no insertion path exercised; verify unchanged behavior.
+CREATE TABLE ctas_nodata AS SELECT 1 AS a WITH NO DATA;
+SELECT count(*) FROM ctas_nodata;
+DROP TABLE ctas_nodata;
--
2.52.0
[application/octet-stream] v1-0003-matview-use-buffered-insert-API-for-CMV-and-RMV.patch (12.0K, ../../CABXr29ExTDUoWZsiA0w+mkrjkJWKhPdiseM7vHJgHY-Mwq9PNg@mail.gmail.com/6-v1-0003-matview-use-buffered-insert-API-for-CMV-and-RMV.patch)
download | inline diff:
From 0b810edcdc5e80ab4ec7d77a8d6687fdedd73b32 Mon Sep 17 00:00:00 2001
From: Haibo Yan <[email protected]>
Date: Thu, 23 Apr 2026 12:43:31 -0700
Subject: [PATCH v1 3/5] matview: use buffered-insert API for CMV and RMV
Adopt the buffered-insert lifecycle API in the transientrel datafill
path used by CREATE MATERIALIZED VIEW and REFRESH MATERIALIZED VIEW.
The path uses table_buffered_insert_begin()/put()/end() with a NULL
flush callback and falls back to the existing single-row path when
needed.
Retain the volatile-function check conservatively for the initial patch
series. Concurrent refresh is unchanged.
Add focused CMV/RMV regression tests.
---
src/backend/commands/matview.c | 99 +++++++++++++----
src/test/regress/expected/matview.out | 152 ++++++++++++++++++++++++++
src/test/regress/sql/matview.sql | 59 ++++++++++
3 files changed, 291 insertions(+), 19 deletions(-)
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index f7d8007f796..7de4acbdfb1 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -31,6 +31,7 @@
#include "executor/executor.h"
#include "executor/spi.h"
#include "miscadmin.h"
+#include "optimizer/optimizer.h"
#include "pgstat.h"
#include "rewrite/rewriteHandler.h"
#include "storage/lmgr.h"
@@ -50,7 +51,9 @@ typedef struct
Relation transientrel; /* relation to write to */
CommandId output_cid; /* cmin to insert in output tuples */
uint32 ti_options; /* table_tuple_insert performance options */
- BulkInsertState bistate; /* bulk insert state */
+ BulkInsertState bistate; /* bulk insert state (fallback path only) */
+ TableBufferedInsertState buffered_state; /* buffered-insert state, or NULL */
+ bool use_buffered_insert; /* true if buffered path is eligible */
} DR_transientrel;
static int matview_maintenance_depth = 0;
@@ -427,6 +430,24 @@ refresh_matview_datafill(DestReceiver *dest, Query *query,
/* Plan the query which will generate data for the refresh. */
plan = pg_plan_query(query, queryString, CURSOR_OPT_PARALLEL_OK, NULL, NULL);
+ /*
+ * Conservative implementation choice: disable the buffered-insert path
+ * if the planned target list contains volatile functions.
+ *
+ * The buffered-insert API contract does not require this check — the
+ * matview's defining query was parsed at creation time and cannot
+ * reference the transient target table. This guard is retained for the
+ * initial patch series as a caller-local conservatism and can be relaxed
+ * after validation without any API change.
+ */
+ {
+ DR_transientrel *myState = (DR_transientrel *) dest;
+
+ myState->use_buffered_insert =
+ !contain_volatile_functions_after_planning(
+ (Expr *) plan->planTree->targetlist);
+ }
+
/*
* Use a snapshot with an updated command ID to ensure this query sees
* results of any previously executed queries. (This could only matter if
@@ -492,7 +513,40 @@ transientrel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
myState->transientrel = transientrel;
myState->output_cid = GetCurrentCommandId(true);
myState->ti_options = TABLE_INSERT_SKIP_FSM | TABLE_INSERT_FROZEN;
- myState->bistate = GetBulkInsertState();
+
+ if (myState->use_buffered_insert)
+ {
+ /*
+ * Try the buffered-insert path. Pass NULL flush callback — the
+ * transient table has no indexes, triggers, or per-tuple post-insert
+ * work during the datafill phase.
+ */
+ myState->ti_options |= TABLE_INSERT_BAS_BULKWRITE;
+ myState->buffered_state =
+ table_buffered_insert_begin(transientrel,
+ myState->output_cid,
+ myState->ti_options,
+ NULL, NULL);
+
+ if (myState->buffered_state != NULL)
+ {
+ myState->bistate = NULL;
+ }
+ else
+ {
+ /* AM does not support buffered inserts; fall back */
+ myState->bistate = GetBulkInsertState();
+ }
+ }
+ else
+ {
+ /*
+ * Buffered insertion not selected for this datafill. Currently this
+ * is reached when the conservative volatile-function guard fires.
+ */
+ myState->buffered_state = NULL;
+ myState->bistate = GetBulkInsertState();
+ }
/*
* Valid smgr_targblock implies something already wrote to the relation.
@@ -509,20 +563,19 @@ transientrel_receive(TupleTableSlot *slot, DestReceiver *self)
{
DR_transientrel *myState = (DR_transientrel *) self;
- /*
- * Note that the input slot might not be of the type of the target
- * relation. That's supported by table_tuple_insert(), but slightly less
- * efficient than inserting with the right slot - but the alternative
- * would be to copy into a slot of the right type, which would not be
- * cheap either. This also doesn't allow accessing per-AM data (say a
- * tuple's xmin), but since we don't do that here...
- */
-
- table_tuple_insert(myState->transientrel,
- slot,
- myState->output_cid,
- myState->ti_options,
- myState->bistate);
+ /* Both paths accept the caller-provided slot directly. */
+ if (myState->buffered_state != NULL)
+ {
+ table_buffered_insert_put(myState->buffered_state, slot);
+ }
+ else
+ {
+ table_tuple_insert(myState->transientrel,
+ slot,
+ myState->output_cid,
+ myState->ti_options,
+ myState->bistate);
+ }
/* We know this is a newly created relation, so there are no indexes */
@@ -537,9 +590,17 @@ transientrel_shutdown(DestReceiver *self)
{
DR_transientrel *myState = (DR_transientrel *) self;
- FreeBulkInsertState(myState->bistate);
-
- table_finish_bulk_insert(myState->transientrel, myState->ti_options);
+ if (myState->buffered_state != NULL)
+ {
+ /* end() flushes remaining tuples and subsumes finish_bulk_insert */
+ table_buffered_insert_end(myState->buffered_state);
+ myState->buffered_state = NULL;
+ }
+ else
+ {
+ FreeBulkInsertState(myState->bistate);
+ table_finish_bulk_insert(myState->transientrel, myState->ti_options);
+ }
/* close transientrel, but keep lock until commit */
table_close(myState->transientrel, NoLock);
diff --git a/src/test/regress/expected/matview.out b/src/test/regress/expected/matview.out
index 0355720dfc6..330ea4989f2 100644
--- a/src/test/regress/expected/matview.out
+++ b/src/test/regress/expected/matview.out
@@ -699,3 +699,155 @@ NOTICE: relation "matview_ine_tab" already exists, skipping
(0 rows)
DROP MATERIALIZED VIEW matview_ine_tab;
+--
+-- Tests for CMV/RMV with buffered-insert path.
+--
+-- These tests verify correctness under both the buffered path (no volatile
+-- functions) and the fallback single-row path (volatile functions present).
+-- Path selection is not directly observable in SQL output.
+--
+-- CMV: basic correctness
+CREATE MATERIALIZED VIEW mv_buffered_1 AS
+ SELECT g AS a, g * 10 AS b FROM generate_series(1, 5) g;
+SELECT count(*) FROM mv_buffered_1;
+ count
+-------
+ 5
+(1 row)
+
+SELECT * FROM mv_buffered_1 ORDER BY a;
+ a | b
+---+----
+ 1 | 10
+ 2 | 20
+ 3 | 30
+ 4 | 40
+ 5 | 50
+(5 rows)
+
+-- RMV: refresh repopulates correctly
+REFRESH MATERIALIZED VIEW mv_buffered_1;
+SELECT count(*) FROM mv_buffered_1;
+ count
+-------
+ 5
+(1 row)
+
+SELECT * FROM mv_buffered_1 ORDER BY a;
+ a | b
+---+----
+ 1 | 10
+ 2 | 20
+ 3 | 30
+ 4 | 40
+ 5 | 50
+(5 rows)
+
+DROP MATERIALIZED VIEW mv_buffered_1;
+-- CMV + RMV: bulk case to exercise auto-flush (>1000 rows)
+CREATE MATERIALIZED VIEW mv_buffered_bulk AS
+ SELECT g AS a FROM generate_series(1, 2500) g;
+SELECT count(*) FROM mv_buffered_bulk;
+ count
+-------
+ 2500
+(1 row)
+
+SELECT min(a), max(a) FROM mv_buffered_bulk;
+ min | max
+-----+------
+ 1 | 2500
+(1 row)
+
+REFRESH MATERIALIZED VIEW mv_buffered_bulk;
+SELECT count(*) FROM mv_buffered_bulk;
+ count
+-------
+ 2500
+(1 row)
+
+SELECT min(a), max(a) FROM mv_buffered_bulk;
+ min | max
+-----+------
+ 1 | 2500
+(1 row)
+
+DROP MATERIALIZED VIEW mv_buffered_bulk;
+-- Wide tuples: verify no regression
+CREATE MATERIALIZED VIEW mv_buffered_wide AS
+ SELECT g AS id,
+ repeat('x', 200) AS col1,
+ repeat('y', 200) AS col2,
+ repeat('z', 200) AS col3
+ FROM generate_series(1, 100) g;
+SELECT count(*) FROM mv_buffered_wide;
+ count
+-------
+ 100
+(1 row)
+
+SELECT id, length(col1), length(col2), length(col3)
+ FROM mv_buffered_wide WHERE id IN (1, 50, 100) ORDER BY id;
+ id | length | length | length
+-----+--------+--------+--------
+ 1 | 200 | 200 | 200
+ 50 | 200 | 200 | 200
+ 100 | 200 | 200 | 200
+(3 rows)
+
+DROP MATERIALIZED VIEW mv_buffered_wide;
+-- Volatile-function fallback: random() triggers conservative guard.
+-- Result must still be correct through the single-row path.
+CREATE MATERIALIZED VIEW mv_volatile AS
+ SELECT g AS a, random() AS r FROM generate_series(1, 10) g;
+SELECT count(*) FROM mv_volatile;
+ count
+-------
+ 10
+(1 row)
+
+SELECT a FROM mv_volatile ORDER BY a;
+ a
+----
+ 1
+ 2
+ 3
+ 4
+ 5
+ 6
+ 7
+ 8
+ 9
+ 10
+(10 rows)
+
+REFRESH MATERIALIZED VIEW mv_volatile;
+SELECT count(*) FROM mv_volatile;
+ count
+-------
+ 10
+(1 row)
+
+SELECT a FROM mv_volatile ORDER BY a;
+ a
+----
+ 1
+ 2
+ 3
+ 4
+ 5
+ 6
+ 7
+ 8
+ 9
+ 10
+(10 rows)
+
+DROP MATERIALIZED VIEW mv_volatile;
+-- WITH NO DATA: unchanged behavior
+CREATE MATERIALIZED VIEW mv_nodata AS SELECT 1 AS a WITH NO DATA;
+SELECT count(*) FROM mv_nodata; -- error: not populated
+ERROR: materialized view "mv_nodata" has not been populated
+HINT: Use the REFRESH MATERIALIZED VIEW command.
+REFRESH MATERIALIZED VIEW mv_nodata WITH NO DATA;
+DROP MATERIALIZED VIEW mv_nodata;
diff --git a/src/test/regress/sql/matview.sql b/src/test/regress/sql/matview.sql
index 934426b9ae8..43ceee50e90 100644
--- a/src/test/regress/sql/matview.sql
+++ b/src/test/regress/sql/matview.sql
@@ -318,3 +318,62 @@ EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF, BUFFERS OFF)
CREATE MATERIALIZED VIEW IF NOT EXISTS matview_ine_tab AS
SELECT 1 / 0 WITH NO DATA; -- ok
DROP MATERIALIZED VIEW matview_ine_tab;
+
+--
+-- Tests for CMV/RMV with buffered-insert path.
+--
+-- These tests verify correctness under both the buffered path (no volatile
+-- functions) and the fallback single-row path (volatile functions present).
+-- Path selection is not directly observable in SQL output.
+--
+
+-- CMV: basic correctness
+CREATE MATERIALIZED VIEW mv_buffered_1 AS
+ SELECT g AS a, g * 10 AS b FROM generate_series(1, 5) g;
+SELECT count(*) FROM mv_buffered_1;
+SELECT * FROM mv_buffered_1 ORDER BY a;
+
+-- RMV: refresh repopulates correctly
+REFRESH MATERIALIZED VIEW mv_buffered_1;
+SELECT count(*) FROM mv_buffered_1;
+SELECT * FROM mv_buffered_1 ORDER BY a;
+DROP MATERIALIZED VIEW mv_buffered_1;
+
+-- CMV + RMV: bulk case to exercise auto-flush (>1000 rows)
+CREATE MATERIALIZED VIEW mv_buffered_bulk AS
+ SELECT g AS a FROM generate_series(1, 2500) g;
+SELECT count(*) FROM mv_buffered_bulk;
+SELECT min(a), max(a) FROM mv_buffered_bulk;
+REFRESH MATERIALIZED VIEW mv_buffered_bulk;
+SELECT count(*) FROM mv_buffered_bulk;
+SELECT min(a), max(a) FROM mv_buffered_bulk;
+DROP MATERIALIZED VIEW mv_buffered_bulk;
+
+-- Wide tuples: verify no regression
+CREATE MATERIALIZED VIEW mv_buffered_wide AS
+ SELECT g AS id,
+ repeat('x', 200) AS col1,
+ repeat('y', 200) AS col2,
+ repeat('z', 200) AS col3
+ FROM generate_series(1, 100) g;
+SELECT count(*) FROM mv_buffered_wide;
+SELECT id, length(col1), length(col2), length(col3)
+ FROM mv_buffered_wide WHERE id IN (1, 50, 100) ORDER BY id;
+DROP MATERIALIZED VIEW mv_buffered_wide;
+
+-- Volatile-function fallback: random() triggers conservative guard.
+-- Result must still be correct through the single-row path.
+CREATE MATERIALIZED VIEW mv_volatile AS
+ SELECT g AS a, random() AS r FROM generate_series(1, 10) g;
+SELECT count(*) FROM mv_volatile;
+SELECT a FROM mv_volatile ORDER BY a;
+REFRESH MATERIALIZED VIEW mv_volatile;
+SELECT count(*) FROM mv_volatile;
+SELECT a FROM mv_volatile ORDER BY a;
+DROP MATERIALIZED VIEW mv_volatile;
+
+-- WITH NO DATA: unchanged behavior
+CREATE MATERIALIZED VIEW mv_nodata AS SELECT 1 AS a WITH NO DATA;
+SELECT count(*) FROM mv_nodata; -- error: not populated
+REFRESH MATERIALIZED VIEW mv_nodata WITH NO DATA;
+DROP MATERIALIZED VIEW mv_nodata;
--
2.52.0
[application/octet-stream] v1-0004-copy-adopt-buffered-insert-API-for-COPY-FROM.patch (18.4K, ../../CABXr29ExTDUoWZsiA0w+mkrjkJWKhPdiseM7vHJgHY-Mwq9PNg@mail.gmail.com/7-v1-0004-copy-adopt-buffered-insert-API-for-COPY-FROM.patch)
download | inline diff:
From 696e8bc0b5606385444d6f89397aa3e3b6801d43 Mon Sep 17 00:00:00 2001
From: Haibo Yan <[email protected]>
Date: Thu, 23 Apr 2026 13:42:46 -0700
Subject: [PATCH v1 4/5] copy: adopt buffered-insert API for COPY FROM
Adopt the buffered-insert lifecycle API in COPY FROM for non-FDW heap
targets, moving buffering ownership from COPY's local multi-insert
infrastructure into the table AM.
COPY's existing eligibility policy remains unchanged. The non-FDW
buffered path now uses table_buffered_insert_begin()/put()/flush()/end()
with a COPY-local flush callback for per-tuple post-insert work,
including index updates and AFTER ROW INSERT triggers. FDW batching is
unchanged.
Add focused COPY regression coverage for basic correctness, bulk input,
indexed targets, and AFTER ROW trigger behavior.
---
src/backend/commands/copyfrom.c | 185 +++++++++++++++++++++++++++--
src/test/regress/expected/copy.out | 166 ++++++++++++++++++++++++++
src/test/regress/sql/copy.sql | 102 ++++++++++++++++
3 files changed, 440 insertions(+), 13 deletions(-)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 64ac3063c61..f733bd2d296 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -76,6 +76,8 @@
*/
#define MAX_PARTITION_BUFFERS 32
+typedef struct CopyBufferedFlushState CopyBufferedFlushState;
+
/* Stores multi-insert data related to a single relation in CopyFrom. */
typedef struct CopyMultiInsertBuffer
{
@@ -83,6 +85,9 @@ typedef struct CopyMultiInsertBuffer
ResultRelInfo *resultRelInfo; /* ResultRelInfo for 'relid' */
BulkInsertState bistate; /* BulkInsertState for this rel if plain
* table; NULL if foreign table */
+ TableBufferedInsertState buffered_state; /* AM-owned buffered-insert
+ * state, or NULL */
+ CopyBufferedFlushState *flush_ctx; /* flush callback context, or NULL */
int nused; /* number of 'slots' containing tuples */
uint64 linenos[MAX_BUFFERED_TUPLES]; /* Line # of tuple in copy
* stream */
@@ -102,8 +107,24 @@ typedef struct CopyMultiInsertInfo
EState *estate; /* Executor state used for COPY */
CommandId mycid; /* Command Id used for COPY */
uint32 ti_options; /* table insert options */
+ int64 *processed; /* pointer to CopyFrom's row counter */
} CopyMultiInsertInfo;
+/*
+ * Context for the buffered-insert flush callback used by COPY. Carries the
+ * state needed to perform per-tuple post-insert work (index updates, AFTER
+ * ROW INSERT triggers, error context, progress tracking).
+ */
+typedef struct CopyBufferedFlushState
+{
+ CopyFromState cstate;
+ EState *estate;
+ ResultRelInfo *resultRelInfo;
+ int64 *processed; /* pointer to CopyFrom's processed counter */
+} CopyBufferedFlushState;
+
+static void CopyBufferedFlushCallback(void *context, TupleTableSlot *slot);
+
/* non-export function prototypes */
static void ClosePipeFromProgram(CopyFromState cstate);
@@ -362,15 +383,57 @@ CopyLimitPrintoutLength(const char *str)
* ResultRelInfo.
*/
static CopyMultiInsertBuffer *
-CopyMultiInsertBufferInit(ResultRelInfo *rri)
+CopyMultiInsertBufferInit(CopyMultiInsertInfo *miinfo, ResultRelInfo *rri)
{
CopyMultiInsertBuffer *buffer;
buffer = palloc_object(CopyMultiInsertBuffer);
memset(buffer->slots, 0, sizeof(TupleTableSlot *) * MAX_BUFFERED_TUPLES);
buffer->resultRelInfo = rri;
- buffer->bistate = (rri->ri_FdwRoutine == NULL) ? GetBulkInsertState() : NULL;
buffer->nused = 0;
+ buffer->buffered_state = NULL;
+ buffer->flush_ctx = NULL;
+
+ if (rri->ri_FdwRoutine == NULL)
+ {
+ /*
+ * Non-FDW table: try the AM-owned buffered-insert path. The flush
+ * callback handles index updates and AFTER ROW INSERT triggers.
+ */
+ CopyBufferedFlushState *flush_ctx;
+
+ flush_ctx = palloc_object(CopyBufferedFlushState);
+ flush_ctx->cstate = miinfo->cstate;
+ flush_ctx->estate = miinfo->estate;
+ flush_ctx->resultRelInfo = rri;
+ flush_ctx->processed = miinfo->processed;
+
+ buffer->buffered_state =
+ table_buffered_insert_begin(rri->ri_RelationDesc,
+ miinfo->mycid,
+ miinfo->ti_options |
+ TABLE_INSERT_BAS_BULKWRITE,
+ CopyBufferedFlushCallback,
+ flush_ctx);
+
+ if (buffer->buffered_state != NULL)
+ {
+ /* AM-owned buffering active; no COPY-side bistate needed */
+ buffer->bistate = NULL;
+ buffer->flush_ctx = flush_ctx;
+ }
+ else
+ {
+ /* AM does not support buffered inserts; fall back */
+ pfree(flush_ctx);
+ buffer->bistate = GetBulkInsertState();
+ }
+ }
+ else
+ {
+ /* FDW table: no buffered-insert, no bistate */
+ buffer->bistate = NULL;
+ }
return buffer;
}
@@ -384,7 +447,7 @@ CopyMultiInsertInfoSetupBuffer(CopyMultiInsertInfo *miinfo,
{
CopyMultiInsertBuffer *buffer;
- buffer = CopyMultiInsertBufferInit(rri);
+ buffer = CopyMultiInsertBufferInit(miinfo, rri);
/* Setup back-link so we can easily find this buffer again */
rri->ri_CopyMultiInsertBuffer = buffer;
@@ -401,7 +464,7 @@ CopyMultiInsertInfoSetupBuffer(CopyMultiInsertInfo *miinfo,
static void
CopyMultiInsertInfoInit(CopyMultiInsertInfo *miinfo, ResultRelInfo *rri,
CopyFromState cstate, EState *estate, CommandId mycid,
- uint32 ti_options)
+ uint32 ti_options, int64 *processed)
{
miinfo->multiInsertBuffers = NIL;
miinfo->bufferedTuples = 0;
@@ -410,6 +473,7 @@ CopyMultiInsertInfoInit(CopyMultiInsertInfo *miinfo, ResultRelInfo *rri,
miinfo->estate = estate;
miinfo->mycid = mycid;
miinfo->ti_options = ti_options;
+ miinfo->processed = processed;
/*
* Only setup the buffer when not dealing with a partitioned table.
@@ -532,6 +596,32 @@ CopyMultiInsertBufferFlush(CopyMultiInsertInfo *miinfo,
/* reset relname_only */
cstate->relname_only = false;
}
+ else if (buffer->buffered_state != NULL)
+ {
+ bool line_buf_valid = cstate->line_buf_valid;
+ uint64 save_cur_lineno = cstate->cur_lineno;
+
+ /*
+ * AM-owned buffered path: tuples were submitted via put() and are
+ * inside the AM already. Flush triggers the batch write and invokes
+ * the flush callback once per written tuple for index updates,
+ * trigger firing, and progress tracking.
+ */
+ cstate->line_buf_valid = false;
+
+ table_buffered_insert_flush(buffer->buffered_state);
+
+ /* Clear slots that were used for staging before put() */
+ for (i = 0; i < nused; i++)
+ {
+ if (slots[i] != NULL)
+ ExecClearTuple(slots[i]);
+ }
+
+ /* reset cur_lineno and line_buf_valid to what they were */
+ cstate->line_buf_valid = line_buf_valid;
+ cstate->cur_lineno = save_cur_lineno;
+ }
else
{
CommandId mycid = miinfo->mycid;
@@ -631,10 +721,20 @@ CopyMultiInsertBufferCleanup(CopyMultiInsertInfo *miinfo,
/* Remove back-link to ourself */
resultRelInfo->ri_CopyMultiInsertBuffer = NULL;
- if (resultRelInfo->ri_FdwRoutine == NULL)
+ if (buffer->buffered_state != NULL)
+ {
+ /* end() flushes remaining tuples and subsumes finish_bulk_insert */
+ table_buffered_insert_end(buffer->buffered_state);
+ buffer->buffered_state = NULL;
+ if (buffer->flush_ctx != NULL)
+ pfree(buffer->flush_ctx);
+ }
+ else if (resultRelInfo->ri_FdwRoutine == NULL)
{
Assert(buffer->bistate != NULL);
FreeBulkInsertState(buffer->bistate);
+ table_finish_bulk_insert(resultRelInfo->ri_RelationDesc,
+ miinfo->ti_options);
}
else
Assert(buffer->bistate == NULL);
@@ -643,10 +743,6 @@ CopyMultiInsertBufferCleanup(CopyMultiInsertInfo *miinfo,
for (i = 0; i < MAX_BUFFERED_TUPLES && buffer->slots[i] != NULL; i++)
ExecDropSingleTupleTableSlot(buffer->slots[i]);
- if (resultRelInfo->ri_FdwRoutine == NULL)
- table_finish_bulk_insert(resultRelInfo->ri_RelationDesc,
- miinfo->ti_options);
-
pfree(buffer);
}
@@ -761,12 +857,26 @@ CopyMultiInsertInfoStore(CopyMultiInsertInfo *miinfo, ResultRelInfo *rri,
CopyMultiInsertBuffer *buffer = rri->ri_CopyMultiInsertBuffer;
Assert(buffer != NULL);
- Assert(slot == buffer->slots[buffer->nused]);
- /* Store the line number so we can properly report any errors later */
+ /* Store the line number for error context during flush */
buffer->linenos[buffer->nused] = lineno;
- /* Record this slot as being used */
+ if (buffer->buffered_state != NULL)
+ {
+ /*
+ * AM-owned buffered path: submit the tuple directly to the AM.
+ * The AM captures the data internally; the caller retains slot
+ * ownership and may reuse it. The AM may auto-flush during put(),
+ * which fires the flush callback for already-buffered tuples.
+ */
+ table_buffered_insert_put(buffer->buffered_state, slot);
+ }
+ else
+ {
+ /* Legacy path: slot was already placed into buffer->slots by caller */
+ Assert(slot == buffer->slots[buffer->nused]);
+ }
+
buffer->nused++;
/* Update how many tuples are stored and their size */
@@ -774,6 +884,55 @@ CopyMultiInsertInfoStore(CopyMultiInsertInfo *miinfo, ResultRelInfo *rri,
miinfo->bufferedBytes += tuplen;
}
+/*
+ * Flush callback for the AM-owned buffered-insert path.
+ *
+ * Invoked once per flushed tuple, in insertion order. The slot is an
+ * AM-owned scratch object with TID set; it is valid only for the duration
+ * of this callback.
+ */
+static void
+CopyBufferedFlushCallback(void *context, TupleTableSlot *slot)
+{
+ CopyBufferedFlushState *ctx = (CopyBufferedFlushState *) context;
+ ResultRelInfo *resultRelInfo = ctx->resultRelInfo;
+ EState *estate = ctx->estate;
+
+ /*
+ * cstate->cur_lineno is not restored per-tuple here. During auto-flush
+ * (triggered inside put()), it reflects the line being stored, which is
+ * the best available context. During explicit flush, the caller has
+ * already set line_buf_valid = false, so only the relation name appears
+ * in error context — matching the legacy multi-insert path.
+ */
+
+ if (resultRelInfo->ri_NumIndices > 0)
+ {
+ List *recheckIndexes;
+
+ recheckIndexes = ExecInsertIndexTuples(resultRelInfo,
+ estate, 0, slot,
+ NIL, NULL);
+ ExecARInsertTriggers(estate, resultRelInfo,
+ slot, recheckIndexes,
+ ctx->cstate->transition_capture);
+ list_free(recheckIndexes);
+ }
+ else if (resultRelInfo->ri_TrigDesc != NULL &&
+ (resultRelInfo->ri_TrigDesc->trig_insert_after_row ||
+ resultRelInfo->ri_TrigDesc->trig_insert_new_table))
+ {
+ ExecARInsertTriggers(estate, resultRelInfo,
+ slot, NIL,
+ ctx->cstate->transition_capture);
+ }
+
+ /* Update row counter and progress */
+ (*ctx->processed)++;
+ pgstat_progress_update_param(PROGRESS_COPY_TUPLES_PROCESSED,
+ *ctx->processed);
+}
+
/*
* Copy FROM file to relation.
*/
@@ -1073,7 +1232,7 @@ CopyFrom(CopyFromState cstate)
insertMethod = CIM_MULTI;
CopyMultiInsertInfoInit(&multiInsertInfo, resultRelInfo, cstate,
- estate, mycid, ti_options);
+ estate, mycid, ti_options, &processed);
}
/*
diff --git a/src/test/regress/expected/copy.out b/src/test/regress/expected/copy.out
index 1714faab39c..ec8e781c5ce 100644
--- a/src/test/regress/expected/copy.out
+++ b/src/test/regress/expected/copy.out
@@ -594,3 +594,169 @@ id val
5 15
6 16
DROP TABLE PP;
+--
+-- Tests for COPY FROM with buffered-insert path.
+--
+-- These validate correctness of COPY FROM under the AM-owned buffered path.
+-- Path selection is not directly observable in SQL output.
+--
+-- Basic COPY: small row count
+CREATE TABLE copy_buffered_basic (a int, b text);
+COPY copy_buffered_basic FROM stdin;
+SELECT count(*) FROM copy_buffered_basic;
+ count
+-------
+ 3
+(1 row)
+
+SELECT * FROM copy_buffered_basic ORDER BY a;
+ a | b
+---+-------
+ 1 | hello
+ 2 | world
+ 3 | foo
+(3 rows)
+
+DROP TABLE copy_buffered_basic;
+-- Bulk COPY: exercise buffered path with enough rows for auto-flush.
+-- Generate data in a source table, COPY TO a file, then COPY FROM that file.
+CREATE TABLE copy_bulk_src (a int, b text);
+INSERT INTO copy_bulk_src SELECT g, 'row-' || g FROM generate_series(1, 2500) g;
+CREATE TABLE copy_bulk_dst (a int, b text);
+COPY copy_bulk_src TO '/tmp/copy_buffered_bulk_test.csv' CSV;
+COPY copy_bulk_dst FROM '/tmp/copy_buffered_bulk_test.csv' CSV;
+SELECT count(*) FROM copy_bulk_dst;
+ count
+-------
+ 2500
+(1 row)
+
+SELECT min(a), max(a) FROM copy_bulk_dst;
+ min | max
+-----+------
+ 1 | 2500
+(1 row)
+
+-- Verify data integrity
+SELECT count(*) FROM copy_bulk_dst d JOIN copy_bulk_src s ON d.a = s.a AND d.b = s.b;
+ count
+-------
+ 2500
+(1 row)
+
+DROP TABLE copy_bulk_src;
+DROP TABLE copy_bulk_dst;
+-- COPY into indexed table: verify index maintenance via flush callback
+CREATE TABLE copy_indexed (a int PRIMARY KEY, b text);
+COPY copy_indexed FROM stdin;
+SELECT count(*) FROM copy_indexed;
+ count
+-------
+ 5
+(1 row)
+
+SELECT * FROM copy_indexed ORDER BY a;
+ a | b
+---+---------
+ 1 | alpha
+ 2 | beta
+ 3 | gamma
+ 4 | delta
+ 5 | epsilon
+(5 rows)
+
+-- Verify index is usable
+SET enable_seqscan = off;
+SELECT a FROM copy_indexed WHERE a = 3;
+ a
+---
+ 3
+(1 row)
+
+RESET enable_seqscan;
+DROP TABLE copy_indexed;
+-- COPY with AFTER ROW INSERT trigger: verify trigger fires via flush callback
+CREATE TABLE copy_trigger_tgt (a int, b text);
+CREATE TABLE copy_trigger_log (a int, b text);
+CREATE FUNCTION copy_trigger_fn() RETURNS trigger AS $$
+BEGIN
+ INSERT INTO copy_trigger_log(a, b) VALUES (NEW.a, NEW.b);
+ RETURN NEW;
+END;
+$$ LANGUAGE plpgsql;
+CREATE TRIGGER copy_after_ins
+ AFTER INSERT ON copy_trigger_tgt
+ FOR EACH ROW EXECUTE FUNCTION copy_trigger_fn();
+COPY copy_trigger_tgt FROM stdin;
+SELECT count(*) FROM copy_trigger_tgt;
+ count
+-------
+ 3
+(1 row)
+
+SELECT a, b FROM copy_trigger_tgt ORDER BY a;
+ a | b
+----+--------
+ 10 | first
+ 20 | second
+ 30 | third
+(3 rows)
+
+-- Verify trigger fired for each row
+SELECT count(*) FROM copy_trigger_log;
+ count
+-------
+ 3
+(1 row)
+
+SELECT a, b FROM copy_trigger_log ORDER BY a;
+ a | b
+----+--------
+ 10 | first
+ 20 | second
+ 30 | third
+(3 rows)
+
+DROP TABLE copy_trigger_tgt CASCADE;
+DROP TABLE copy_trigger_log;
+DROP FUNCTION copy_trigger_fn;
+-- Partitioned COPY: verify correctness under partition routing with buffered path
+CREATE TABLE copy_part_routed (
+ id int,
+ val text
+) PARTITION BY RANGE (id);
+CREATE TABLE copy_part_routed_p1 PARTITION OF copy_part_routed
+ FOR VALUES FROM (1) TO (100);
+CREATE TABLE copy_part_routed_p2 PARTITION OF copy_part_routed
+ FOR VALUES FROM (100) TO (200);
+COPY copy_part_routed FROM stdin;
+-- Total row count
+SELECT count(*) FROM copy_part_routed;
+ count
+-------
+ 6
+(1 row)
+
+-- Per-partition row counts
+SELECT tableoid::regclass, count(*)
+FROM copy_part_routed
+GROUP BY tableoid ORDER BY tableoid::regclass::name;
+ tableoid | count
+---------------------+-------
+ copy_part_routed_p1 | 3
+ copy_part_routed_p2 | 3
+(2 rows)
+
+-- Content correctness
+SELECT * FROM copy_part_routed ORDER BY id;
+ id | val
+-----+---------
+ 1 | alpha
+ 50 | bravo
+ 99 | charlie
+ 100 | delta
+ 150 | echo
+ 199 | foxtrot
+(6 rows)
+
+DROP TABLE copy_part_routed;
diff --git a/src/test/regress/sql/copy.sql b/src/test/regress/sql/copy.sql
index eaad290b257..e4a10e8cf95 100644
--- a/src/test/regress/sql/copy.sql
+++ b/src/test/regress/sql/copy.sql
@@ -535,3 +535,105 @@ CREATE TABLE pp_510 PARTITION OF pp_2 FOR VALUES FROM (5) TO (10);
INSERT INTO pp SELECT g, 10 + g FROM generate_series(1,6) g;
COPY pp TO stdout(header);
DROP TABLE PP;
+
+--
+-- Tests for COPY FROM with buffered-insert path.
+--
+-- These validate correctness of COPY FROM under the AM-owned buffered path.
+-- Path selection is not directly observable in SQL output.
+--
+
+-- Basic COPY: small row count
+CREATE TABLE copy_buffered_basic (a int, b text);
+COPY copy_buffered_basic FROM stdin;
+1 hello
+2 world
+3 foo
+\.
+SELECT count(*) FROM copy_buffered_basic;
+SELECT * FROM copy_buffered_basic ORDER BY a;
+DROP TABLE copy_buffered_basic;
+
+-- Bulk COPY: exercise buffered path with enough rows for auto-flush.
+-- Generate data in a source table, COPY TO a file, then COPY FROM that file.
+CREATE TABLE copy_bulk_src (a int, b text);
+INSERT INTO copy_bulk_src SELECT g, 'row-' || g FROM generate_series(1, 2500) g;
+CREATE TABLE copy_bulk_dst (a int, b text);
+COPY copy_bulk_src TO '/tmp/copy_buffered_bulk_test.csv' CSV;
+COPY copy_bulk_dst FROM '/tmp/copy_buffered_bulk_test.csv' CSV;
+SELECT count(*) FROM copy_bulk_dst;
+SELECT min(a), max(a) FROM copy_bulk_dst;
+-- Verify data integrity
+SELECT count(*) FROM copy_bulk_dst d JOIN copy_bulk_src s ON d.a = s.a AND d.b = s.b;
+DROP TABLE copy_bulk_src;
+DROP TABLE copy_bulk_dst;
+
+-- COPY into indexed table: verify index maintenance via flush callback
+CREATE TABLE copy_indexed (a int PRIMARY KEY, b text);
+COPY copy_indexed FROM stdin;
+1 alpha
+2 beta
+3 gamma
+4 delta
+5 epsilon
+\.
+SELECT count(*) FROM copy_indexed;
+SELECT * FROM copy_indexed ORDER BY a;
+-- Verify index is usable
+SET enable_seqscan = off;
+SELECT a FROM copy_indexed WHERE a = 3;
+RESET enable_seqscan;
+DROP TABLE copy_indexed;
+
+-- COPY with AFTER ROW INSERT trigger: verify trigger fires via flush callback
+CREATE TABLE copy_trigger_tgt (a int, b text);
+CREATE TABLE copy_trigger_log (a int, b text);
+CREATE FUNCTION copy_trigger_fn() RETURNS trigger AS $$
+BEGIN
+ INSERT INTO copy_trigger_log(a, b) VALUES (NEW.a, NEW.b);
+ RETURN NEW;
+END;
+$$ LANGUAGE plpgsql;
+CREATE TRIGGER copy_after_ins
+ AFTER INSERT ON copy_trigger_tgt
+ FOR EACH ROW EXECUTE FUNCTION copy_trigger_fn();
+COPY copy_trigger_tgt FROM stdin;
+10 first
+20 second
+30 third
+\.
+SELECT count(*) FROM copy_trigger_tgt;
+SELECT a, b FROM copy_trigger_tgt ORDER BY a;
+-- Verify trigger fired for each row
+SELECT count(*) FROM copy_trigger_log;
+SELECT a, b FROM copy_trigger_log ORDER BY a;
+DROP TABLE copy_trigger_tgt CASCADE;
+DROP TABLE copy_trigger_log;
+DROP FUNCTION copy_trigger_fn;
+
+-- Partitioned COPY: verify correctness under partition routing with buffered path
+CREATE TABLE copy_part_routed (
+ id int,
+ val text
+) PARTITION BY RANGE (id);
+CREATE TABLE copy_part_routed_p1 PARTITION OF copy_part_routed
+ FOR VALUES FROM (1) TO (100);
+CREATE TABLE copy_part_routed_p2 PARTITION OF copy_part_routed
+ FOR VALUES FROM (100) TO (200);
+COPY copy_part_routed FROM stdin;
+1 alpha
+50 bravo
+99 charlie
+100 delta
+150 echo
+199 foxtrot
+\.
+-- Total row count
+SELECT count(*) FROM copy_part_routed;
+-- Per-partition row counts
+SELECT tableoid::regclass, count(*)
+FROM copy_part_routed
+GROUP BY tableoid ORDER BY tableoid::regclass::name;
+-- Content correctness
+SELECT * FROM copy_part_routed ORDER BY id;
+DROP TABLE copy_part_routed;
--
2.52.0
^ permalink raw reply [nested|flat] 47+ messages in thread
end of thread, other threads:[~2026-04-28 04:27 UTC | newest]
Thread overview: 47+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-01-18 03:54 Thread-unsafe coding in ecpg Tom Lane <[email protected]>
2019-01-19 19:15 ` Michael Meskes <[email protected]>
2019-01-20 00:10 ` Tom Lane <[email protected]>
2019-01-20 00:23 ` Michael Meskes <[email protected]>
2019-01-20 00:37 ` Tom Lane <[email protected]>
2019-01-20 17:08 ` Michael Meskes <[email protected]>
2019-01-20 17:30 ` Tom Lane <[email protected]>
2019-01-20 20:04 ` Tom Lane <[email protected]>
2019-01-21 02:14 ` Tsunakawa, Takayuki <[email protected]>
2019-01-21 02:20 ` Tom Lane <[email protected]>
2019-01-21 02:57 ` Tsunakawa, Takayuki <[email protected]>
2019-01-21 03:16 ` Tom Lane <[email protected]>
2019-01-21 06:07 ` Tsunakawa, Takayuki <[email protected]>
2019-01-21 17:09 ` Tom Lane <[email protected]>
2019-01-21 17:51 ` Michael Meskes <[email protected]>
2019-01-21 19:35 ` Andres Freund <[email protected]>
2019-01-21 20:05 ` Tom Lane <[email protected]>
2019-01-21 20:18 ` Joshua D. Drake <[email protected]>
2019-01-21 20:25 ` Tom Lane <[email protected]>
2019-01-22 03:00 ` Andrew Dunstan <[email protected]>
2019-01-22 17:50 ` Andrew Dunstan <[email protected]>
2019-01-22 17:56 ` Andres Freund <[email protected]>
2019-01-23 22:37 ` Andrew Dunstan <[email protected]>
2019-01-23 23:01 ` Tom Lane <[email protected]>
2019-01-24 00:28 ` Andrew Dunstan <[email protected]>
2019-01-24 03:10 ` Tom Lane <[email protected]>
2019-01-24 03:53 ` Tom Lane <[email protected]>
2019-01-24 05:23 ` Tom Lane <[email protected]>
2019-01-24 16:17 ` Andrew Dunstan <[email protected]>
2019-01-26 04:25 ` Michael Meskes <[email protected]>
2019-01-26 14:57 ` Tom Lane <[email protected]>
2019-01-29 11:58 ` Michael Meskes <[email protected]>
2019-01-21 20:21 ` Andres Freund <[email protected]>
2019-01-21 20:45 ` Tom Lane <[email protected]>
2019-01-22 06:57 ` Tsunakawa, Takayuki <[email protected]>
2019-01-20 01:32 ` Andrew Gierth <[email protected]>
2019-01-20 02:00 ` Tom Lane <[email protected]>
2019-02-21 03:42 [PATCH 4/6] Allow dsm to use on postmaster. Kyotaro Horiguchi <[email protected]>
2019-02-21 03:42 [PATCH 4/6] Allow dsm to use on postmaster. Kyotaro Horiguchi <[email protected]>
2023-09-12 05:22 [PATCH v6 1/7] Row pattern recognition patch for raw parser. Tatsuo Ishii <[email protected]>
2024-10-02 10:00 Re: pg_basebackup and error messages dependent on the order of the arguments Daniel Westermann (DWE) <[email protected]>
2024-10-03 19:19 ` Re: pg_basebackup and error messages dependent on the order of the arguments Robert Haas <[email protected]>
2024-10-03 19:32 ` Re: pg_basebackup and error messages dependent on the order of the arguments Tom Lane <[email protected]>
2025-03-09 11:27 Re: Introduce new multi insert Table AM and improve performance of various SQL commands with it for Heap AM Daniil Davydov <[email protected]>
2025-03-17 04:49 ` Re: Introduce new multi insert Table AM and improve performance of various SQL commands with it for Heap AM Daniil Davydov <[email protected]>
2025-04-07 06:26 ` Re: Introduce new multi insert Table AM and improve performance of various SQL commands with it for Heap AM Daniil Davydov <[email protected]>
2026-04-28 04:27 ` Re: Introduce new multi insert Table AM and improve performance of various SQL commands with it for Heap AM Haibo Yan <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox