Re: split TOAST support out of postgres.h
Hi, I've thought about this while working on Pluggable TOAST and 64-bit TOAST value ID myself. Agree with #2 to seem the best of the above. There are not so many places where a new header should be included. On Wed, Dec 28, 2022 at 6:08 PM Tom Lane wrote: > Peter Eisentraut writes: > > ... Then we could either > > > 1) Include varatt.h in postgres.h, similar to elog.h and palloc.h. That > > way we clean up the files a bit but don't change any external interfaces. > > > 2) Just let everyone who needs it include the new file. > > > 3) Compromise: You can avoid most "damage" by having fmgr.h include > > varatt.h. That satisfies most data types and extension code. That way, > > there are only a few places that need an explicit include of varatt.h. > > > I went with the last option in my patch. > > I dunno, #3 seems kind of unprincipled. Also, since fmgr.h is included > so widely, I doubt it is buying very much in terms of reducing header > footprint. How bad is it to do #2? > > regards, tom lane > > > -- Regards, Nikita Malakhov Postgres Professional https://postgrespro.ru/
Re: Avoiding unnecessary clog lookups while freezing
On Wed, Dec 28, 2022 at 4:43 PM Andres Freund wrote: > > > Hm. I dimply recall that we had repeated cases where the hint bits were > > > set > > > wrongly due to some of the multixact related bugs. I think I was trying > > > to be > > > paranoid about not freezing stuff in those situations, since it can lead > > > to > > > reviving dead tuples, which obviously is bad. > > > > I think that it's a reasonable check, and I'm totally in favor of > > keeping it (or something very close, at least). > > I don't quite follow - one paragraph up you say we should fix something, and > then here you seem to say we should continue not to rely on the hint bits? I didn't mean that we should continue to not rely on the hint bits. Is that really all that the test is for? I think of it as a general sanity check. The important call to avoid with page-level freezing is the xmin call to TransactionIdDidCommit(), not the xmax call. The xmax call only occurs when VACUUM prepares to freeze a tuple that was updated by an updater (not a locker) that aborted. While the xmin calls will now take place with most unfrozen tuples. -- Peter Geoghegan
psql: stop at error immediately during \copy
Hi, hackers.
psql \copy command can stream data from client host just like the normal
copy command can do on server host. Let's assume we want to stream a
local data file from psql:
pgsql =# \copy tbl from '/tmp/datafile' (format 'text');
If there's error inside the data file, \copy will still stream the
whole data file
before it reports the error. This is undesirable If the data file is very large,
or it's an infinite pipe. The normal copy command which reads file on the
server host can report error immediately as expected.
The problem seems to be pqParseInput3(). When error occurs in server
backend, it will send 'E' packet back to client. During \copy command, the
connection's asyncStatus is PGASYNC_COPY_IN, any 'E' packet will
get ignored by this path:
else if (conn->asyncStatus != PGASYNC_BUSY)
{
/* If not IDLE state, just wait ... */
if (conn->asyncStatus != PGASYNC_IDLE)
return;
So the client can't detect the error sent back by server.
I've attached a patch to demonstrate one way to workaround this. Save
the error via pqGetErrorNotice3() if the conn is PGASYNC_COPY_IN
status. The client code(psql) can detect the error via PQerrorMessage().
Probably still lots of details need to be considered but should be good
enough to start this discussion. Any thoughts on this issue?
Best regards
Peifeng Qiu
From a155adf6c1e6a6c31cf4146ce53827180247f384 Mon Sep 17 00:00:00 2001
From: Peifeng Qiu
Date: Thu, 29 Dec 2022 11:37:19 +0900
Subject: [PATCH] psql: stop at error immediately during \copy
---
src/bin/psql/copy.c | 7 +++
src/interfaces/libpq/fe-protocol3.c | 13 +
2 files changed, 20 insertions(+)
diff --git a/src/bin/psql/copy.c b/src/bin/psql/copy.c
index 0f66ebc2ed..1d285312c2 100644
--- a/src/bin/psql/copy.c
+++ b/src/bin/psql/copy.c
@@ -580,6 +580,7 @@ handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res)
bool copydone = false;
int buflen;
bool at_line_begin = true;
+ char *err;
/*
* In text mode, we have to read the input one line at a time, so that
@@ -660,6 +661,12 @@ handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res)
OK = false;
break;
}
+err = PQerrorMessage(conn);
+if (err && *err)
+{
+ /* We got error from server backend. Stop processing. */
+ break;
+}
buflen = 0;
}
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index 364bad2b88..b50ce7da2e 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -150,6 +150,19 @@ pqParseInput3(PGconn *conn)
if (pqGetErrorNotice3(conn, false))
return;
}
+ else if (conn->asyncStatus == PGASYNC_COPY_IN)
+ {
+ /*
+ * Process and save error message during COPY. Client can check
+ * this error via PQerrorMessage.
+ */
+ if (id == 'E')
+ {
+if (pqGetErrorNotice3(conn, true))
+ return;
+break;
+ }
+ }
else if (conn->asyncStatus != PGASYNC_BUSY)
{
/* If not IDLE state, just wait ... */
--
2.39.0
Re: BUG #17717: Regression in vacuumdb (15 is slower than 10/11 and possible memory issue)
Hi, On 2022-12-28 15:13:23 -0500, Tom Lane wrote: > [ redirecting to -hackers because patch attached ] > > Peter Geoghegan writes: > > On Fri, Dec 16, 2022 at 6:49 AM Tom Lane wrote: > >> That is a really good point. How about teaching VACUUM to track > >> the oldest original relfrozenxid and relminmxid among the table(s) > >> it processed, and skip vac_update_datfrozenxid unless at least one > >> of those matches the database's values? For extra credit, also > >> skip if we didn't successfully advance the source rel's value. > > > Hmm. I think that that would probably work. > > I poked into that idea some more and concluded that getting VACUUM to > manage it behind the user's back is not going to work very reliably. > The key problem is explained by this existing comment in autovacuum.c: > > * Even if we didn't vacuum anything, it may still be important to do > * this, because one indirect effect of vac_update_datfrozenxid() is to > * update ShmemVariableCache->xidVacLimit. That might need to be done > * even if we haven't vacuumed anything, because relations with older > * relfrozenxid values or other databases with older datfrozenxid values > * might have been dropped, allowing xidVacLimit to advance. > > That is, if the table that's holding back datfrozenxid gets dropped > between VACUUM runs, VACUUM would never think that it might have > advanced the global minimum. I wonder if a less aggressive version of this idea might still work. Perhaps we could use ShmemVariableCache->latestCompletedXid or ShmemVariableCache->nextXid to skip at least some updates? Obviously this isn't going to help if there's a lot of concurrent activity, but the case of just running vacuumdb -a might be substantially improved. Separately I wonder if it's worth micro-optimizing vac_update_datfrozenxid() a bit. I e.g. see a noticable speedup bypassing systable_getnext() and using heap_getnext(). It's really too bad that we want to check for "in the future" xids, otherwise we could use a ScanKey to filter at a lower level. Greetings, Andres Freund
Re: BUG #17717: Regression in vacuumdb (15 is slower than 10/11 and possible memory issue)
Justin Pryzby writes:
> On Wed, Dec 28, 2022 at 03:13:23PM -0500, Tom Lane wrote:
>> I'm forced to the conclusion that we have to expose some VACUUM
>> options if we want this to work well. Attached is a draft patch
>> that invents SKIP_DATABASE_STATS and ONLY_DATABASE_STATS options
>> (name bikeshedding welcome) and teaches vacuumdb to use them.
> I assumed it would look like:
> VACUUM (UPDATE_DATABASE_STATS {yes,no,only})
Meh. We could do it like that, but I think options that look like
booleans but aren't are messy.
regards, tom lane
Re: windows/meson cfbot warnings
Hi,
On 2022-12-28 18:35:38 -0600, Justin Pryzby wrote:
> Since a few days ago, the windows/meson task has been spewing messages for
> each tap
> test:
>
> | Unknown TAP version. The first line MUST be `TAP version `. Assuming
> version 12.
>
> I guess because the image is updated to use meson v1.0.0.
> https://github.com/mesonbuild/meson/commit/b7a5c384a1f1ba80c09904e7ef4f5160bdae3345
>
> mesonbuild/mtest.py-if version is None:
> mesonbuild/mtest.py:self.warnings.append('Unknown TAP version.
> The first line MUST be `TAP version `. Assuming version 12.')
> mesonbuild/mtest.py-version = 12
I think the change is somewhat likely to be reverted in the next meson minor
version. It apparently came about due to somewhat odd language in the tap-14
spec... So I think we should just wait for now.
Greetings,
Andres Freund
Re: BUG #17717: Regression in vacuumdb (15 is slower than 10/11 and possible memory issue)
On Wed, Dec 28, 2022 at 03:13:23PM -0500, Tom Lane wrote:
> Peter Geoghegan writes:
> > On Fri, Dec 16, 2022 at 6:49 AM Tom Lane wrote:
> >> That is a really good point. How about teaching VACUUM to track
> >> the oldest original relfrozenxid and relminmxid among the table(s)
> >> it processed, and skip vac_update_datfrozenxid unless at least one
> >> of those matches the database's values? For extra credit, also
> >> skip if we didn't successfully advance the source rel's value.
>
> > Hmm. I think that that would probably work.
>
> I'm forced to the conclusion that we have to expose some VACUUM
> options if we want this to work well. Attached is a draft patch
> that invents SKIP_DATABASE_STATS and ONLY_DATABASE_STATS options
> (name bikeshedding welcome) and teaches vacuumdb to use them.
I was surprised to hear that this added *two* options.
I assumed it would look like:
VACUUM (UPDATE_DATABASE_STATS {yes,no,only})
--
Justin
Re: Avoiding unnecessary clog lookups while freezing
On 2022-12-28 16:37:27 -0800, Peter Geoghegan wrote: > On Wed, Dec 28, 2022 at 4:20 PM Andres Freund wrote: > > > Theoretically this is an old issue that dates back to commit > > > 699bf7d05c, as opposed to an issue in the page-level freezing patch. > > > But that's not really true in a real practical sense. In practice the > > > calls to TransactionIdDidCommit() will happen far more frequently > > > following today's commit 1de58df4fe (since we're using OldestXmin as > > > the cutoff that gates performing freeze_xmin/freeze_xmax processing -- > > > not FreezeLimit). > > > > Hm. But we still only do the check when we actually freeze, rather than just > > during the pre-check in heap_tuple_should_freeze(). So we'll only incur the > > increased overhead when we also do more WAL logging etc. Correct? > > Yes, that's how it worked up until today's commit 1de58df4fe. > > I don't have strong feelings about back patching a fix, but this does > seem like something that I should fix now, on HEAD. > > > Hm. I dimply recall that we had repeated cases where the hint bits were set > > wrongly due to some of the multixact related bugs. I think I was trying to > > be > > paranoid about not freezing stuff in those situations, since it can lead to > > reviving dead tuples, which obviously is bad. > > I think that it's a reasonable check, and I'm totally in favor of > keeping it (or something very close, at least). I don't quite follow - one paragraph up you say we should fix something, and then here you seem to say we should continue not to rely on the hint bits?
Re: Avoiding unnecessary clog lookups while freezing
On Wed, Dec 28, 2022 at 4:20 PM Andres Freund wrote: > > Theoretically this is an old issue that dates back to commit > > 699bf7d05c, as opposed to an issue in the page-level freezing patch. > > But that's not really true in a real practical sense. In practice the > > calls to TransactionIdDidCommit() will happen far more frequently > > following today's commit 1de58df4fe (since we're using OldestXmin as > > the cutoff that gates performing freeze_xmin/freeze_xmax processing -- > > not FreezeLimit). > > Hm. But we still only do the check when we actually freeze, rather than just > during the pre-check in heap_tuple_should_freeze(). So we'll only incur the > increased overhead when we also do more WAL logging etc. Correct? Yes, that's how it worked up until today's commit 1de58df4fe. I don't have strong feelings about back patching a fix, but this does seem like something that I should fix now, on HEAD. > Hm. I dimply recall that we had repeated cases where the hint bits were set > wrongly due to some of the multixact related bugs. I think I was trying to be > paranoid about not freezing stuff in those situations, since it can lead to > reviving dead tuples, which obviously is bad. I think that it's a reasonable check, and I'm totally in favor of keeping it (or something very close, at least). > There's practically no tuple-level concurrent activity in a normal regression > test. So I'm a bit doubtful that is an interesting metric. It'd be a tad more > interesting to run tests, including isolationtester and pgbench, against a > running cluster. Even on HEAD, with page-level freezing in place, we're only going to test XIDs that are < OldestXmin, that appear on pages tha VACUUM actually scans in the first place. Just checking tuple-level hint bits should be effective. But even if it isn't (for whatever reason) then it's similar to cases where our second heap pass has to do clog lookups in heap_page_is_all_visible() just because hint bits couldn't be set earlier on, back when lazy_scan_prune() processed the same page during VACUUM's initial heap pass. -- Peter Geoghegan
windows/meson cfbot warnings
Since a few days ago, the windows/meson task has been spewing messages for each
tap
test:
| Unknown TAP version. The first line MUST be `TAP version `. Assuming
version 12.
I guess because the image is updated to use meson v1.0.0.
https://github.com/mesonbuild/meson/commit/b7a5c384a1f1ba80c09904e7ef4f5160bdae3345
mesonbuild/mtest.py-if version is None:
mesonbuild/mtest.py:self.warnings.append('Unknown TAP version. The
first line MUST be `TAP version `. Assuming version 12.')
mesonbuild/mtest.py-version = 12
--
Justin
Re: Avoiding unnecessary clog lookups while freezing
Hi, On 2022-12-28 15:24:28 -0800, Peter Geoghegan wrote: > I took another look at the code coverage situation around freezing > following pushing the page-level freezing patch earlier today. I > spotted an issue that I'd missed up until now: certain sanity checks > in heap_prepare_freeze_tuple() call TransactionIdDidCommit() more > often than really seems necessary. > > Theoretically this is an old issue that dates back to commit > 699bf7d05c, as opposed to an issue in the page-level freezing patch. > But that's not really true in a real practical sense. In practice the > calls to TransactionIdDidCommit() will happen far more frequently > following today's commit 1de58df4fe (since we're using OldestXmin as > the cutoff that gates performing freeze_xmin/freeze_xmax processing -- > not FreezeLimit). Hm. But we still only do the check when we actually freeze, rather than just during the pre-check in heap_tuple_should_freeze(). So we'll only incur the increased overhead when we also do more WAL logging etc. Correct? > I took another look at the code coverage situation around freezing > I see no reason why we can't just condition the XID sanity check calls > to TransactionIdDidCommit() (for both the freeze_xmin and the > freeze_xmax callsites) on a cheap tuple hint bit precheck not being > enough. We only need an expensive call to TransactionIdDidCommit() > when the precheck doesn't immediately indicate that the tuple xmin > looks committed when that's what the sanity check expects to see (or > that the tuple's xmax looks aborted, in the case of the callsite where > that's what we expect to see). Hm. I dimply recall that we had repeated cases where the hint bits were set wrongly due to some of the multixact related bugs. I think I was trying to be paranoid about not freezing stuff in those situations, since it can lead to reviving dead tuples, which obviously is bad. > Attached patch shows what I mean. A quick run of the standard > regression tests (with custom instrumentation) shows that this patch > eliminates 100% of all relevant calls to TransactionIdDidCommit(), for > both the freeze_xmin and the freeze_xmax callsites. There's practically no tuple-level concurrent activity in a normal regression test. So I'm a bit doubtful that is an interesting metric. It'd be a tad more interesting to run tests, including isolationtester and pgbench, against a running cluster. Greetings, Andres Freund
Re: perl 5.36, C99, -Wdeclaration-after-statement -Wshadow=compatible-local
Andres Freund writes: > On 2022-12-28 13:43:27 -0500, Tom Lane wrote: >> Hmm ... I guess the buildfarm would tell us whether that detection works >> correctly on platforms where it matters. Let's keep it simple if we >> can. > Quick clarification question: Are you suggesting to use #ifdef __GNUC__, or > that it suffices to use -Werror=unknown-pragmas without a separate configure > check? I'd try -Werror=unknown-pragmas, and then go to the #ifdef if that doesn't seem to work well. regards, tom lane
Re: perl 5.36, C99, -Wdeclaration-after-statement -Wshadow=compatible-local
On 2022-12-28 13:43:27 -0500, Tom Lane wrote: > > In the compiler test, I chose to not check whether -Werror=unknown-pragmas > > is > > supported - it appears to be an old gcc flag, and the worst outcome is that > > HAVE_PRAGMA_SYSTEM_HEADER isn't defined. > > We could alternatively define HAVE_PRAGMA_SYSTEM_HEADER or such based on > > __GNUC__ being defined. > > Hmm ... I guess the buildfarm would tell us whether that detection works > correctly on platforms where it matters. Let's keep it simple if we > can. Quick clarification question: Are you suggesting to use #ifdef __GNUC__, or that it suffices to use -Werror=unknown-pragmas without a separate configure check?
Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)
Hi, On 2022-10-06 13:42:09 -0400, Melanie Plageman wrote: > > Additionally, some minor notes: > > > > - Since the stats are counting blocks, it would make sense to prefix the > > view columns with "blks_", and word them in the past tense (to match > > current style), i.e. "blks_written", "blks_read", "blks_extended", > > "blks_fsynced" (realistically one would combine this new view with other > > data e.g. from pg_stat_database or pg_stat_statements, which all use the > > "blks_" prefix, and stop using pg_stat_bgwriter for this which does not use > > such a prefix) > > I have changed the column names to be in the past tense. For a while I was convinced by the consistency argument (after Melanie pointing it out to me). But the more I look, the less convinced I am. The existing IO related stats in pg_stat_database, pg_stat_bgwriter aren't past tense, just the ones in pg_stat_statements. pg_stat_database uses past tense for tup_*, but not xact_*, deadlocks, checksum_failures etc. And even pg_stat_statements isn't consistent about it - otherwise it'd be 'planned' instead of 'plans', 'called' instead of 'calls' etc. I started to look at the naming "tense" issue again, after I got "confused" about "extended", because that somehow makes me think about more detailed stats or such, rather than files getting extended. ISTM that 'evictions', 'extends', 'fsyncs', 'reads', 'reuses', 'writes' are clearer than the past tense versions, and about as consistent with existing columns. FWIW, I've been hacking on this code a bunch, mostly around renaming things and changing the 'stacking' of the patches. My current state is at https://github.com/anarazel/postgres/tree/pg_stat_io A bit more to do before posting the edited version... Greetings, Andres Freund
Avoiding unnecessary clog lookups while freezing
I took another look at the code coverage situation around freezing following pushing the page-level freezing patch earlier today. I spotted an issue that I'd missed up until now: certain sanity checks in heap_prepare_freeze_tuple() call TransactionIdDidCommit() more often than really seems necessary. Theoretically this is an old issue that dates back to commit 699bf7d05c, as opposed to an issue in the page-level freezing patch. But that's not really true in a real practical sense. In practice the calls to TransactionIdDidCommit() will happen far more frequently following today's commit 1de58df4fe (since we're using OldestXmin as the cutoff that gates performing freeze_xmin/freeze_xmax processing -- not FreezeLimit). No regressions related to clog lookups by VACUUM were apparent from my performance validation of the page-level freezing work, but I suspect that the increase in TransactionIdDidCommit() calls will cause noticeable regressions with the right/wrong workload and/or configuration. The page-level freezing work is expected to reduce clog lookups in VACUUM in general, which is bound to have been a confounding factor. I see no reason why we can't just condition the XID sanity check calls to TransactionIdDidCommit() (for both the freeze_xmin and the freeze_xmax callsites) on a cheap tuple hint bit precheck not being enough. We only need an expensive call to TransactionIdDidCommit() when the precheck doesn't immediately indicate that the tuple xmin looks committed when that's what the sanity check expects to see (or that the tuple's xmax looks aborted, in the case of the callsite where that's what we expect to see). Attached patch shows what I mean. A quick run of the standard regression tests (with custom instrumentation) shows that this patch eliminates 100% of all relevant calls to TransactionIdDidCommit(), for both the freeze_xmin and the freeze_xmax callsites. -- Peter Geoghegan v1-0001-Avoid-unnecessary-clog-lookups-when-freezing.patch Description: Binary data
Re: Support logical replication of DDLs
On Wed, Dec 28, 2022 at 5:42 PM Zheng Li wrote: > > >- CreatePublication has a long list of command tags; is that good? > >Maybe it'd be better to annotate the list in cmdtaglist.h somehow. > > I've addressed this comment by introducing a new flag ddlreplok in the > PG_CMDTAG macro and modified CreatePublication accordingly. I notice the support for the following commands are missing while moving the command tags, will look into it: CMDTAG_CREATE_AGGREGATE CMDTAG_ALTER_AGGREGATE CMDTAG_DROP_AGGREGATE CMDTAG_DROP_TRIGGER CMDTAG_DROP_USER_MAPPING Zane
Re: BUG #17717: Regression in vacuumdb (15 is slower than 10/11 and possible memory issue)
Nathan Bossart writes: > On Wed, Dec 28, 2022 at 03:13:23PM -0500, Tom Lane wrote: >> +executeCommand(conn, "VACUUM (ONLY_DATABASE_STATS);", echo); > When I looked at this, I thought it would be better to send the command > through the parallel slot machinery so that failures would use the same > code path as the rest of the VACUUM commands. However, you also need to > adjust ParallelSlotsWaitCompletion() to mark the slots as idle so that the > slot array can be reused after it is called. Hm. I was just copying the way commands are issued further up in the same function. But I think you're right: once we've done ParallelSlotsAdoptConn(sa, conn); it's probably not entirely kosher to use the conn directly. regards, tom lane
Re: add \dpS to psql
On Wed, Dec 28, 2022 at 02:46:23PM +0300, Maxim Orlov wrote: > The patch applies with no problem, implements what it declared, CF bot is > happy. > Without patch \dpS shows 0 rows, after applying system objects are shown. > Consider this patch useful, hope it will be committed soon. Thanks for reviewing. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: BUG #17717: Regression in vacuumdb (15 is slower than 10/11 and possible memory issue)
On Wed, Dec 28, 2022 at 04:20:19PM -0500, Tom Lane wrote: > Nathan Bossart writes: >> I think the main difference between your patch and mine is that I've >> exposed vac_update_datfrozenxid() via a function instead of a VACUUM >> option. IMHO that feels a little more natural, but I can't say I feel too >> strongly about it. > > I thought about that but it seems fairly unsafe, because that means > that vac_update_datfrozenxid is executing inside a user-controlled > transaction. I don't think it will hurt us if the user does a > ROLLBACK afterward --- but if he sits on the open transaction, > that would be bad, if only because we're still holding the > LockDatabaseFrozenIds lock which will block other VACUUMs. > There might be more hazards besides that; certainly no one has ever > tried to run vac_update_datfrozenxid that way before. That's a good point. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: BUG #17717: Regression in vacuumdb (15 is slower than 10/11 and possible memory issue)
On Wed, Dec 28, 2022 at 03:13:23PM -0500, Tom Lane wrote:
> + /* If we used SKIP_DATABASE_STATS, mop up with ONLY_DATABASE_STATS */
> + if (vacopts->skip_database_stats && stage == ANALYZE_NO_STAGE &&
> !failed)
> + {
> + executeCommand(conn, "VACUUM (ONLY_DATABASE_STATS);", echo);
> + }
When I looked at this, I thought it would be better to send the command
through the parallel slot machinery so that failures would use the same
code path as the rest of the VACUUM commands. However, you also need to
adjust ParallelSlotsWaitCompletion() to mark the slots as idle so that the
slot array can be reused after it is called.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Re: BUG #17717: Regression in vacuumdb (15 is slower than 10/11 and possible memory issue)
Nathan Bossart writes: > I think the main difference between your patch and mine is that I've > exposed vac_update_datfrozenxid() via a function instead of a VACUUM > option. IMHO that feels a little more natural, but I can't say I feel too > strongly about it. I thought about that but it seems fairly unsafe, because that means that vac_update_datfrozenxid is executing inside a user-controlled transaction. I don't think it will hurt us if the user does a ROLLBACK afterward --- but if he sits on the open transaction, that would be bad, if only because we're still holding the LockDatabaseFrozenIds lock which will block other VACUUMs. There might be more hazards besides that; certainly no one has ever tried to run vac_update_datfrozenxid that way before. regards, tom lane
Re: BUG #17717: Regression in vacuumdb (15 is slower than 10/11 and possible memory issue)
On Wed, Dec 28, 2022 at 03:13:23PM -0500, Tom Lane wrote:
> I'm forced to the conclusion that we have to expose some VACUUM
> options if we want this to work well. Attached is a draft patch
> that invents SKIP_DATABASE_STATS and ONLY_DATABASE_STATS options
> (name bikeshedding welcome) and teaches vacuumdb to use them.
This is the conclusion I arrived at, too. In fact, I was just about to
post a similar patch set. I'm attaching it here anyway, but I'm fine with
proceeding with your version.
I think the main difference between your patch and mine is that I've
exposed vac_update_datfrozenxid() via a function instead of a VACUUM
option. IMHO that feels a little more natural, but I can't say I feel too
strongly about it.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From a46934c1a6cec7a5efe8a25d49507a7a2f59c928 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Tue, 27 Dec 2022 15:21:26 -0800
Subject: [PATCH v1 1/3] add UPDATE_DATFROZENXID option to VACUUM
---
doc/src/sgml/ref/vacuum.sgml | 22 ++
src/backend/commands/vacuum.c| 9 ++---
src/backend/postmaster/autovacuum.c | 2 +-
src/include/commands/vacuum.h| 1 +
src/test/regress/expected/vacuum.out | 2 ++
src/test/regress/sql/vacuum.sql | 3 +++
6 files changed, 35 insertions(+), 4 deletions(-)
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index e14ead8826..1219614507 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -36,6 +36,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ boolean ]
TRUNCATE [ boolean ]
PARALLEL integer
+UPDATE_DATFROZENXID [ boolean ]
and table_and_columns is:
@@ -295,6 +296,27 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ boolean
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index ba965b8c7b..51537aa5ba 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -114,6 +114,7 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
bool full = false;
bool disable_page_skipping = false;
bool process_toast = true;
+ bool update_datfrozenxid = vacstmt->is_vacuumcmd;
ListCell *lc;
/* index_cleanup and truncate values unspecified for now */
@@ -200,6 +201,8 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
params.nworkers = nworkers;
}
}
+ else if (strcmp(opt->defname, "update_datfrozenxid") == 0)
+ update_datfrozenxid = defGetBoolean(opt);
else
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -216,7 +219,8 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
(freeze ? VACOPT_FREEZE : 0) |
(full ? VACOPT_FULL : 0) |
(disable_page_skipping ? VACOPT_DISABLE_PAGE_SKIPPING : 0) |
- (process_toast ? VACOPT_PROCESS_TOAST : 0);
+ (process_toast ? VACOPT_PROCESS_TOAST : 0) |
+ (update_datfrozenxid ? VACOPT_UPDATE_DATFROZENXID : 0);
/* sanity checks on options */
Assert(params.options & (VACOPT_VACUUM | VACOPT_ANALYZE));
@@ -528,11 +532,10 @@ vacuum(List *relations, VacuumParams *params,
StartTransactionCommand();
}
- if ((params->options & VACOPT_VACUUM) && !IsAutoVacuumWorkerProcess())
+ if (params->options & VACOPT_UPDATE_DATFROZENXID)
{
/*
* Update pg_database.datfrozenxid, and truncate pg_xact if possible.
- * (autovacuum.c does this for itself.)
*/
vac_update_datfrozenxid();
}
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 0746d80224..edc219c8f5 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2854,7 +2854,7 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
tab->at_relid = relid;
tab->at_sharedrel = classForm->relisshared;
- /* Note that this skips toast relations */
+ /* Note that this skips toast relations and updating datfrozenxid */
tab->at_params.options = (dovacuum ? VACOPT_VACUUM : 0) |
(doanalyze ? VACOPT_ANALYZE : 0) |
(!wraparound ? VACOPT_SKIP_LOCKED : 0);
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 2f274f2bec..700489040e 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -188,6 +188,7 @@ typedef struct VacAttrStats
#define VACOPT_SKIP_LOCKED 0x20 /* skip if cannot get lock */
#define VACOPT_PROCESS_TOAST 0x40 /* process the TOAST table, if any */
#define VACOPT_DISABLE_PAGE_SKIPPING 0x80 /* don't skip any pages */
+#define VACOPT_UPDATE_DATFROZENXID 0x100 /* update datfrozenxid afterwards */
/*
* Values used by index_cleanup and truncate params.
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index 0035d158b7..19c405417d 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -282,6 +282,8 @@ ALTER TABLE vactst ALTER COLUMN t SET STORAGE EXTERNAL;
VACUUM (PROCESS_TOAST FALSE
Re: BUG #17717: Regression in vacuumdb (15 is slower than 10/11 and possible memory issue)
[ redirecting to -hackers because patch attached ]
Peter Geoghegan writes:
> On Fri, Dec 16, 2022 at 6:49 AM Tom Lane wrote:
>> That is a really good point. How about teaching VACUUM to track
>> the oldest original relfrozenxid and relminmxid among the table(s)
>> it processed, and skip vac_update_datfrozenxid unless at least one
>> of those matches the database's values? For extra credit, also
>> skip if we didn't successfully advance the source rel's value.
> Hmm. I think that that would probably work.
I poked into that idea some more and concluded that getting VACUUM to
manage it behind the user's back is not going to work very reliably.
The key problem is explained by this existing comment in autovacuum.c:
* Even if we didn't vacuum anything, it may still be important to do
* this, because one indirect effect of vac_update_datfrozenxid() is to
* update ShmemVariableCache->xidVacLimit. That might need to be done
* even if we haven't vacuumed anything, because relations with older
* relfrozenxid values or other databases with older datfrozenxid values
* might have been dropped, allowing xidVacLimit to advance.
That is, if the table that's holding back datfrozenxid gets dropped
between VACUUM runs, VACUUM would never think that it might have
advanced the global minimum.
I'm forced to the conclusion that we have to expose some VACUUM
options if we want this to work well. Attached is a draft patch
that invents SKIP_DATABASE_STATS and ONLY_DATABASE_STATS options
(name bikeshedding welcome) and teaches vacuumdb to use them.
Light testing says that this is a win: even on the regression
database, which isn't all that big, I see a drop in vacuumdb's
runtime from ~260 ms to ~175 ms. Of course this is a case where
VACUUM doesn't really have anything to do, so it's a best-case
scenario ... but still, I was expecting the effect to be barely
above noise with this many tables, yet it's a good bit more.
regards, tom lane
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index e14ead8826..443928f286 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -36,6 +36,8 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ boolean ]
TRUNCATE [ boolean ]
PARALLEL integer
+SKIP_DATABASE_STATS [ boolean ]
+ONLY_DATABASE_STATS [ boolean ]
and table_and_columns is:
@@ -295,6 +297,40 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ table_and_columns
+ list must be empty, and no other option may be enabled.
+
+
+
+
boolean
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index ba965b8c7b..055c1a146b 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -114,6 +114,8 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
bool full = false;
bool disable_page_skipping = false;
bool process_toast = true;
+ bool skip_database_stats = false;
+ bool only_database_stats = false;
ListCell *lc;
/* index_cleanup and truncate values unspecified for now */
@@ -200,6 +202,10 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
params.nworkers = nworkers;
}
}
+ else if (strcmp(opt->defname, "skip_database_stats") == 0)
+ skip_database_stats = defGetBoolean(opt);
+ else if (strcmp(opt->defname, "only_database_stats") == 0)
+ only_database_stats = defGetBoolean(opt);
else
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -216,7 +222,9 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
(freeze ? VACOPT_FREEZE : 0) |
(full ? VACOPT_FULL : 0) |
(disable_page_skipping ? VACOPT_DISABLE_PAGE_SKIPPING : 0) |
- (process_toast ? VACOPT_PROCESS_TOAST : 0);
+ (process_toast ? VACOPT_PROCESS_TOAST : 0) |
+ (skip_database_stats ? VACOPT_SKIP_DATABASE_STATS : 0) |
+ (only_database_stats ? VACOPT_ONLY_DATABASE_STATS : 0);
/* sanity checks on options */
Assert(params.options & (VACOPT_VACUUM | VACOPT_ANALYZE));
@@ -349,6 +357,23 @@ vacuum(List *relations, VacuumParams *params,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("PROCESS_TOAST required with VACUUM FULL")));
+ /* sanity check for ONLY_DATABASE_STATS */
+ if (params->options & VACOPT_ONLY_DATABASE_STATS)
+ {
+ Assert(params->options & VACOPT_VACUUM);
+ if (relations != NIL)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("ONLY_DATABASE_STATS cannot be specified with a list of tables")));
+ /* don't require people to turn off PROCESS_TOAST explicitly */
+ if (params->options & ~(VACOPT_VACUUM |
+VACOPT_PROCESS_TOAST |
+VACOPT_ONLY_DATABASE_STATS))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("ONLY_DATABASE_STATS cannot be specified with other VACUUM options")));
+ }
+
/*
* Create special memory context for cross-transaction storage.
*
@@ -376
Re: perl 5.36, C99, -Wdeclaration-after-statement -Wshadow=compatible-local
Andres Freund writes: > The attached *prototype* patch is a slightly different spin on the idea of > using -isystem: It adds a > #pragma GCC system_header > to plperl.h if supported by the compiler. That also avoids warnings from > within plperl and subsidiary headers. > I don't really have an opinion about whether using the pragma or -isystem is > preferrable. I chose the pragma because it makes it easier to grep for headers > where we chose to do this. This seems like a reasonable answer. It feels quite a bit less magic in the way that it suppresses warnings than -isystem, and also less likely to have unexpected side-effects (I have a nasty feeling that -isystem is more magic on macOS than elsewhere). So far it seems like only the Perl headers have much of an issue, though I can foresee Python coming along soon. > In the compiler test, I chose to not check whether -Werror=unknown-pragmas is > supported - it appears to be an old gcc flag, and the worst outcome is that > HAVE_PRAGMA_SYSTEM_HEADER isn't defined. > We could alternatively define HAVE_PRAGMA_SYSTEM_HEADER or such based on > __GNUC__ being defined. Hmm ... I guess the buildfarm would tell us whether that detection works correctly on platforms where it matters. Let's keep it simple if we can. regards, tom lane
Re: perl 5.36, C99, -Wdeclaration-after-statement -Wshadow=compatible-local
Hi,
On 2022-11-02 17:03:34 -0700, Andres Freund wrote:
> On 2022-11-02 19:57:45 -0400, Tom Lane wrote:
> > Andres Freund writes:
> > > On 2022-11-01 17:00:27 -0400, Peter Eisentraut wrote:
> > >> Python has the same issues. There are a few other Python-embedding
> > >> projects
> > >> that use -Wdeclaration-after-statement and complain if the Python headers
> > >> violate it. But it's getting tedious. -isystem would be a better
> > >> solution.
> >
> > > Which dependencies should we convert to -isystem?
> >
> > Color me confused about what's being discussed here. I see nothing
> > in the gcc manual suggesting that -isystem has any effect on warning
> > levels?
>
> It's only indirectly explained :(
>
>The -isystem and -idirafter options also mark the directory as a
> system directory, so that it gets the same special treatment that is applied
> to
>the standard system directories.
>
> and then https://gcc.gnu.org/onlinedocs/cpp/System-Headers.html
The attached *prototype* patch is a slightly different spin on the idea of
using -isystem: It adds a
#pragma GCC system_header
to plperl.h if supported by the compiler. That also avoids warnings from
within plperl and subsidiary headers.
I don't really have an opinion about whether using the pragma or -isystem is
preferrable. I chose the pragma because it makes it easier to grep for headers
where we chose to do this.
I added the pragma detection only to the meson build, but if others think this
is a good way to go, I'll do the necessary autoconf wrangling as well.
In the compiler test, I chose to not check whether -Werror=unknown-pragmas is
supported - it appears to be an old gcc flag, and the worst outcome is that
HAVE_PRAGMA_SYSTEM_HEADER isn't defined.
We could alternatively define HAVE_PRAGMA_SYSTEM_HEADER or such based on
__GNUC__ being defined.
Greetings,
Andres Freund
>From ee6d9760206b59baa8f2d9adb09cf730ca2d24a4 Mon Sep 17 00:00:00 2001
From: Andres Freund
Date: Wed, 28 Dec 2022 10:09:48 -0800
Subject: [PATCH] wip: perl: If supported, use gcc's system_header pragma to
hide warnings
Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
meson.build| 9 +
src/pl/plperl/plperl.h | 8
2 files changed, 17 insertions(+)
diff --git a/meson.build b/meson.build
index b872470cdfe..1532b04c62a 100644
--- a/meson.build
+++ b/meson.build
@@ -1408,6 +1408,15 @@ if not cc.compiles(c99_test, name: 'c99', args: test_c_args)
endif
endif
+# Does the compiler support #pragma GCC system_header? We optionally use it to
+# avoid warnings that we can't fix (e.g. in the perl headers).
+# See https://gcc.gnu.org/onlinedocs/cpp/System-Headers.html
+if cc.compiles('#pragma GCC system_header',
+name: 'pragma system header',
+args: test_c_args + ['-Werror=unknown-pragmas'])
+ cdata.set('HAVE_PRAGMA_SYSTEM_HEADER', 1)
+endif
+
sizeof_long = cc.sizeof('long', args: test_c_args)
cdata.set('SIZEOF_LONG', sizeof_long)
if sizeof_long == 8
diff --git a/src/pl/plperl/plperl.h b/src/pl/plperl/plperl.h
index 5243f308bd5..2656a0376ee 100644
--- a/src/pl/plperl/plperl.h
+++ b/src/pl/plperl/plperl.h
@@ -74,6 +74,14 @@
#define HAS_BOOL 1
#endif
+/*
+ * At least in newer versions the perl headers trigger a lot of warnings with
+ * our compiler flags. The system_header pragma hides warnings from within the
+ * rest of this file, if supported.
+ */
+#if HAVE_PRAGMA_SYSTEM_HEADER
+#pragma GCC system_header
+#endif
/*
* Get the basic Perl API. We use PERL_NO_GET_CONTEXT mode so that our code
--
2.38.0
Re: Allow placeholders in ALTER ROLE w/o superuser
On Tue, Dec 27, 2022 at 11:29:40PM -0500, Tom Lane wrote: > Justin Pryzby writes: > > This fails when run more than once: > > time meson test --setup running --print > > test_pg_db_role_setting-running/regress > > Ah. > > > It didn't fail for you because it says: > > ./src/test/modules/test_pg_db_role_setting/Makefile > > +# disable installcheck for now > > +NO_INSTALLCHECK = 1 > > So ... exactly why is the meson infrastructure failing to honor that? > This test looks sufficiently careless about its side-effects that > I completely agree with the decision to not run it in pre-existing > installations. Failing to drop a created superuser is just one of > its risk factors --- it also leaves around pg_db_role_setting entries. Meson doesn't try to parse the Makefiles (like the MSVC scripts) but (since 3f0e786ccb) has its own implementation, which involves setting 'runningcheck': false. 096dd80f3c seems to have copied the NO_INSTALLCHECK from oat's makefile, but didn't copy "runningcheck" from oat's meson.build (but did copy its regress_args). -- Justin
Re: Making Vars outer-join aware
Richard Guo writes:
> I think I see where the problem is. And I can see currently in
> get_eclass_for_sort_expr we always use the top JoinDomain. So although
> the equality clause 'a.x = b.y' belongs to JoinDomain {B}, we set up ECs
> for 'a.x' and 'b.y' that belong to the top JoinDomain {A, B, A/B}.
Yeah, that's a pretty squishy point, and likely wrong in detail.
If we want to associate an EC with the sort order of an index on
b.y, we almost certainly want that EC to belong to join domain {B}.
I had tried to do that in an earlier iteration of 0012, by dint of
adding a JoinDomain argument to get_eclass_for_sort_expr and then
having build_index_pathkeys specify the lowest join domain containing
the index's relation. It did not work very well: it couldn't generate
mergejoins on full-join clauses, IIRC.
Maybe some variant on that plan can be made to fly, but I'm not at
all clear on what needs to be adjusted. Anyway, that's part of why
I backed off on the notion of explicitly associating ECs with join
domains. As long as we only pay attention to the join domain in
connection with constants, get_eclass_for_sort_expr can get away with
just using the top join domain, because we'd never apply it to a
constant unless perhaps somebody manages to ORDER BY or GROUP BY a
constant, and in those cases the top domain really is the right one.
(It's syntactically difficult to write such a thing anyway, but not
impossible.)
I think this is sort of an intermediate state, and hopefully a
year from now we'll have a better idea of how to manage all that.
What I mainly settled for doing in 0012 was getting rid of the
"below outer join" concept for ECs, because having to identify
a value for that had been giving me fits in several previous
attempts at extending ECs to cover outer-join equalities.
I think that the JoinDomain concept will offer a better-defined
replacement.
regards, tom lane
Re: split TOAST support out of postgres.h
Peter Eisentraut writes: > ... Then we could either > 1) Include varatt.h in postgres.h, similar to elog.h and palloc.h. That > way we clean up the files a bit but don't change any external interfaces. > 2) Just let everyone who needs it include the new file. > 3) Compromise: You can avoid most "damage" by having fmgr.h include > varatt.h. That satisfies most data types and extension code. That way, > there are only a few places that need an explicit include of varatt.h. > I went with the last option in my patch. I dunno, #3 seems kind of unprincipled. Also, since fmgr.h is included so widely, I doubt it is buying very much in terms of reducing header footprint. How bad is it to do #2? regards, tom lane
Re: Error-safe user functions
On 2022-12-28 We 01:00, Amul Sul wrote: > On Tue, Dec 27, 2022 at 11:17 PM Tom Lane wrote: >> Andrew Dunstan writes: >>> Here's a patch that covers the ltree and intarray contrib modules. >> I would probably have done this a little differently --- I think >> the added "res" parameters aren't really necessary for most of >> these. But it's not worth arguing over. >> > Also, it would be good if we can pass "escontext" through the "state" > argument of makepool() like commit 78212f210114 done for makepol() of > tsquery.c. Attached patch is the updated version that does the same. > Thanks, I have done both of these things. Looks like we're now done with this task, thanks everybody. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Underscores in numeric literals
On 2022-12-27 Tu 09:55, Tom Lane wrote: > We already accept that numeric input is different from numeric > literals: you can't write Infinity or NaN in SQL without quotes. > So I don't see an argument that we have to allow this in numeric > input for consistency. > That's almost the same, but not quite, ISTM. Those are things you can't say without quotes, but here unless I'm mistaken you'd be disallowing this style if you use quotes. I get the difficulties with input functions, but it seems like we'll be building lots of grounds for confusion. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: split TOAST support out of postgres.h
On Wed, 28 Dec 2022 at 08:07, Peter Eisentraut < peter.eisentr...@enterprisedb.com> wrote: > Most backend code doesn't actually need the variable-length data types > support (TOAST support) in postgres.h. So I figured we could try to put > it into a separate header file. That makes postgres.h more manageable, > and it avoids including a bunch of complicated unused stuff everywhere. > I picked "varatt.h" as the name. Then we could either > […] > I went with the last option in my patch. > > Thoughts? This is a bit of a bikeshed suggestion, but I'm wondering if you considered calling it toast.h? Only because the word is so distinctive within Postgres; everybody knows exactly to what it refers. I definitely agree with the principle of organizing and splitting up the header files. Personally, I don't mind importing a bunch of headers if I'm using a bunch of subsystems so I would be OK with needing to import this new header if I need it.
split TOAST support out of postgres.h
Most backend code doesn't actually need the variable-length data types
support (TOAST support) in postgres.h. So I figured we could try to put
it into a separate header file. That makes postgres.h more manageable,
and it avoids including a bunch of complicated unused stuff everywhere.
I picked "varatt.h" as the name. Then we could either
1) Include varatt.h in postgres.h, similar to elog.h and palloc.h. That
way we clean up the files a bit but don't change any external interfaces.
2) Just let everyone who needs it include the new file.
3) Compromise: You can avoid most "damage" by having fmgr.h include
varatt.h. That satisfies most data types and extension code. That way,
there are only a few places that need an explicit include of varatt.h.
I went with the last option in my patch.
Thoughts?From fba83e468a8ea881f0bdd6cf49aef13b71f67273 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Wed, 28 Dec 2022 13:30:05 +0100
Subject: [PATCH] New header varatt.h split off from postgres.h
This new header contains all the variable-length data types support
(TOAST support) from postgres.h, which isn't needed by large parts of
the backend code.
This new header file is included by fmgr.h, so most data type and
extension code won't need any changes.
---
contrib/cube/cubedata.h | 2 +
contrib/pg_trgm/trgm_regexp.c | 1 +
src/backend/access/table/toast_helper.c | 1 +
src/backend/libpq/pqformat.c| 1 +
src/include/access/htup_details.h | 1 +
src/include/fmgr.h | 2 +
src/include/meson.build | 1 +
src/include/postgres.h | 358 +--
src/include/utils/expandeddatum.h | 2 +
src/include/{postgres.h => varatt.h}| 579 +---
10 files changed, 21 insertions(+), 927 deletions(-)
copy src/include/{postgres.h => varatt.h} (53%)
diff --git a/contrib/cube/cubedata.h b/contrib/cube/cubedata.h
index 96fa41a04e..d8df478059 100644
--- a/contrib/cube/cubedata.h
+++ b/contrib/cube/cubedata.h
@@ -1,5 +1,7 @@
/* contrib/cube/cubedata.h */
+#include "fmgr.h"
+
/*
* This limit is pretty arbitrary, but don't make it so large that you
* risk overflow in sizing calculations.
diff --git a/contrib/pg_trgm/trgm_regexp.c b/contrib/pg_trgm/trgm_regexp.c
index 58d32ba946..08eecd269b 100644
--- a/contrib/pg_trgm/trgm_regexp.c
+++ b/contrib/pg_trgm/trgm_regexp.c
@@ -196,6 +196,7 @@
#include "tsearch/ts_locale.h"
#include "utils/hsearch.h"
#include "utils/memutils.h"
+#include "varatt.h"
/*
* Uncomment (or use -DTRGM_REGEXP_DEBUG) to print debug info,
diff --git a/src/backend/access/table/toast_helper.c
b/src/backend/access/table/toast_helper.c
index 74ba2189f0..a4889d7642 100644
--- a/src/backend/access/table/toast_helper.c
+++ b/src/backend/access/table/toast_helper.c
@@ -19,6 +19,7 @@
#include "access/toast_helper.h"
#include "access/toast_internals.h"
#include "catalog/pg_type_d.h"
+#include "varatt.h"
/*
diff --git a/src/backend/libpq/pqformat.c b/src/backend/libpq/pqformat.c
index 9c24df3360..65a913e893 100644
--- a/src/backend/libpq/pqformat.c
+++ b/src/backend/libpq/pqformat.c
@@ -77,6 +77,7 @@
#include "libpq/pqformat.h"
#include "mb/pg_wchar.h"
#include "port/pg_bswap.h"
+#include "varatt.h"
/*
diff --git a/src/include/access/htup_details.h
b/src/include/access/htup_details.h
index c1af814e8d..c7b505e33f 100644
--- a/src/include/access/htup_details.h
+++ b/src/include/access/htup_details.h
@@ -19,6 +19,7 @@
#include "access/tupdesc.h"
#include "access/tupmacs.h"
#include "storage/bufpage.h"
+#include "varatt.h"
/*
* MaxTupleAttributeNumber limits the number of (user) columns in a tuple.
diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index 972afe3aff..0ce0090360 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -18,6 +18,8 @@
#ifndef FMGR_H
#define FMGR_H
+#include "varatt.h"
+
/* We don't want to include primnodes.h here, so make some stub references */
typedef struct Node *fmNodePtr;
typedef struct Aggref *fmAggrefPtr;
diff --git a/src/include/meson.build b/src/include/meson.build
index b4820049c8..63b444bc99 100644
--- a/src/include/meson.build
+++ b/src/include/meson.build
@@ -113,6 +113,7 @@ install_headers(
'postgres.h',
'postgres_ext.h',
'postgres_fe.h',
+ 'varatt.h',
'windowapi.h',
pg_config_ext,
pg_config_os,
diff --git a/src/include/postgres.h b/src/include/postgres.h
index 54730dfb38..a915f2b97f 100644
--- a/src/include/postgres.h
+++ b/src/include/postgres.h
@@ -23,9 +23,8 @@
*
* section description
* ---
- * 1) variable-length datatypes (TOAST support)
- * 2) Datum type + support functions
- * 3) miscellaneous
+ * 1) Datum type + support functions
+ *
Re: add \dpS to psql
> Here it is: https://commitfest.postgresql.org/41/4043/ > Hi! The patch applies with no problem, implements what it declared, CF bot is happy. Without patch \dpS shows 0 rows, after applying system objects are shown. Consider this patch useful, hope it will be committed soon. -- Best regards, Maxim Orlov.
Re: Data loss on logical replication, 12.12 to 14.5, ALTER SUBSCRIPTION
Hello. > None of these entries are from the point mentioned by you [1] > yesterday where you didn't find the corresponding data in the > subscriber. How did you identify that the entries corresponding to > that timing were missing? Some of the before the interval, some after... But the source database was generating a lot of WAL during logical replication - some of these log entries from time AFTER completion of initial sync of B but (probably) BEFORE finishing B table catch up (entering streaming mode). Just to clarify, tables A, B and C are updated in the same transaction, something like: BEGIN; UPDATE A SET x = x +1 WHERE id = :id; INSERT INTO B(current_time, :id); INSERT INTO C(current_time, :id); COMMIT; Other (non-mentioned) tables also included into this transaction, but only B missed small amount of data. So, shortly the story looks like: * initial sync of A (and other tables) started and completed, they are in streaming mode * B and C initial sync started (by altering PUBLICATION and SUBSCRIPTION) * B sync completed, but new changes are still applying to the tables to catch up primary * logical replication apply worker is restarted because IO error on WAL receive * Postgres killed * Postgres restarted * C initial sync restarted * logical replication apply worker few times restarted because IO error on WAL receive * finally every table in streaming mode but with small gap in B Thanks, Michail.
Re: Add SHELL_EXIT_CODE to psql
Hi! The patch is implementing what is declared to do. Shell return code is now accessible is psql var. Overall code is in a good condition. Applies with no errors on master. Unfortunately, regression tests are failing on the macOS due to the different shell output. @@ -1308,13 +1308,13 @@ deallocate q; -- test SHELL_EXIT_CODE \! nosuchcommand -sh: line 1: nosuchcommand: command not found +sh: nosuchcommand: command not found \echo :SHELL_EXIT_CODE 127 \set nosuchvar `nosuchcommand` -sh: line 1: nosuchcommand: command not found +sh: nosuchcommand: command not found \! nosuchcommand -sh: line 1: nosuchcommand: command not found +sh: nosuchcommand: command not found \echo :SHELL_EXIT_CODE 127 Since we do not want to test shell output in these cases, but only return code, what about using this kind of commands? postgres=# \! true > /dev/null 2>&1 postgres=# \echo :SHELL_EXIT_CODE 0 postgres=# \! false > /dev/null 2>&1 postgres=# \echo :SHELL_EXIT_CODE 1 postgres=# \! nosuchcommand > /dev/null 2>&1 postgres=# \echo :SHELL_EXIT_CODE 127 It is better to use spaces around "=". + if (WIFEXITED(exit_code)) + exit_code=WEXITSTATUS(exit_code); + else if(WIFSIGNALED(exit_code)) + exit_code=WTERMSIG(exit_code); + else if(WIFSTOPPED(exit_code)) + exit_code=WSTOPSIG(exit_code); -- Best regards, Maxim Orlov.
Add BufFileRead variants with short read and EOF detection
Most callers of BufFileRead() want to check whether they read the full
specified length. Checking this at every call site is very tedious.
This patch provides additional variants BufFileReadExact() and
BufFileReadMaybeEOF() that include the length checks.
I considered changing BufFileRead() itself, but this function is also
used in extensions, and so changing the behavior like this would create
a lot of problems there. The new names are analogous to the existing
LogicalTapeReadExact().From c40ee920f931d42b69338c777639200bafbee805 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Wed, 28 Dec 2022 11:46:14 +0100
Subject: [PATCH] Add BufFileRead variants with short read and EOF detection
Most callers of BufFileRead() want to check whether they read the full
specified length. Checking this at every call site is very tedious.
This patch provides additional variants BufFileReadExact() and
BufFileReadMaybeEOF() that include the length checks.
I considered changing BufFileRead() itself, but this function is also
used in extensions, and so changing the behavior like this would
create a lot of problems there. The new names are analogous to the
existing LogicalTapeReadExact().
---
src/backend/access/gist/gistbuildbuffers.c | 7 +---
src/backend/backup/backup_manifest.c | 8 +---
src/backend/executor/nodeHashjoin.c| 18 ++--
src/backend/replication/logical/worker.c | 29 +++--
src/backend/storage/file/buffile.c | 47 +++--
src/backend/utils/sort/logtape.c | 9 +---
src/backend/utils/sort/sharedtuplestore.c | 49 +++---
src/backend/utils/sort/tuplestore.c| 29 +++--
src/include/storage/buffile.h | 4 +-
9 files changed, 71 insertions(+), 129 deletions(-)
diff --git a/src/backend/access/gist/gistbuildbuffers.c
b/src/backend/access/gist/gistbuildbuffers.c
index 538e3880c9..bef98b292d 100644
--- a/src/backend/access/gist/gistbuildbuffers.c
+++ b/src/backend/access/gist/gistbuildbuffers.c
@@ -753,14 +753,9 @@ gistRelocateBuildBuffersOnSplit(GISTBuildBuffers *gfbb,
GISTSTATE *giststate,
static void
ReadTempFileBlock(BufFile *file, long blknum, void *ptr)
{
- size_t nread;
-
if (BufFileSeekBlock(file, blknum) != 0)
elog(ERROR, "could not seek to block %ld in temporary file",
blknum);
- nread = BufFileRead(file, ptr, BLCKSZ);
- if (nread != BLCKSZ)
- elog(ERROR, "could not read temporary file: read only %zu of
%zu bytes",
-nread, (size_t) BLCKSZ);
+ BufFileReadExact(file, ptr, BLCKSZ);
}
static void
diff --git a/src/backend/backup/backup_manifest.c
b/src/backend/backup/backup_manifest.c
index a54185fdab..ae2077794f 100644
--- a/src/backend/backup/backup_manifest.c
+++ b/src/backend/backup/backup_manifest.c
@@ -362,16 +362,10 @@ SendBackupManifest(backup_manifest_info *manifest, bbsink
*sink)
while (manifest_bytes_done < manifest->manifest_size)
{
size_t bytes_to_read;
- size_t rc;
bytes_to_read = Min(sink->bbs_buffer_length,
manifest->manifest_size
- manifest_bytes_done);
- rc = BufFileRead(manifest->buffile, sink->bbs_buffer,
-bytes_to_read);
- if (rc != bytes_to_read)
- ereport(ERROR,
- (errcode_for_file_access(),
-errmsg("could not read from temporary
file: %m")));
+ BufFileReadExact(manifest->buffile, sink->bbs_buffer,
bytes_to_read);
bbsink_manifest_contents(sink, bytes_to_read);
manifest_bytes_done += bytes_to_read;
}
diff --git a/src/backend/executor/nodeHashjoin.c
b/src/backend/executor/nodeHashjoin.c
index 3e1a997f92..605e12fe8c 100644
--- a/src/backend/executor/nodeHashjoin.c
+++ b/src/backend/executor/nodeHashjoin.c
@@ -1260,28 +1260,18 @@ ExecHashJoinGetSavedTuple(HashJoinState *hjstate,
* we can read them both in one BufFileRead() call without any type
* cheating.
*/
- nread = BufFileRead(file, header, sizeof(header));
+ nread = BufFileReadMaybeEOF(file, header, sizeof(header), true);
if (nread == 0) /* end of file */
{
ExecClearTuple(tupleSlot);
return NULL;
}
- if (nread != sizeof(header))
- ereport(ERROR,
- (errcode_for_file_access(),
-errmsg("could not read from hash-join
temporary file: read only %zu of %zu bytes",
- nread, sizeof(header;
*hashvalue = header[0];
tuple = (MinimalTuple) palloc(header[1]);
tuple->t_len = header[1];
-
Re: Add 64-bit XIDs into PostgreSQL 15
Hi! I want to make a quick summary here. 1. An overall consensus has been reached: we shall focus on committing SLRU changes first. 2. I've created an appropriate patch set here [0]. 3. How [0] is waiting for a review. As always, all opinions will be welcome. 4. While discussing error/warning messages and some other stuff, this thread was marked as "Waiting on Author". 5. I do rebase this patch set once in a week, but do not post it here, since there is no need in it. See (1). 6. For now, I don't understand what changes I have to make here. So, does "Waiting on Author" is appropriate status here? Anyway. Let's discuss on-disk page format, shall we? AFAICS, we have a following options: 1. Making "true" 64–bit XIDs. I.e. making every tuple have 64–bit xmin and xmax fields. 2. Put special in every page where base for XIDs are stored. This is what we have done in the current patch set. 3. Put base for XIDs in a fork. 4. Make explicit 64–bit XIDs for concrete relations. I.e. CREATE TABLE foo WITH (xid8) of smth. There were opinions that the proposed solution (2) is not the optimal. It would be great to hear your concerns and thoughts. [0] https://www.postgresql.org/message-id/CACG%3Dezav34TL%2BfGXD5vJ48%3DQbQBL9BiwkOTWduu9yRqie-h%2BDg%40mail.gmail.com -- Best regards, Maxim Orlov.
RE: Exit walsender before confirming remote flush in logical replication
Dear Amit, > Thanks for the verification. BTW, do you think we should document this > either with time-delayed replication or otherwise unless this is > already documented? I think this should be documented at "Shutting Down the Server" section in runtime.sgml or logical-replicaiton.sgml, but I cannot find. It will be included in next version. > Another thing we can investigate here why do we need to ensure that > there is no pending send before shutdown. I have not done yet about it, will continue next year. It seems that walsenders have been sending all data before shutting down since ea5516, e0b581 and 754baa. There were many threads related with streaming replication, so I could not pin the specific message that written in the commit message of ea5516. I have also checked some wiki pages [1][2], but I could not find any design about it. [1]: https://wiki.postgresql.org/wiki/Streaming_Replication [2]: https://wiki.postgresql.org/wiki/Synchronous_Replication_9/2010_Proposal Best Regards, Hayato Kuroda FUJITSU LIMITED
Re: [PATCH] Support using "all" for the db user in pg_ident.conf
@Michael > Anyway, it is a bit confusing to see a patch touching parts of the > ident code related to the system-username while it claims to provide a > mean to shortcut a check on the database-username. That's totally fair, I attached a new iteration of this patchset where this refactoring and the new functionality are split up in two patches. > That seems pretty dangerous to me. For one, how does this work in > cases where we expect the ident entry to be case-sensitive, aka > authentication methods where check_ident_usermap() and check_usermap() > use case_insensitive = false? I'm not sure if I'm understanding your concern correctly, but the system username will still be compared case sensitively if requested. The only thing this changes is that: before comparing the pg_role column to the requested pg_role case sensitively, it now checks if the value of the pg_role column is lowercase "all". If that's the case, then the pg_role column is not compared to the requested pg|role anymore, and instead access is granted. @Isaac > is there a reason why pg_ident.conf can't/shouldn't be replaced by a system > table? I'm not sure of the exact reason, maybe someone else can clarify this. But even if it could be replaced by a system table, I think that should be a separate patch from this patch. v2-0001-Rename-token-to-systemuser-in-IdentLine-struct.patch Description: v2-0001-Rename-token-to-systemuser-in-IdentLine-struct.patch v2-0002-Support-using-all-for-the-db-user-in-pg_ident.con.patch Description: v2-0002-Support-using-all-for-the-db-user-in-pg_ident.con.patch
Re: Making Vars outer-join aware
On Tue, Dec 27, 2022 at 11:31 PM Tom Lane wrote:
> The thing that I couldn't get around before is that if you have,
> say, a mergejoinable equality clause in an outer join:
>
> select ... from a left join b on a.x = b.y;
>
> that equality clause can only be associated with the join domain
> for B, because it certainly can't be enforced against A. However,
> you'd still wish to be able to do a mergejoin using indexes on
> a.x and b.y, and this means that we have to understand the ordering
> induced by a PathKey based on this EC as applicable to A, even
> though that relation is not in the same join domain. So there are
> situations where sort orderings apply across domain boundaries even
> though equalities don't. We might have to split the notion of
> EquivalenceClass into two sorts of objects, and somewhere right
> about here is where I realized that this wasn't getting finished
> for v16 :-(.
I think I see where the problem is. And I can see currently in
get_eclass_for_sort_expr we always use the top JoinDomain. So although
the equality clause 'a.x = b.y' belongs to JoinDomain {B}, we set up ECs
for 'a.x' and 'b.y' that belong to the top JoinDomain {A, B, A/B}.
But doing so would lead to a situation where the "same" Vars from
different join domains might have the same varnullingrels and thus would
match by equal(). As an example, consider
select ... from a left join b on a.x = b.y where a.x = 1;
As said we would set up EC for 'b.y' as belonging to the top JoinDomain.
Then when reconsider_outer_join_clause generates the equality clause
'b.y = 1', we figure out that the new clause belongs to JoinDomain {B}.
Note that the two 'b.y' here belong to different join domains but they
have the same varnullingrels (empty varnullingrels actually). As a
result, the equality 'b.y = 1' would be merged into the existing EC for
'b.y', because the two 'b.y' matches by equal() and we do not check
JoinDomain for non-const EC members. So we would end up with an EC
containing EC members of different join domains.
And it seems this would make the following statement in README not hold
any more.
We don't have to worry about this for Vars (or expressions
containing Vars), because references to the "same" column from
different join domains will have different varnullingrels and thus
won't be equal() anyway.
Thanks
Richard
