Re: buildfarm + meson
On 2023-03-18 Sa 19:00, Andres Freund wrote: Hi, On 2023-03-18 17:53:38 -0400, Andrew Dunstan wrote: On 2023-03-11 Sa 16:25, Andres Freund wrote: Hi, On 2023-03-09 18:31:10 -0500, Andrew Dunstan wrote: Another thing: the test for uuid.h is too strict. On Fedora 36 the OSSP header is in /usr/include, not /usr/include/ossp (I got around that for now by symlinking it, but obviously that's a nasty hack we can't ask people to do) Yea, that was just wrong. It happened to work on debian and a few other OSs, but ossp's .pc puts whatever the right directory is into the include path. Pushed the fairly obvious fix. Another issue: building plpython appears impossible on Windows because it's finding meson's own python: Program python3 found: YES (C:\Program Files\Meson\meson.exe runpython) Could not find Python3 library 'C:\\Program Files\\Meson\\libs\\python311.lib' Any more details - windows CI builds with python. What python do you want to use and where is it installed? It's in c:/python37, which is at the front of the PATH. It fails as above if I add -Dplpython=enabled to the config. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: Commitfest 2023-03 starting tomorrow!
On Sat, Mar 18, 2023 at 04:28:02PM -0700, Peter Geoghegan wrote: > On Sat, Mar 18, 2023 at 4:19 PM Justin Pryzby wrote: > > The only issue with this is that cfbot has squished all the commits into > > one, and lost the original commit messages (if any). I submitted > > patches to address that but still waiting for feedback. > > > > https://www.postgresql.org/message-id/20220623193125.gb22...@telsasoft.com > > Right. I would like to see that change. But you still need to have CF > tester/the CF app remember the last master branch commit that worked > before bitrot. And you have to provide an easy way to get that > information. No - the last in cfbot's repo is from the last time it successfully applied the patch. You can summarily check checkout cfbot's branch to build (or just to git log -p it, if you dislike github's web interface). If you're curious and still wanted to know what commit it was applied on, it's currently the 2nd commit in "git log" (due to squishing all patches into one).
Re: Commitfest 2023-03 starting tomorrow!
On Sat, Mar 18, 2023 at 4:19 PM Justin Pryzby wrote: > The only issue with this is that cfbot has squished all the commits into > one, and lost the original commit messages (if any). I submitted > patches to address that but still waiting for feedback. > > https://www.postgresql.org/message-id/20220623193125.gb22...@telsasoft.com Right. I would like to see that change. But you still need to have CF tester/the CF app remember the last master branch commit that worked before bitrot. And you have to provide an easy way to get that information. I generally don't care if that means that I have to initdb - I do that all the time. It's a small price to pay for a workflow that I know is practically guaranteed to get me a usable postgres executable on the first try, without requiring any special effort. I don't want to even think about bitrot until I'm at least 10 minutes into looking at something. -- Peter Geoghegan
Re: Commitfest 2023-03 starting tomorrow!
On Sat, Mar 18, 2023 at 02:43:38PM -0700, Peter Geoghegan wrote: > On Sat, Mar 18, 2023 at 1:26 PM Alvaro Herrera > wrote: > > At this point, I'm going to suggest that reviewers should be open to the > > idea of applying a submitted patch to some older Git commit in order to > > review it. If we have given feedback, then it's OK to put a patch as > > "waiting on author" and eventually boot it; but if we have not given > > feedback, and there is no reason to think that the merge conflicts some > > how make the patch fundamentally obsolete, then we should *not* set it > > Waiting on Author. After all, it is quite easy to "git checkout" a > > slightly older tree to get the patch to apply cleanly and review it > > there. > > It seems plausible that improved tooling that makes it quick and easy > to test a given patch locally could improve things for everybody. > > It's possible to do a git checkout to a slightly older tree today, of > course. But in practice it's harder than it really should be. It would > be very nice if there was an easy way to fetch from a git remote, and > then check out a branch with a given patch applied on top of the "last > known good git tip" commit. The tricky part would be systematically > tracking which precise master branch commit is the last known "good > commit" for a given CF entry. That seems doable to me. It's not only doable, but already possible. https://www.postgresql.org/message-id/CA%2BhUKGLW2PnHxabF3JZGoPfcKFYRCtx%2Bhu5a5yw%3DKWy57yW5cg%40mail.gmail.com The only issue with this is that cfbot has squished all the commits into one, and lost the original commit messages (if any). I submitted patches to address that but still waiting for feedback. https://www.postgresql.org/message-id/20220623193125.gb22...@telsasoft.com -- Justin
Re: buildfarm + meson
Hi, On 2023-03-18 17:53:38 -0400, Andrew Dunstan wrote: > On 2023-03-11 Sa 16:25, Andres Freund wrote: > > Hi, > > > > On 2023-03-09 18:31:10 -0500, Andrew Dunstan wrote: > > > Another thing: the test for uuid.h is too strict. On Fedora 36 the OSSP > > > header is in /usr/include, not /usr/include/ossp (I got around that for > > > now > > > by symlinking it, but obviously that's a nasty hack we can't ask people to > > > do) > > Yea, that was just wrong. It happened to work on debian and a few other OSs, > > but ossp's .pc puts whatever the right directory is into the include > > path. Pushed the fairly obvious fix. > > > Another issue: building plpython appears impossible on Windows because it's > finding meson's own python: > > > Program python3 found: YES (C:\Program Files\Meson\meson.exe runpython) > Could not find Python3 library 'C:\\Program > Files\\Meson\\libs\\python311.lib' Any more details - windows CI builds with python. What python do you want to use and where is it installed? Greetings, Andres Freund
Re: Should vacuum process config file reload more often
On Wed, Mar 15, 2023 at 1:14 AM Masahiko Sawada wrote:
> On Sat, Mar 11, 2023 at 8:11 AM Melanie Plageman
> wrote:
> > I've implemented the atomic cost limit in the attached patch. Though,
> > I'm pretty unsure about how I initialized the atomics in
> > AutoVacuumShmemInit()...
>
> +
> /* initialize the WorkerInfo free list */
> for (i = 0; i < autovacuum_max_workers; i++)
> dlist_push_head(>av_freeWorkers,
> [i].wi_links);
> +
> +dlist_foreach(iter, >av_freeWorkers)
> +pg_atomic_init_u32(
> +
> &(dlist_container(WorkerInfoData, wi_links, iter.cur))->wi_cost_limit,
> + 0);
> +
>
> I think we can do like:
>
> /* initialize the WorkerInfo free list */
> for (i = 0; i < autovacuum_max_workers; i++)
> {
> dlist_push_head(>av_freeWorkers,
> [i].wi_links);
> pg_atomic_init_u32(&(worker[i].wi_cost_limit));
> }
Ah, yes, I was distracted by the variable name "worker" (as opposed to
"workers").
> > If the consensus is that it is simply too confusing to take
> > wi_cost_delay out of WorkerInfo, we might be able to afford using a
> > shared lock to access it because we won't call AutoVacuumUpdateDelay()
> > on every invocation of vacuum_delay_point() -- only when we've reloaded
> > the config file.
> >
> > One potential option to avoid taking a shared lock on every call to
> > AutoVacuumUpdateDelay() is to set a global variable to indicate that we
> > did update it (since we are the only ones updating it) and then only
> > take the shared LWLock in AutoVacuumUpdateDelay() if that flag is true.
> >
>
> If we remove wi_cost_delay from WorkerInfo, probably we don't need to
> acquire the lwlock in AutoVacuumUpdateDelay()? The shared field we
> access in that function will be only wi_dobalance, but this field is
> updated only by its owner autovacuum worker.
I realized that we cannot use dobalance to decide whether or not to
update wi_cost_delay because dobalance could be false because of table
option cost limit being set (with no table option cost delay) and we
would still need to update VacuumCostDelay and wi_cost_delay with the
new value of autovacuum_vacuum_cost_delay.
But v5 skirts around this issue altogether.
> > > ---
> > > void
> > > AutoVacuumUpdateDelay(void)
> > > {
> > > -if (MyWorkerInfo)
> > > +/*
> > > + * We are using autovacuum-related GUCs to update
> > > VacuumCostDelay, so we
> > > + * only want autovacuum workers and autovacuum launcher to do
> > > this.
> > > + */
> > > +if (!(am_autovacuum_worker || am_autovacuum_launcher))
> > > +return;
> > >
> > > Is there any case where the autovacuum launcher calls
> > > AutoVacuumUpdateDelay() function?
> >
> > I had meant to add it to HandleAutoVacLauncherInterrupts() after
> > reloading the config file (done in attached patch). When using the
> > global variables for cost delay (instead of wi_cost_delay in worker
> > info), the autovac launcher also has to do the check in the else branch
> > of AutoVacuumUpdateDelay()
> >
> > VacuumCostDelay = autovacuum_vac_cost_delay >= 0 ?
> > autovacuum_vac_cost_delay : VacuumCostDelay;
> >
> > to make sure VacuumCostDelay is correct for when it calls
> > autovac_balance_cost().
>
> But doesn't the launcher do a similar thing at the beginning of
> autovac_balance_cost()?
>
> double vac_cost_delay = (autovacuum_vac_cost_delay >= 0 ?
> autovacuum_vac_cost_delay :
> VacuumCostDelay);
Ah, yes. You are right.
> Related to this point, I think autovac_balance_cost() should use
> globally-set cost_limit and cost_delay values to calculate worker's
> vacuum-delay parameters. IOW, vac_cost_limit and vac_cost_delay should
> come from the config file setting, not table option etc:
>
> int vac_cost_limit = (autovacuum_vac_cost_limit > 0 ?
> autovacuum_vac_cost_limit :
> VacuumCostLimit);
> double vac_cost_delay = (autovacuum_vac_cost_delay >= 0 ?
> autovacuum_vac_cost_delay :
> VacuumCostDelay);
>
> If my understanding is right, the following change is not right;
> AutoVacUpdateLimit() updates the VacuumCostLimit based on the value in
> MyWorkerInfo:
>
> MyWorkerInfo->wi_cost_limit_base = tab->at_vacuum_cost_limit;
> +AutoVacuumUpdateLimit();
>
> /* do a balance */
> autovac_balance_cost();
>
> -/* set the active cost parameters from the result of that */
> -AutoVacuumUpdateDelay();
>
> Also, even when using the global variables for cost delay, the
> launcher doesn't need to check the global variable. It should always
> be able
Bytea PL/Perl transform
Hi,
PostgreSQL passes bytea arguments to PL/Perl functions as hexadecimal strings,
which is not only inconvenient, but also memory and time consuming.
So I decided to propose a simple transform extension to pass bytea as native
Perl octet strings.
Please find the patch attached.
Regards,
Ivan Panchenko
diff --git a/contrib/Makefile b/contrib/Makefile
index bbf220407b..bb997dda69 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -78,9 +78,9 @@ ALWAYS_SUBDIRS += sepgsql
endif
ifeq ($(with_perl),yes)
-SUBDIRS += bool_plperl hstore_plperl jsonb_plperl
+SUBDIRS += bool_plperl bytea_plperl hstore_plperl jsonb_plperl
else
-ALWAYS_SUBDIRS += bool_plperl hstore_plperl jsonb_plperl
+ALWAYS_SUBDIRS += bool_plperl bytea_plperl hstore_plperl jsonb_plperl
endif
ifeq ($(with_python),yes)
diff --git a/contrib/bytea_plperl/Makefile b/contrib/bytea_plperl/Makefile
new file mode 100644
index 00..1814d2f418
--- /dev/null
+++ b/contrib/bytea_plperl/Makefile
@@ -0,0 +1,39 @@
+# contrib/bytea_plperl/Makefile
+
+MODULE_big = bytea_plperl
+OBJS = \
+ $(WIN32RES) \
+ bytea_plperl.o
+PGFILEDESC = "bytea_plperl - bytea transform for plperl"
+
+PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plperl
+
+EXTENSION = bytea_plperlu bytea_plperl
+DATA = bytea_plperlu--1.0.sql bytea_plperl--1.0.sql
+
+REGRESS = bytea_plperl bytea_plperlu
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/bytea_plperl
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+# We must link libperl explicitly
+ifeq ($(PORTNAME), win32)
+# these settings are the same as for plperl
+override CPPFLAGS += -DPLPERL_HAVE_UID_GID -Wno-comment
+# ... see silliness in plperl Makefile ...
+SHLIB_LINK_INTERNAL += $(sort $(wildcard ../../src/pl/plperl/libperl*.a))
+else
+rpathdir = $(perl_archlibexp)/CORE
+SHLIB_LINK += $(perl_embed_ldflags)
+endif
+
+# As with plperl we need to include the perl_includespec directory last.
+override CPPFLAGS := $(CPPFLAGS) $(perl_embed_ccflags) $(perl_includespec)
diff --git a/contrib/bytea_plperl/bytea_plperl--1.0.sql b/contrib/bytea_plperl/bytea_plperl--1.0.sql
new file mode 100644
index 00..6544b2ac85
--- /dev/null
+++ b/contrib/bytea_plperl/bytea_plperl--1.0.sql
@@ -0,0 +1,19 @@
+/* contrib/bytea_plperl/bytea_plperl--1.0.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION bytea_plperl" to load this file. \quit
+
+CREATE FUNCTION bytea_to_plperl(val internal) RETURNS internal
+LANGUAGE C STRICT IMMUTABLE
+AS 'MODULE_PATHNAME';
+
+CREATE FUNCTION plperl_to_bytea(val internal) RETURNS bytea
+LANGUAGE C STRICT IMMUTABLE
+AS 'MODULE_PATHNAME';
+
+CREATE TRANSFORM FOR bytea LANGUAGE plperl (
+FROM SQL WITH FUNCTION bytea_to_plperl(internal),
+TO SQL WITH FUNCTION plperl_to_bytea(internal)
+);
+
+COMMENT ON TRANSFORM FOR bytea LANGUAGE plperl IS 'transform between bytea and Perl';
diff --git a/contrib/bytea_plperl/bytea_plperl.c b/contrib/bytea_plperl/bytea_plperl.c
new file mode 100644
index 00..0e20761b69
--- /dev/null
+++ b/contrib/bytea_plperl/bytea_plperl.c
@@ -0,0 +1,36 @@
+#include "postgres.h"
+
+#include "fmgr.h"
+#include "plperl.h"
+#include "varatt.h"
+
+PG_MODULE_MAGIC;
+
+PG_FUNCTION_INFO_V1(bytea_to_plperl);
+
+Datum
+bytea_to_plperl(PG_FUNCTION_ARGS)
+{
+ dTHX;
+ bytea *in = PG_GETARG_BYTEA_PP(0);
+ return PointerGetDatum(newSVpvn_flags( (char *) VARDATA_ANY(in), VARSIZE_ANY_EXHDR(in), 0 ));
+}
+
+
+PG_FUNCTION_INFO_V1(plperl_to_bytea);
+
+Datum
+plperl_to_bytea(PG_FUNCTION_ARGS)
+{
+ dTHX;
+ bytea *result;
+ STRLEN len;
+ SV *in = (SV *) PG_GETARG_POINTER(0);
+ char *ptr = SvPVbyte(in, len);
+ result = palloc(VARHDRSZ + len );
+ SET_VARSIZE(result, VARHDRSZ + len );
+ memcpy(VARDATA_ANY(result), ptr,len );
+ PG_RETURN_BYTEA_P(result);
+}
+
+
diff --git a/contrib/bytea_plperl/bytea_plperl.control b/contrib/bytea_plperl/bytea_plperl.control
new file mode 100644
index 00..9ff0f2a8dd
--- /dev/null
+++ b/contrib/bytea_plperl/bytea_plperl.control
@@ -0,0 +1,7 @@
+# bytea_plperl extension
+comment = 'transform between bytea and plperl'
+default_version = '1.0'
+module_pathname = '$libdir/bytea_plperl'
+relocatable = true
+trusted = true
+requires = 'plperl'
diff --git a/contrib/bytea_plperl/bytea_plperlu--1.0.sql b/contrib/bytea_plperl/bytea_plperlu--1.0.sql
new file mode 100644
index 00..d43e8fbaf4
--- /dev/null
+++ b/contrib/bytea_plperl/bytea_plperlu--1.0.sql
@@ -0,0 +1,19 @@
+/* contrib/bytea_plperl/bytea_plperlu--1.0.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION bytea_plperlu" to load this file. \quit
+
+CREATE FUNCTION bytea_to_plperlu(val internal) RETURNS internal
+LANGUAGE C STRICT IMMUTABLE
+AS 'MODULE_PATHNAME', 'bytea_to_plperl';
+
+CREATE FUNCTION plperlu_to_bytea(val internal) RETURNS bytea
Re: Making background psql nicer to use in tap tests
On 2023-03-17 Fr 18:58, Andres Freund wrote:
Hi,
On 2023-03-17 12:25:14 -0400, Andrew Dunstan wrote:
On 2023-03-17 Fr 10:08, Daniel Gustafsson wrote:
If we are going to keep this as a separate package, then we should put some
code in the constructor to prevent it being called from elsewhere than the
Cluster package. e.g.
# this constructor should only be called from PostgreSQL::Test::Cluster
my ($package, $file, $line) = caller;
die "Forbidden caller of constructor: package: $package, file:
$file:$line"
unless $package eq 'PostgreSQL::Test::Cluster';
I don't have strong feelings about where to place this, but Cluster.pm is
already quite long so I see a small upside to keeping it separate to not make
that worse.
Yeah, I can go along with that.
Cool - I'd prefer a separate file. I do find Cluster.pm somewhat unwieldy at
this point, and I susect that we'll end up with additional helpers around
BackgroundPsql.
Yeah. BTW, a better test than the one above would be
$package->isa("PostgreSQL::Test::Cluster")
cheers
andrew
--
Andrew Dunstan
EDB:https://www.enterprisedb.com
Re: buildfarm + meson
On 2023-03-11 Sa 16:25, Andres Freund wrote: Hi, On 2023-03-09 18:31:10 -0500, Andrew Dunstan wrote: Another thing: the test for uuid.h is too strict. On Fedora 36 the OSSP header is in /usr/include, not /usr/include/ossp (I got around that for now by symlinking it, but obviously that's a nasty hack we can't ask people to do) Yea, that was just wrong. It happened to work on debian and a few other OSs, but ossp's .pc puts whatever the right directory is into the include path. Pushed the fairly obvious fix. Another issue: building plpython appears impossible on Windows because it's finding meson's own python: Program python3 found: YES (C:\Program Files\Meson\meson.exe runpython) Could not find Python3 library 'C:\\Program Files\\Meson\\libs\\python311.lib' cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: Commitfest 2023-03 starting tomorrow!
On Sat, Mar 18, 2023 at 1:26 PM Alvaro Herrera wrote: > At this point, I'm going to suggest that reviewers should be open to the > idea of applying a submitted patch to some older Git commit in order to > review it. If we have given feedback, then it's OK to put a patch as > "waiting on author" and eventually boot it; but if we have not given > feedback, and there is no reason to think that the merge conflicts some > how make the patch fundamentally obsolete, then we should *not* set it > Waiting on Author. After all, it is quite easy to "git checkout" a > slightly older tree to get the patch to apply cleanly and review it > there. It seems plausible that improved tooling that makes it quick and easy to test a given patch locally could improve things for everybody. It's possible to do a git checkout to a slightly older tree today, of course. But in practice it's harder than it really should be. It would be very nice if there was an easy way to fetch from a git remote, and then check out a branch with a given patch applied on top of the "last known good git tip" commit. The tricky part would be systematically tracking which precise master branch commit is the last known "good commit" for a given CF entry. That seems doable to me. I suspect that removing friction when it comes to testing a patch locally (often just "kicking the tires" of a patch) could have an outsized impact. If something is made extremely easy, and requires little or no context to get going with, then people tend to do much more of it. Even when they theoretically don't have a good reason to do so. And even when they theoretically already had a good reason to do so, before the improved tooling/workflow was in place. -- Peter Geoghegan
Re: Commitfest 2023-03 starting tomorrow!
On 2023-Mar-17, Greg Stark wrote: > I'm going to go ahead and do this today. Any of these patches that are > "Waiting on Author" and haven't received any emails or status changes > since March 1 I'm going to move out of the commitfest(*). So I've come around to thinking that booting patches out of commitfest is not really such a great idea. It turns out that the number of active patch submitters seems to have reached a peak during the Postgres 12 timeframe, and has been steadily decreasing since then; and I think this is partly due to frustration caused by our patch process. It turns out that we expect that contributors will keep the patches the submit up to date, rebasing over and over for months on end, with no actual review occurring, and if this rebasing activity stops for a few weeks, we boot these patches out. This is demotivating: people went great lengths to introduce themselves to our admittedly antiquated process (no pull requests, remember), we gave them no feedback, and then we reject their patches with no further effort? I think this is not good. At this point, I'm going to suggest that reviewers should be open to the idea of applying a submitted patch to some older Git commit in order to review it. If we have given feedback, then it's OK to put a patch as "waiting on author" and eventually boot it; but if we have not given feedback, and there is no reason to think that the merge conflicts some how make the patch fundamentally obsolete, then we should *not* set it Waiting on Author. After all, it is quite easy to "git checkout" a slightly older tree to get the patch to apply cleanly and review it there. Authors should, of course, be encouraged to keep patches conflict-free, but this should not be a hard requirement. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Re: Infinite Interval
Joseph Koshakow writes:
> On Sat, Mar 18, 2023 at 3:08 PM Tom Lane wrote:
>> More specifically, those are from running pg_indent with an obsolete
>> typedefs list.
> I must be doing something wrong because even after doing that I get the
> same strange formatting. Specifically from the root directory I ran
Hmm, I dunno what's going on there. When I do this:
> curl https://buildfarm.postgresql.org/cgi-bin/typedefs.pl -o
> src/tools/pgindent/typedefs.list
I end up with a plausible set of updates, notably
$ git diff
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 097f42e1b3..667f8e13ed 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
...
@@ -545,10 +548,12 @@ DataDumperPtr
DataPageDeleteStack
DatabaseInfo
DateADT
+DateTimeErrorExtra
Datum
DatumTupleFields
DbInfo
DbInfoArr
+DbLocaleInfo
DeClonePtrType
DeadLockState
DeallocateStmt
so it sure ought to know DateTimeErrorExtra is a typedef.
I then tried pgindent'ing datetime.c and timestamp.c,
and it did not want to change either file. I do get
diffs like
DecodeDateTime(char **field, int *ftype, int nf,
int *dtype, struct pg_tm *tm, fsec_t *fsec, int *tzp,
- DateTimeErrorExtra *extra)
+ DateTimeErrorExtra * extra)
{
int fmask = 0,
if I try to pgindent datetime.c with typedefs.list as it
stands in HEAD. That's pretty much pgindent's normal
behavior when it doesn't recognize a name as a typedef.
regards, tom lane
Re: proposal: possibility to read dumped table's name from file
Hi
fresh rebase
regards
Pavel
From 09c64d9a5f2ed033c2691ca25ca6bec23320e1b0 Mon Sep 17 00:00:00 2001
From: "ok...@github.com"
Date: Thu, 16 Mar 2023 08:18:08 +0100
Subject: [PATCH] possibility to read options for dump from file
---
doc/src/sgml/ref/pg_dump.sgml | 110 +++
doc/src/sgml/ref/pg_dumpall.sgml| 22 +
doc/src/sgml/ref/pg_restore.sgml| 25 +
src/bin/pg_dump/Makefile| 5 +-
src/bin/pg_dump/filter.c| 489 ++
src/bin/pg_dump/filter.h| 56 ++
src/bin/pg_dump/meson.build | 2 +
src/bin/pg_dump/pg_dump.c | 117
src/bin/pg_dump/pg_dumpall.c| 61 +-
src/bin/pg_dump/pg_restore.c| 114
src/bin/pg_dump/t/005_pg_dump_filterfile.pl | 701
src/tools/msvc/Mkvcbuild.pm | 1 +
12 files changed, 1700 insertions(+), 3 deletions(-)
create mode 100644 src/bin/pg_dump/filter.c
create mode 100644 src/bin/pg_dump/filter.h
create mode 100644 src/bin/pg_dump/t/005_pg_dump_filterfile.pl
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index d6b1faa804..5672fbb273 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -829,6 +829,102 @@ PostgreSQL documentation
+
+ --filter=filename
+
+
+Specify a filename from which to read patterns for objects to include
+or exclude from the dump. The patterns are interpreted according to the
+same rules as the corresponding options:
+-t/--table,
+--table-and-children,
+--exclude-table-and-children or
+-T for tables,
+-n/--schema for schemas,
+--include-foreign-data for data on foreign servers and
+--exclude-table-data,
+--exclude-table-data-and-and-children for table data.
+To read from STDIN, use - as the
+filename. The --filter option can be specified in
+conjunction with the above listed options for including or excluding
+objects, and can also be specified more than once for multiple filter
+files.
+
+
+
+The file lists one object pattern per row, with the following format:
+
+{ include | exclude } { table | table_and_children | schema | foreign_data | table_data | table_data_and_children } PATTERN
+
+
+
+
+The first keyword specifies whether the objects matched by the pattern
+are to be included or excluded. The second keyword specifies the type
+of object to be filtered using the pattern:
+
+
+
+ table: tables, works like
+ -t/--table
+
+
+
+
+ table_and_children: tables, works like
+ -t/--table, except that
+ it also includes any partitions or inheritance child
+ tables of the table(s) matching the
+ pattern.
+
+
+
+
+ schema: schemas, works like
+ -n/--schema
+
+
+
+
+ foreign_data: data on foreign servers, works like
+ --include-foreign-data. This keyword can only be
+ used with the include keyword.
+
+
+
+
+ table_data: table data, works like
+ --exclude-table-data. This keyword can only be
+ used with the exclude keyword.
+
+
+
+
+ table_data_and_children: table data of any
+ partitions or inheritance child, works like
+ --exclude-table-data-and-children. This keyword can only be
+ used with the exclude keyword.
+
+
+
+
+
+
+
+Lines starting with # are considered comments and
+ignored. Comments can be placed after filter as well. Blank lines
+are also ignored. See for how to
+perform quoting in patterns.
+
+
+
+Example files are listed below in the
+section.
+
+
+
+
+
--if-exists
@@ -1159,6 +1255,7 @@ PostgreSQL documentation
schema (-n/--schema) and
table (-t/--table) pattern
match at least one extension/schema/table in the database to be dumped.
+This also applies to filters used with --filter.
Note that if none of the extension/schema/table patterns find
matches, pg_dump will generate an error
even without --strict-names.
@@ -1581,6 +1678,19 @@ CREATE DATABASE foo WITH TEMPLATE template0;
$ pg_dump -t "\"MixedCaseName\"" mydb mytab.sql
+
+
+
+ To dump all tables with names starting with mytable, except for table
+ mytable2, specify a filter file
+ filter.txt like:
+
+include table mytable*
+exclude table mytable2
+
+
+
+$ pg_dump
Re: Infinite Interval
On Sat, Mar 18, 2023 at 3:08 PM Tom Lane wrote:
> Joseph Koshakow writes:
>> On Thu, Mar 9, 2023 at 12:42 PM Ashutosh Bapat <
ashutosh.bapat@gmail.com>
>> wrote:
>>> There are a lot of these diffs. PG code doesn't leave an extra space
>>> between variable name and *.
>
>> Those appeared from running pg_indent. I've removed them all.
>
> More specifically, those are from running pg_indent with an obsolete
> typedefs list. Good practice is to fetch an up-to-date list from
> the buildfarm:
>
> curl https://buildfarm.postgresql.org/cgi-bin/typedefs.pl -o
.../typedefs.list
>
> and use that. (If your patch adds any typedefs, you can then add them
> to that list.) There's been talk of trying harder to keep
> src/tools/pgindent/typedefs.list up to date, but not much has happened
> yet.
I must be doing something wrong because even after doing that I get the
same strange formatting. Specifically from the root directory I ran
curl https://buildfarm.postgresql.org/cgi-bin/typedefs.pl -o
src/tools/pgindent/typedefs.list
src/tools/pgindent/pgindent src/backend/utils/adt/datetime.c
src/include/common/int.h src/backend/utils/adt/timestamp.c
src/backend/utils/adt/date.c src/backend/utils/adt/formatting.c
src/backend/utils/adt/selfuncs.c src/include/datatype/timestamp.h
src/include/utils/timestamp.h
>The specific issue with float zero is that plus zero and minus zero
>are distinct concepts with distinct bit patterns, but the IEEE spec
>says that they compare as equal. The C standard says about "if":
>
> [#1] The controlling expression of an if statement shall
> have scalar type.
> [#2] In both forms, the first substatement is executed if
> the expression compares unequal to 0. In the else form, the
> second substatement is executed if the expression compares
> equal to 0.
>
>so it sure looks to me like a float control expression is valid and
>minus zero should be treated as "false". Nonetheless, personally
>I'd consider this to be poor style and would write "r != 0" or
>"r != 0.0" rather than depending on that.
Thanks for the info, I've updated the three instances of the check to
be "r != 0.0"
>BTW, this may already need a rebase over 75bd846b6.
The patches in this email should be rebased over master.
- Joe Koshakow
From da22f9b3d55433c408f04056eecf0fddf60f01c9 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 18 Mar 2023 12:38:58 -0400
Subject: [PATCH 2/3] Check for overflow in make_interval
---
src/backend/utils/adt/timestamp.c | 24 +++-
src/include/common/int.h | 13 +
src/test/regress/expected/interval.out | 5 +
src/test/regress/sql/interval.sql | 4
4 files changed, 41 insertions(+), 5 deletions(-)
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index aaadc68ae6..b79af28ae3 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -1517,13 +1517,27 @@ make_interval(PG_FUNCTION_ARGS)
errmsg("interval out of range")));
result = (Interval *) palloc(sizeof(Interval));
- result->month = years * MONTHS_PER_YEAR + months;
- result->day = weeks * 7 + days;
+ result->month = months;
+ if (pg_mul_add_s32_overflow(years, MONTHS_PER_YEAR, >month))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("interval out of range")));
+ result->day = days;
+ if (pg_mul_add_s32_overflow(weeks, 7, >day))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("interval out of range")));
secs = rint(secs * USECS_PER_SEC);
- result->time = hours * ((int64) SECS_PER_HOUR * USECS_PER_SEC) +
- mins * ((int64) SECS_PER_MINUTE * USECS_PER_SEC) +
- (int64) secs;
+ result->time = secs;
+ if (pg_mul_add_s64_overflow(mins, ((int64) SECS_PER_MINUTE * USECS_PER_SEC), >time))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("interval out of range")));
+ if (pg_mul_add_s64_overflow(hours, ((int64) SECS_PER_HOUR * USECS_PER_SEC), >time))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("interval out of range")));
PG_RETURN_INTERVAL_P(result);
}
diff --git a/src/include/common/int.h b/src/include/common/int.h
index 81726c65f7..48ef495551 100644
--- a/src/include/common/int.h
+++ b/src/include/common/int.h
@@ -154,6 +154,19 @@ pg_mul_s32_overflow(int32 a, int32 b, int32 *result)
#endif
}
+/*
+ * Add val * multiplier to *sum.
+ * Returns false if successful, true on overflow.
+ */
+static inline bool
+pg_mul_add_s32_overflow(int32 val, int32 multiplier, int32 *sum)
+{
+ int32 product;
+
+ return pg_mul_s32_overflow(val, multiplier, ) ||
+ pg_add_s32_overflow(*sum, product, sum);
+}
+
/*
* INT64
*/
diff --git a/src/test/regress/expected/interval.out b/src/test/regress/expected/interval.out
index 28b71d9681..27bfb8ba9b 100644
---
Re: Infinite Interval
Joseph Koshakow writes: > On Thu, Mar 9, 2023 at 12:42 PM Ashutosh Bapat > wrote: >> There are a lot of these diffs. PG code doesn't leave an extra space >> between variable name and *. > Those appeared from running pg_indent. I've removed them all. More specifically, those are from running pg_indent with an obsolete typedefs list. Good practice is to fetch an up-to-date list from the buildfarm: curl https://buildfarm.postgresql.org/cgi-bin/typedefs.pl -o .../typedefs.list and use that. (If your patch adds any typedefs, you can then add them to that list.) There's been talk of trying harder to keep src/tools/pgindent/typedefs.list up to date, but not much has happened yet. > I've separated this out into another patch attached to this email. > Should I start a new email thread or is it ok to include it in this > one? Having separate threads with interdependent patches is generally a bad idea :-( ... the cfbot certainly won't cope. >> I see that this code is very similar to the corresponding code in >> timestamp and >> timestamptz, so it's bound to be correct. But I always thought float >> equality >> is unreliable. if (r) is equivalent to if (r == 0.0) so it will not >> work as >> intended. But may be (float) 0.0 is a special value for which equality >> holds >> true. > I'm not familiar with float equality being unreliable, but I'm by no > means a C or float expert. Can you link me to some docs/explanation? The specific issue with float zero is that plus zero and minus zero are distinct concepts with distinct bit patterns, but the IEEE spec says that they compare as equal. The C standard says about "if": [#1] The controlling expression of an if statement shall have scalar type. [#2] In both forms, the first substatement is executed if the expression compares unequal to 0. In the else form, the second substatement is executed if the expression compares equal to 0. so it sure looks to me like a float control expression is valid and minus zero should be treated as "false". Nonetheless, personally I'd consider this to be poor style and would write "r != 0" or "r != 0.0" rather than depending on that. BTW, this may already need a rebase over 75bd846b6. regards, tom lane
Re: Option to not use ringbuffer in VACUUM, using it in failsafe mode
> Subject: [PATCH v4 3/3] add vacuum[db] option to specify ring_size and guc
> +Specifies the ring buffer size to be used for a given invocation of
> +VACUUM or instance of autovacuum. This size is
> +converted to a number of shared buffers which will be reused as part
> of
I'd say "specifies the size of shared_buffers to be reused as .."
> +a Buffer Access Strategy. 0
> will
> +disable use of a Buffer Access Strategy.
> +-1 will set the size to a default of 256
> +kB. The maximum ring buffer size is 16
> GB.
> +Though you may set vacuum_buffer_usage_limit below
> +128 kB, it will be clamped to 128
> +kB at runtime. The default value is -1.
> +This parameter can be set at any time.
GUC docs usually also say something like
"If this value is specified without units, it is taken as .."
> + is used to calculate a number of shared buffers which will be reused as
*the* number?
> + VACUUM. The analyze stage and parallel vacuum
> workers
> + do not use this size.
I think what you mean is that vacuum's heap scan stage uses the
strategy, but the index scan/cleanup phases doesn't?
> +The size in kB of the ring buffer used for vacuuming. This size is
> used
> +to calculate a number of shared buffers which will be reused as part
> of
*the* number
> +++ b/doc/src/sgml/ref/vacuumdb.sgml
The docs here duplicate the sql-vacuum docs. It seems better to refer
to the vacuum page for details, like --parallel does.
Unrelated: it would be nice if the client-side options were documented
separately from the server-side options. Especially due to --jobs and
--parallel.
> + if (!parse_int(vac_buffer_size, , GUC_UNIT_KB,
> NULL))
> + {
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> +
> errmsg("buffer_usage_limit for a vacuum must be between -1 and %d. %s is
> invalid.",
> +
> MAX_BAS_RING_SIZE_KB, vac_buffer_size),
> +
> parser_errposition(pstate, opt->location)));
> + }
> +
> + /* check for out-of-bounds */
> + if (result < -1 || result > MAX_BAS_RING_SIZE_KB)
> + {
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> +
> errmsg("buffer_usage_limit for a vacuum must be between -1 and %d",
> +
> MAX_BAS_RING_SIZE_KB),
> +
> parser_errposition(pstate, opt->location)));
> + }
I think these checks could be collapsed into a single ereport().
if !parse_int() || (result < -1 || result > MAX_BAS_RINGSIZE_KB):
ereport(ERROR,
errcode(ERRCODE_SYNTAX_ERROR),
errmsg("buffer_usage_limit for a vacuum must be an integer
between -1 and %d",
MAX_BAS_RING_SIZE_KB),
There was a recent, similar, and unrelated suggestion here:
https://www.postgresql.org/message-id/20230314.135859.260879647537075548.horikyota.ntt%40gmail.com
> +#vacuum_buffer_usage_limit = -1 # size of vacuum buffer access strategy ring.
> + # -1 to use default,
> + # 0 to disable vacuum buffer access strategy
> and use shared buffers
I think it's confusing to say "and use shared buffers", since
"strategies" also use shared_buffers. It seems better to remove those 4
words.
> @@ -550,6 +563,13 @@ vacuum_one_database(ConnParams *cparams,
> pg_fatal("cannot use the \"%s\" option on server versions older
> than PostgreSQL %s",
>"--parallel", "13");
>
> + // TODO: this is a problem: if the user specifies this option with -1
> in a
> + // version before 16, it will not produce an error message. it also
> won't
> + // do anything, but that still doesn't seem right.
Actually, that seems fine to me. If someone installs v16 vacuumdb, they
can run it against old servers and specify the option as -1 without it
failing with an error. I don't know if anyone will find that useful,
but it doesn't seem unreasonable.
I still think adding something to the glossary would be good.
Buffer Access Strategy:
A circular/ring buffer used for reading or writing data pages from/to
the operating system. Ring buffers are used for sequential scans of
large tables, VACUUM, COPY FROM, CREATE TABLE AS SELECT, ALTER TABLE,
and CLUSTER. By using only a limited portion of >shared_buffers<, the
ring buffer avoids avoids evicting large amounts of data
Re: generate_series for timestamptz and time zone problem
Pushed v7 after making a bunch of cosmetic changes. One gripe I had was that rearranging the logic in timestamptz_pl_interval[_internal] made it nearly impossible to see what functional changes you'd made there, while not really buying anything in return. I undid that to make the diff readable. I did not push the fmgr.h changes. Maybe that is worthwhile (although I'd vote against it), but it certainly does not belong in a localized feature patch. regards, tom lane
Re: proposal: psql: psql variable BACKEND_PID
so 18. 3. 2023 v 16:24 odesílatel Andrew Dunstan napsal: > > On 2023-02-16 Th 23:04, Pavel Stehule wrote: > > > > čt 16. 2. 2023 v 12:49 odesílatel Jelte Fennema napsal: > >> On Thu, 16 Feb 2023 at 12:44, Pavel Stehule >> wrote: >> > To find and use pg_backend_pid is not rocket science. But use >> :BACKEND_PID is simpler. >> >> I wanted to call out that if there's a connection pooler (e.g. >> PgBouncer) in the middle, then BACKEND_PID (and %p) are incorrect, but >> pg_backend_pid() would work for the query. This might be an edge case, >> but if BACKEND_PID is added it might be worth listing this edge case >> in the docs somewhere. >> > > good note > > > > This patch is marked RFC, but given the comments upthread from Tom, Andres > and Peter, I think it should actually be Rejected. > ok regards Pavel > > cheers > > > andrew > > -- > Andrew Dunstan > EDB: https://www.enterprisedb.com > >
Re: proposal: psql: psql variable BACKEND_PID
On 2023-02-16 Th 23:04, Pavel Stehule wrote: čt 16. 2. 2023 v 12:49 odesílatel Jelte Fennema napsal: On Thu, 16 Feb 2023 at 12:44, Pavel Stehule wrote: > To find and use pg_backend_pid is not rocket science. But use :BACKEND_PID is simpler. I wanted to call out that if there's a connection pooler (e.g. PgBouncer) in the middle, then BACKEND_PID (and %p) are incorrect, but pg_backend_pid() would work for the query. This might be an edge case, but if BACKEND_PID is added it might be worth listing this edge case in the docs somewhere. good note This patch is marked RFC, but given the comments upthread from Tom, Andres and Peter, I think it should actually be Rejected. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: logical decoding and replication of sequences, take 2
On 3/18/23 06:35, Amit Kapila wrote: > On Sat, Mar 18, 2023 at 3:13 AM Tomas Vondra > wrote: >> >> ... >> >> Clearly, for sequences we can't quite rely on snapshots/slots, we need >> to get the LSN to decide what changes to apply/skip from somewhere else. >> I wonder if we can just ignore the queued changes in tablesync, but I >> guess not - there can be queued increments after reading the sequence >> state, and we need to apply those. But maybe we could use the page LSN >> from the relfilenode - that should be the LSN of the last WAL record. >> >> Or maybe we could simply add pg_current_wal_insert_lsn() into the SQL we >> use to read the sequence state ... >> > > What if some Alter Sequence is performed before the copy starts and > after the copy is finished, the containing transaction rolled back? > Won't it copy something which shouldn't have been copied? > That shouldn't be possible - the alter creates a new relfilenode and it's invisible until commit. So either it gets committed (and then replicated), or it remains invisible to the SELECT during sync. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: BF mamba failure
Hi,
18.03.2023 07:26, Tom Lane wrote:
Amit Kapila writes:
Peter Smith has recently reported a BF failure [1]. AFAICS, the call
stack of failure [2] is as follows:
Note the assertion report a few lines further up:
TRAP: failed Assert("pg_atomic_read_u32(_ref->shared_entry->refcount) == 0"), File:
"pgstat_shmem.c", Line: 560, PID: 25004
This assertion failure can be reproduced easily with the attached patch:
== running regression test queries ==
test oldest_xmin ... ok 55 ms
test oldest_xmin ... FAILED (test process exited with exit
code 1) 107 ms
test oldest_xmin ... FAILED (test process exited with exit
code 1) 8 ms
== shutting down postmaster ==
contrib/test_decoding/output_iso/log/postmaster.log contains:
TRAP: failed Assert("pg_atomic_read_u32(_ref->shared_entry->refcount) == 0"), File: "pgstat_shmem.c", Line: 561,
PID: 456844
With the sleep placed above Assert(entry_ref->shared_entry->dropped) this
Assert fails too.
Best regards,
Alexanderdiff --git a/contrib/test_decoding/Makefile b/contrib/test_decoding/Makefile
index c7ce603706..9b3389a90e 100644
--- a/contrib/test_decoding/Makefile
+++ b/contrib/test_decoding/Makefile
@@ -9,6 +9,7 @@ REGRESS = ddl xact rewrite toast permissions decoding_in_xact \
ISOLATION = mxact delayed_startup ondisk_startup concurrent_ddl_dml \
oldest_xmin snapshot_transfer subxact_without_top concurrent_stream \
twophase_snapshot slot_creation_error catalog_change_snapshot
+ISOLATION = oldest_xmin oldest_xmin oldest_xmin
REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/test_decoding/logical.conf
ISOLATION_OPTS = --temp-config $(top_srcdir)/contrib/test_decoding/logical.conf
diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c
index 09fffd0e82..5307116670 100644
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -551,6 +551,7 @@ pgstat_release_entry_ref(PgStat_HashKey key, PgStat_EntryRef *entry_ref,
/* only dropped entries can reach a 0 refcount */
Assert(entry_ref->shared_entry->dropped);
+pg_usleep(10L);
shent = dshash_find(pgStatLocal.shared_hash,
_ref->shared_entry->key,
true);
Re: Initial Schema Sync for Logical Replication
On 2023-Mar-15, Kumar, Sachin wrote: > 1. In CreateSubscription() when we create replication > slot(walrcv_create_slot()), should > use CRS_EXPORT_SNAPSHOT, So that we can use this snapshot later in the > pg_dump. > > 2. Now we can call pg_dump with above snapshot from CreateSubscription. Overall I'm not on board with the idea that logical replication would depend on pg_dump; that seems like it could run into all sorts of trouble (what if calling external binaries requires additional security setup? what about pg_hba connection requirements? what about max_connections in tight circumstances?). It would be much better, I think, to handle this internally in the publisher instead: similar to how DDL sync would work, except it'd somehow generate the CREATE statements from the existing tables instead of waiting for DDL events to occur. I grant that this does require writing a bunch of new code for each object type, a lot of which would duplicate the pg_dump logic, but it would probably be a lot more robust. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Re: [PATCH] Clarify the behavior of the system when approaching XID wraparound
Thanks for picking up this badly-needed topic again! I was irresponsible last year and let it fall off my radar, but I'm looking at the patches, as well as revisiting discussions from the last four (!?) years that didn't lead to action. 0001: +In this condition the system can still execute read-only transactions. +The active transactions will continue to execute and will be able to +commit. This is ambiguous. I'd first say that any transactions already started can continue, and then say that only new read-only transactions can be started. 0004: -HINT: Stop the postmaster and vacuum that database in single-user mode. +HINT: VACUUM or VACUUM FREEZE that database. VACUUM FREEZE is worse and should not be mentioned, since it does unnecessary work. Emergency vacuum is not school -- you don't get extra credit for doing unnecessary work. Also, we may consider adding a boxed NOTE warning specifically against single-user mode, especially if this recommendation will change in at least some minor releases so people may not hear about it. See also [1]. - * If we're past xidStopLimit, refuse to execute transactions, unless - * we are running in single-user mode (which gives an escape hatch - * to the DBA who somehow got past the earlier defenses). + * If we're past xidStopLimit, refuse to allocate new XIDs. This patch doesn't completely get rid of the need for single-user mode, so it should keep all information about it. If a DBA wanted to e.g. drop or truncate a table to save vacuum time, it is still possible to do it in single-user mode, so the escape hatch is still useful. In swapping this topic back in my head, I also saw [2] where Robert suggested "that old prepared transactions and stale replication slots should be emphasized more prominently. Maybe something like: HINT: Commit or roll back old prepared transactions, drop stale replication slots, or kill long-running sessions. Ensure that autovacuum is progressing, or run a manual database-wide VACUUM." That sounds like a good direction to me. There is more we could do here to make the message more specific [3][4][5], but the patches here are in the right direction. Note for possible backpatching: It seems straightforward to go back to PG14, which has the failsafe, but we should have better testing in place first. There is a patch in this CF to make it easier to get close to wraparound, so I'll look at what it does as well. [1] https://www.postgresql.org/message-id/CA%2BTgmoadjx%2Br8_gGbbnNifL6vEyjZntiQRPzyixrUihvtZ5jdQ%40mail.gmail.com [2] https://www.postgresql.org/message-id/CA+Tgmob1QCMJrHwRBK8HZtGsr+6cJANRQw2mEgJ9e=d+z7c...@mail.gmail.com [3] https://www.postgresql.org/message-id/20190504023015.5mgpbl27tld4irw5%40alap3.anarazel.de [4] https://www.postgresql.org/message-id/20220204013539.qdegpqzvayq3d4y2%40alap3.anarazel.de [5] https://www.postgresql.org/message-id/20220220045757.GA3733812%40rfd.leadboat.com -- John Naylor EDB: http://www.enterprisedb.com
Re: Allow logical replication to copy tables in binary format
On Sat, Mar 18, 2023 at 3:11 PM Peter Smith wrote:
>
> On Fri, Mar 17, 2023 at 11:25 PM Melih Mutlu wrote:
> ==
> src/backend/replication/logical/tablesync.c
>
> 2.
> @@ -1168,6 +1170,15 @@ copy_table(Relation rel)
>
> appendStringInfoString(, ") TO STDOUT");
> }
> +
> + /* The binary option for replication is supported since v16 */
> + if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 16 &&
> + MySubscription->binary)
> + {
> + appendStringInfoString(, " WITH (FORMAT binary)");
> + options = lappend(options, makeDefElem("format", (Node *)
> makeString("binary"), -1));
> + }
>
>
> Logical replication binary mode was introduced in v14, so the old
> comment ("The binary option for replication is supported since v14")
> was correct. Unfortunately, after changing the code check to 16000, I
> think the new comment ("The binary option for replication is supported
> since v16") became incorrect, and so it needs some rewording. Maybe it
> should say something like below:
>
> SUGGESTION
> If the publisher is v16 or later, then any initial table
> synchronization will use the same format as specified by the
> subscription binary mode. If the publisher is before v16, then any
> initial table synchronization will use text format regardless of the
> subscription binary mode.
>
I agree that the previous comment should be updated but I would prefer
something along the lines: "Prior to v16, initial table
synchronization will use text format even if the binary option is
enabled for a subscription."
--
With Regards,
Amit Kapila.
Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher
On Thu, Mar 16, 2023 at 2:15 PM Amit Kapila wrote: > > On Wed, Mar 15, 2023 at 9:12 AM Amit Kapila wrote: > > > > On Tue, Mar 14, 2023 at 3:18 PM Önder Kalacı wrote: > > >> > > > > Pushed this patch but forgot to add a new testfile. Will do that soon. > > > > The main patch is committed now. I think the pending item in this > thread is to conclude whether we need a storage or subscription to > disable/enable this feature. Both Andres and Onder don't seem to be in > favor but I am of opinion that it could be helpful in scenarios where > the index scan (due to duplicates or dead tuples) is slower. However, > if we don't have a consensus on the same, we can anyway add it later. > If there are no more votes in favor of adding such an option, we can > probably close the CF entry. > I have closed this CF entry for now. However, if there is any interest in pursuing the storage or subscription option for this feature, we can still discuss it. -- With Regards, Amit Kapila.
Re: Allow logical replication to copy tables in binary format
On Fri, Mar 17, 2023 at 11:25 PM Melih Mutlu wrote:
>
> Hi,
>
> Sharing v17.
>
> Amit Kapila , 17 Mar 2023 Cum, 03:02 tarihinde şunu
> yazdı:
>>
>> I think to reduce the risk of breakage, let's change the check to
>> >=v16. Also, accordingly, update the doc and commit message.
>
>
> Done.
>
Here are my review comments for v17-0001
==
Commit message
1.
Binary copy is supported for v16 or later.
~
As written that's very general and not quite correct. E.g. COPY ...
WITH (FORMAT binary) has been available for a long time. IMO that
commit message sentence ought to be more specific.
SUGGESTION
Binary copy for logical replication table synchronization is supported
only when both publisher and subscriber are v16 or later.
==
src/backend/replication/logical/tablesync.c
2.
@@ -1168,6 +1170,15 @@ copy_table(Relation rel)
appendStringInfoString(, ") TO STDOUT");
}
+
+ /* The binary option for replication is supported since v16 */
+ if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 16 &&
+ MySubscription->binary)
+ {
+ appendStringInfoString(, " WITH (FORMAT binary)");
+ options = lappend(options, makeDefElem("format", (Node *)
makeString("binary"), -1));
+ }
Logical replication binary mode was introduced in v14, so the old
comment ("The binary option for replication is supported since v14")
was correct. Unfortunately, after changing the code check to 16000, I
think the new comment ("The binary option for replication is supported
since v16") became incorrect, and so it needs some rewording. Maybe it
should say something like below:
SUGGESTION
If the publisher is v16 or later, then any initial table
synchronization will use the same format as specified by the
subscription binary mode. If the publisher is before v16, then any
initial table synchronization will use text format regardless of the
subscription binary mode.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
Re: Should we remove vacuum_defer_cleanup_age?
On 2023-Mar-17, Andres Freund wrote: > I started writing a test for vacuum_defer_cleanup_age while working on the fix > referenced above, but now I am wondering if said energy would be better spent > removing vacuum_defer_cleanup_age alltogether. +1 I agree it's not useful anymore. > I don't think I have the cycles to push this through in the next weeks, but if > we agree removing vacuum_defer_cleanup_age is a good idea, it seems like a > good idea to mark it as deprecated in 16? Hmm, for the time being, can we just "disable" it by disallowing to set the GUC to a value different from 0? Then we can remove the code later in the cycle at leisure. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "La gente vulgar sólo piensa en pasar el tiempo; el que tiene talento, en aprovecharlo"
Re: Allow logical replication to copy tables in binary format
On Fri, 17 Mar 2023 at 17:55, Melih Mutlu wrote:
>
> Hi,
>
> Sharing v17.
>
> Amit Kapila , 17 Mar 2023 Cum, 03:02 tarihinde şunu
> yazdı:
>>
>> I think to reduce the risk of breakage, let's change the check to
>> >=v16. Also, accordingly, update the doc and commit message.
>
>
> Done.
>
> Peter Smith , 17 Mar 2023 Cum, 04:58 tarihinde şunu
> yazdı:
>>
>> IMO the sentence "However, logical replication in binary format is
>> more restrictive." should just be plain text.
>
>
> Done.
>
> shiy.f...@fujitsu.com , 17 Mar 2023 Cum, 05:26
> tarihinde şunu yazdı:
>>
>> It looks that you forgot to pass `offset` into wait_for_log().
>
>
> Yes, I somehow didn't include those lines into the patch. Thanks for
> noticing. Fixed them now.
Thanks for the updated patch, few comments:
1) Currently we refer the link to the beginning of create subscription
page, this can be changed to refer to binary option contents in create
subscription:
+
+ See the binary option of
+ CREATE
SUBSCRIPTION
+ for details about copying pre-existing data in binary format.
+
2) Running pgperltidy shows the test script 014_binary.pl could be
slightly improved as in the attachment.
Regards,
Vignesh
diff --git a/doc/src/sgml/logical-replication.sgml b/doc/src/sgml/logical-replication.sgml
index bad25e54cd..9c96f900b4 100644
--- a/doc/src/sgml/logical-replication.sgml
+++ b/doc/src/sgml/logical-replication.sgml
@@ -253,7 +253,7 @@
will be filled with the default value as specified in the definition of the
target table. However, logical replication in binary
format is more restrictive. See the binary option of
- CREATE SUBSCRIPTION
+ CREATE SUBSCRIPTION
for details.
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index ccd4ed6f59..fb0ddd2d7f 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -179,7 +179,7 @@ ALTER SUBSCRIPTION name RENAME TO <
See the binary option of
- CREATE SUBSCRIPTION
+ CREATE SUBSCRIPTION
for details about copying pre-existing data in binary format.
diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index e0be2b586f..edf52fa7d4 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -185,7 +185,7 @@ CREATE SUBSCRIPTION subscription_name
-
+
binary (boolean)
diff --git a/src/test/subscription/t/014_binary.pl b/src/test/subscription/t/014_binary.pl
index d20f94b8d3..f5fa5976e1 100644
--- a/src/test/subscription/t/014_binary.pl
+++ b/src/test/subscription/t/014_binary.pl
@@ -57,16 +57,18 @@ $node_publisher->safe_psql(
my $publisher_connstring = $node_publisher->connstr . ' dbname=postgres';
$node_subscriber->safe_psql('postgres',
- "CREATE SUBSCRIPTION tsub CONNECTION '$publisher_connstring' "
+ "CREATE SUBSCRIPTION tsub CONNECTION '$publisher_connstring' "
. "PUBLICATION tpub WITH (slot_name = tpub_slot, binary = true)");
# Ensure the COPY command is executed in binary format on the publisher
-$node_publisher->wait_for_log(qr/LOG: ( [A-Z0-9]+:)? statement: COPY (.+)? TO STDOUT WITH \(FORMAT binary\)/);
+$node_publisher->wait_for_log(
+ qr/LOG: ( [A-Z0-9]+:)? statement: COPY (.+)? TO STDOUT WITH \(FORMAT binary\)/
+);
# Ensure nodes are in sync with each other
$node_subscriber->wait_for_subscription_sync($node_publisher, 'tsub');
-my $sync_check = qq(
+my $sync_check = qq(
SELECT a, b, c, d FROM test_numerical ORDER BY a;
SELECT a, b, c FROM test_arrays ORDER BY a;
);
@@ -207,10 +209,12 @@ $node_publisher->safe_psql(
my $offset = -s $node_subscriber->logfile;
# Refresh the publication to trigger the tablesync
-$node_subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION tsub REFRESH PUBLICATION");
+$node_subscriber->safe_psql('postgres',
+ "ALTER SUBSCRIPTION tsub REFRESH PUBLICATION");
# It should fail
-$node_subscriber->wait_for_log(qr/ERROR: ( [A-Z0-9]+:)? no binary input function available for type/,
+$node_subscriber->wait_for_log(
+ qr/ERROR: ( [A-Z0-9]+:)? no binary input function available for type/,
$offset);
# Create and set send/rcv functions for the custom type
@@ -231,9 +235,10 @@ $node_subscriber->safe_psql('postgres', $ddl);
$node_subscriber->wait_for_subscription_sync($node_publisher, 'tsub');
# Check the synced data on the subscriber
-$result = $node_subscriber->safe_psql('postgres', 'SELECT a FROM test_myvarchar;');
+$result =
+ $node_subscriber->safe_psql('postgres', 'SELECT a FROM test_myvarchar;');
-is( $result, 'a', 'check synced data on subscriber with custom type');
+is($result, 'a', 'check synced data on subscriber with custom type');
# -
# Test mismatched column types with/without binary mode
@@ -241,7
[PATCH] Use RelationClose rather than table_close in heap_create_with_catalog
Hi hackers, In heap_create_with_catalog, the Relation new_rel_desc is created by RelationBuildLocalRelation, not table_open. So it's better to call RelationClose to release it. What's more, the comment for it seems useless, just delete it. Thanks! 0001-Use-RelationClose-rather-than-table_close-in-heap_cr.patch Description: 0001-Use-RelationClose-rather-than-table_close-in-heap_cr.patch
