Re: v12: ERROR: subplan "InitPlan 2 (returns $4)" was not initialized
On Wed, Apr 12, 2023 at 3:59 AM Tom Lane wrote: > The v1 patch attached is enough to fix the immediate issue, > but there's another thing not to like, which is that we're also > discarding the costs associated with the initplans. That's > strictly cosmetic given that all the planning decisions are > already made, but it still seems potentially annoying if you're > trying to understand EXPLAIN output. So I'm inclined to instead > do something like v2 attached, which deals with that as well. > (On the other hand, we aren't bothering to fix up costs when > we move initplans around in materialize_finished_plan or > standard_planner ... so maybe that should be left for a patch > that fixes those things too.) +1 to the v2 patch. * Should we likewise set the parallel_safe flag for topmost plan in SS_attach_initplans? * In standard_planner around line 443, we move any initPlans from top_plan to the new added Gather node. But since we know that the top_plan is parallel_safe here, shouldn't it have no initPlans? Thanks Richard
Re: Direct I/O
On Wed, Apr 12, 2023 at 3:04 PM Thomas Munro wrote:
> On Wed, Apr 12, 2023 at 2:56 PM Christoph Berg wrote:
> > I'm hitting a panic in t_004_io_direct. The build is running on
> > overlayfs on tmpfs/ext4 (upper/lower) which is probably a weird
> > combination but has worked well for building everything over the last
> > decade. On Debian unstable:
> >
> > PANIC: could not open file "pg_wal/00010001": Invalid
> > argument
> ... I have a new idea: perhaps it is possible to try
> to open a file with O_DIRECT from perl, and if it fails like that,
> skip the test. Looking into that now.
I think I have that working OK. Any Perl hackers want to comment on
my use of IO::File (copied from examples on the internet that showed
how to use O_DIRECT)? I am not much of a perl hacker but according to
my package manager, IO/File.pm came with perl itself. And the Fcntl
eval trick that I copied from File::stat, and the perl-critic
suppression that requires?
I tested this on OpenBSD, which has no O_DIRECT, so that tests the
first reason to skip.
Does it skip OK on your system, for the second reason? Should we be
more specific about the errno?
As far as I know, the only systems on the build farm that should be
affected by this change are the illumos boxen (they have O_DIRECT,
unlike Solaris, but perl's $^O couldn't tell the difference between
Solaris and illumos, so they didn't previously run this test).
One thing I resisted the urge to do is invent PG_TEST_SKIP, a sort of
anti-PG_TEST_EXTRA. I think I'd rather hear about it if there is a
system out there that passes the pre-flight check, but fails later on,
because we'd better investigate why. That's basically the point of
shipping this "developer only" feature long before serious use of it.
From 72e865835bcf1c9dce2090de0da66839908133c6 Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Wed, 12 Apr 2023 16:26:54 +1200
Subject: [PATCH] Skip the 004_io_direct.pl test if a pre-flight check fails.
Previously the test had a list of OSes that direct I/O was expected to
work on, with the idea that we could easily adjust that if problems
showed up on less common systems. That didn't take account of the
possibility of running the tests on an OS where it works on typical
filesystems (eg all the systems our build farm), but doesn't work for
unusual systems like overlayfs, as Christoph discovered.
The new approach is to try to create a file with O_DIRECT from perl. If
that fails, we'll log the errno and skip the whole test.
Reported-by: Christoph Berg
Discussion: https://postgr.es/m/ZDYd4A78cT2ULxZZ%40msg.df7cb.de
diff --git a/src/test/modules/test_misc/t/004_io_direct.pl b/src/test/modules/test_misc/t/004_io_direct.pl
index 5a2dd0d288..467b3bfc02 100644
--- a/src/test/modules/test_misc/t/004_io_direct.pl
+++ b/src/test/modules/test_misc/t/004_io_direct.pl
@@ -2,19 +2,44 @@
use strict;
use warnings;
+use Fcntl;
+use IO::File;
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;
-# Systems that we know to have direct I/O support, and whose typical local
-# filesystems support it or at least won't fail with an error. (illumos should
-# probably be in this list, but perl reports it as solaris. Solaris should not
-# be in the list because we don't support its way of turning on direct I/O, and
-# even if we did, its version of ZFS rejects it, and OpenBSD just doesn't have
-# it.)
-if (!grep { $^O eq $_ } qw(aix darwin dragonfly freebsd linux MSWin32 netbsd))
+# We know that macOS has F_NOCACHE, and we know that Windows has
+# FILE_FLAG_NO_BUFFERING, and we assume that their typical file systems will
+# accept those flags. For every other system, we'll probe for O_DIRECT
+# support.
+
+if ($^O ne 'darwin' && $^O ne 'MSWin32')
{
- plan skip_all => "no direct I/O support";
+ # Perl's Fcntl module knows if this system's has O_DIRECT but can
+ # only tell us by reporting an error, so we copy a trick from File/stat.pm
+ # and probe for the definition with eval.
+ no strict 'refs';## no critic (ProhibitNoStrict)
+ my $val = eval { &{'Fcntl::O_DIRECT'} };
+ if (defined $val)
+ {
+ use Fcntl qw(O_DIRECT);
+
+ # Next we want to find out if we can successfully open a file using
+ # that on the present filesystem.
+ my $f = IO::File->new(
+ "${PostgreSQL::Test::Utils::tmp_check}/test_o_direct_file",
+ O_RDWR | O_DIRECT | O_CREAT);
+ if (!$f)
+ {
+ plan skip_all =>
+ "pre-flight attempt to open file with O_DIRECT fails with errno $!";
+ }
+ $f->close;
+ }
+ else
+ {
+ plan skip_all => "no O_DIRECT";
+ }
}
my $node = PostgreSQL::Test::Cluster->new('main');
--
2.39.2
Re: [PoC] pg_upgrade: allow to upgrade publisher node
FYI, here are some minor review comments for v4-0001
==
src/bin/pg_dump/pg_backup.h
1.
+ int logical_slot_only;
The field should be plural - "logical_slots_only"
==
src/bin/pg_dump/pg_dump.c
2.
+ appendPQExpBufferStr(query,
+ "SELECT r.slot_name, r.plugin, r.two_phase "
+ "FROM pg_replication_slots r "
+ "WHERE r.database = current_database() AND temporary = false "
+ "AND wal_status IN ('reserved', 'extended');");
The alias 'r' may not be needed at all here, but since you already
have it IMO it looks a bit strange that you used it for only some of
the columns but not others.
~~~
3.
+
+ /* FIXME: force dumping */
+ slotinfo[i].dobj.dump = DUMP_COMPONENT_ALL;
Why the "FIXME" here? Are you intending to replace this code with
something else?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
Re: Minimal logical decoding on standbys
On Tue, Apr 11, 2023 at 01:10:57PM -0700, Andres Freund wrote: > On 2023-04-11 11:04:50 +0200, Drouvot, Bertrand wrote: > > On 4/11/23 10:55 AM, Drouvot, Bertrand wrote: > > > I think we might want to add: > > > > > > $node_primary->wait_for_replay_catchup($node_standby); > > > > > > before calling the slot creation. > Pushed. Seems like a clear race in the test, so I didn't think it was worth > waiting for testing it on hoverfly. We'll see what happens in the next run. > I think we should lower the log level, but perhaps wait for a few more cycles > in case there are random failures? Fine with me. > I wonder if we should make the connections in poll_query_until to reduce > verbosity - it's pretty annoying how much that can bloat the log. Perhaps also > introduce some backoff? It's really annoying to have to trawl through all > those logs when there's a problem. Agreed. My ranked wish list for poll_query_until is: 1. Exponential backoff 2. Closed-loop time control via Time::HiRes or similar, instead of assuming that ten loops complete in ~1s. I've seen the loop take 3x as long as the intended timeout. 3. Connect less often than today's once per probe
Re: refactoring relation extension and BufferAlloc(), faster COPY
12.04.2023 02:21, Andres Freund wrote: Hi, On 2023-04-11 22:00:00 +0300, Alexander Lakhin wrote: A few days later I've found a new defect introduced with 31966b151. That's the same issue that Tom also just reported, at https://postgr.es/m/392271.1681238924%40sss.pgh.pa.us Attached is my WIP fix, including a test. Thanks for the fix. I can confirm that the issue is gone. ReadBuffer_common() contains an Assert(), that is similar to the fixed one, but it looks unreachable for the WAL replay case after 26158b852. Best regards, Alexander
Re: Add index scan progress to pg_stat_progress_vacuum
On Mon, Apr 10, 2023 at 07:20:42PM +, Imseih (AWS), Sami wrote:
> See v28 addressing the comments.
This should be OK (also checked the code paths where the reports are
added). Note that the patch needed a few adjustments for its
indentation.
--
Michael
From 9267342ba2a1b8b120efaa98871b736a5bbde9a9 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Wed, 12 Apr 2023 13:45:59 +0900
Subject: [PATCH v29] Report index vacuum progress.
Add 2 new columns to pg_stat_progress_vacuum.
The columns are ndexes_total as the total indexes to be vacuumed or cleaned and
indexes_processed as the number of indexes vacuumed or cleaned up so far.
Authors: Sami Imseih
Reviewed by: Masahiko Sawada, Michael Paquier, Nathan Bossart, Andres Freund
Discussion: https://www.postgresql.org/message-id/flat/5478dfcd-2333-401a-b2f0-0d186ab09...@amazon.com
---
src/include/commands/progress.h | 2 +
src/include/utils/backend_progress.h | 1 +
src/backend/access/heap/vacuumlazy.c | 70 ---
src/backend/access/transam/parallel.c | 18 +
src/backend/catalog/system_views.sql | 3 +-
src/backend/commands/vacuumparallel.c | 9 ++-
src/backend/utils/activity/backend_progress.c | 32 +
src/test/regress/expected/rules.out | 4 +-
doc/src/sgml/monitoring.sgml | 23 ++
9 files changed, 150 insertions(+), 12 deletions(-)
diff --git a/src/include/commands/progress.h b/src/include/commands/progress.h
index e5add41352..2478e87425 100644
--- a/src/include/commands/progress.h
+++ b/src/include/commands/progress.h
@@ -25,6 +25,8 @@
#define PROGRESS_VACUUM_NUM_INDEX_VACUUMS 4
#define PROGRESS_VACUUM_MAX_DEAD_TUPLES 5
#define PROGRESS_VACUUM_NUM_DEAD_TUPLES 6
+#define PROGRESS_VACUUM_INDEXES_TOTAL 7
+#define PROGRESS_VACUUM_INDEXES_PROCESSED 8
/* Phases of vacuum (as advertised via PROGRESS_VACUUM_PHASE) */
#define PROGRESS_VACUUM_PHASE_SCAN_HEAP 1
diff --git a/src/include/utils/backend_progress.h b/src/include/utils/backend_progress.h
index a84752ade9..70dea55fc0 100644
--- a/src/include/utils/backend_progress.h
+++ b/src/include/utils/backend_progress.h
@@ -37,6 +37,7 @@ extern void pgstat_progress_start_command(ProgressCommandType cmdtype,
Oid relid);
extern void pgstat_progress_update_param(int index, int64 val);
extern void pgstat_progress_incr_param(int index, int64 incr);
+extern void pgstat_progress_parallel_incr_param(int index, int64 incr);
extern void pgstat_progress_update_multi_param(int nparam, const int *index,
const int64 *val);
extern void pgstat_progress_end_command(void);
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 0a9ebd22bd..e5707098bd 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -2314,6 +2314,17 @@ lazy_vacuum_all_indexes(LVRelState *vacrel)
{
bool allindexes = true;
double old_live_tuples = vacrel->rel->rd_rel->reltuples;
+ const int progress_start_index[] = {
+ PROGRESS_VACUUM_PHASE,
+ PROGRESS_VACUUM_INDEXES_TOTAL
+ };
+ const int progress_end_index[] = {
+ PROGRESS_VACUUM_INDEXES_TOTAL,
+ PROGRESS_VACUUM_INDEXES_PROCESSED,
+ PROGRESS_VACUUM_NUM_INDEX_VACUUMS
+ };
+ int64 progress_start_val[2];
+ int64 progress_end_val[3];
Assert(vacrel->nindexes > 0);
Assert(vacrel->do_index_vacuuming);
@@ -2326,9 +2337,13 @@ lazy_vacuum_all_indexes(LVRelState *vacrel)
return false;
}
- /* Report that we are now vacuuming indexes */
- pgstat_progress_update_param(PROGRESS_VACUUM_PHASE,
- PROGRESS_VACUUM_PHASE_VACUUM_INDEX);
+ /*
+ * Report that we are now vacuuming indexes and the number of indexes to
+ * vacuum.
+ */
+ progress_start_val[0] = PROGRESS_VACUUM_PHASE_VACUUM_INDEX;
+ progress_start_val[1] = vacrel->nindexes;
+ pgstat_progress_update_multi_param(2, progress_start_index, progress_start_val);
if (!ParallelVacuumIsActive(vacrel))
{
@@ -2341,6 +2356,10 @@ lazy_vacuum_all_indexes(LVRelState *vacrel)
old_live_tuples,
vacrel);
+ /* Report the number of indexes vacuumed */
+ pgstat_progress_update_param(PROGRESS_VACUUM_INDEXES_PROCESSED,
+ idx + 1);
+
if (lazy_check_wraparound_failsafe(vacrel))
{
/* Wraparound emergency -- end current index scan */
@@ -2375,14 +2394,17 @@ lazy_vacuum_all_indexes(LVRelState *vacrel)
Assert(allindexes || VacuumFailsafeActive);
/*
- * Increase and report the number of index scans.
+ * Increase and report the number of index scans. Also, we reset
+ * PROGRESS_VACUUM_INDEXES_TOTAL and PROGRESS_VACUUM_INDEXES_PROCESSED.
*
* We deliberately include the case where we started a round of bulk
* deletes that we weren't able to finish due to the failsafe triggering.
*/
vacrel->num_index_scans++;
- pgstat_progress_update_param(PROGRESS_VACUUM_NUM_INDEX_VACUUMS,
- vacrel->num_index_scans);
+ progress_end_val[0] =
Re: SQL/JSON revisited (documentation)
Hi,
IS JSON is documented as:
expression IS [ NOT ] JSON
[ { VALUE | SCALAR | ARRAY | OBJECT } ]
[ { WITH | WITHOUT } UNIQUE [ KEYS ] ]
which is fine but 'VALUE' is nowhere mentioned
(except in the commit-message as: IS JSON [VALUE] )
Unless I'm mistaken 'VALUE' does indeed not change an IS JSON statement,
so to document we could simply insert this line (as in the attached):
"The VALUE key word is optional noise."
Somewhere in its text in func.sgml, which is now:
"This predicate tests whether expression can be parsed as JSON, possibly
of a specified type. If SCALAR or ARRAY or OBJECT is specified, the
test is whether or not the JSON is of that particular type. If WITH
UNIQUE KEYS is specified, then any object in the expression is also
tested to see if it has duplicate keys."
Erik Rijkers
--- doc/src/sgml/func.sgml.orig 2023-04-12 06:16:40.517722315 +0200
+++ doc/src/sgml/func.sgml 2023-04-12 06:30:56.410837805 +0200
@@ -16037,6 +16037,7 @@
This predicate tests whether expression can
be
parsed as JSON, possibly of a specified type.
+The VALUE key word is optional noise.
If SCALAR or ARRAY or
OBJECT is specified, the
test is whether or not the JSON is of that particular type. If
Re: Various typo fixes
On Tue, Apr 11, 2023 at 05:15:29PM -0500, Justin Pryzby wrote: > The first attachment fixes for typos in user-facing docs new in v16, > combining Thom's changes with the ones that I'd found. If that's > confusing, I'll resend my patches separately. > > The other four numbered patches could use extra review. In v16-typos.diff.. -buffered, the decoding will stream or serialize +buffered, decoding will stream or serialize The could be referred as "the decoding context", as well? - not starting with a . and ending with - .conf will be included. Multiple files within an include + ending with .conf and not starting with a . + will be included. Multiple files within an include In 0001.. Not sure that this is an improvement, switching the starting and ending parts. - include records. These records only contain two fields: + include directives. Include directives only contain two fields: [...] - included. The file or directory can be a relative of absolute path, and can + included. The file or directory can be a relative or absolute path, and can Yep, indeed. We've discussed quite a lot about the current wording that 0004 aims to change, FWIW. I have applied a first batch of fixes that relate to the areas introduced by myself, plus a few extras. The changes for pg_log_standby_snapshot() mostly left out for now (except one simple change in logicaldecoding.sgml). Most of the changes in 0002 and 0003 seem rather OK at quick glance, but perhaps their respective authors would like to weigh in. -- Michael signature.asc Description: PGP signature
Re: longfin missing gssapi_ext.h
Stephen Frost writes: > Understood. Please find attached the updated patch with changes to the > commit message to indicate that we now require MIT Kerberos, an > additional explicit check for gssapi_ext.h in configure.ac/configure, > along with updated documentation explicitly saying we require MIT > Kerberos for GSSAPI support. Um ... could you package this as a straight un-revert of the previous commit, then a delta patch? Would be easier to review. regards, tom lane
Re: Various typo fixes
On Wed, Apr 12, 2023 at 12:28:25PM +0900, Michael Paquier wrote: > On Tue, Apr 11, 2023 at 11:12:58PM +0200, Daniel Gustafsson wrote: > > > On 11 Apr 2023, at 16:53, Justin Pryzby wrote: > > > > > I think "logical" should be a here. > > > > Agree, it should in order to be consistent. > > Indeed. > > + to the wal_level parameter change on the primary won't be decoded. > > This wal_level should also have a markup. > > Number of uses of logical slots in this database that have been > - canceled due to old snapshots or a too low linkend="guc-wal-level"/> > + canceled due to old snapshots or too low a linkend="guc-wal-level"/> > > This sounds a bit strange to me. A too low wal_level would be a cause > for a cancel, hence shouldn't this be "canceled due to old snapshots > or due to a too low guc-wal-level? That's the same as the original language which Thom and I are requesting to change, (but you added another "due to"). "a too low" is poor english. It's good enough for a code comment, but this is a user-facing doc. It could be "an inadequate wal-level" or "a prohibitively low wal-level", but Thom's language is better. "too low a wal-level" means the same thing as "too low of a wal-level" (which would also be fine). -- Justin
Re: Various typo fixes
On Tue, Apr 11, 2023 at 11:12:58PM +0200, Daniel Gustafsson wrote: > > On 11 Apr 2023, at 16:53, Justin Pryzby wrote: > > > I think "logical" should be a here. > > Agree, it should in order to be consistent. Indeed. + to the wal_level parameter change on the primary won't be decoded. This wal_level should also have a markup. Number of uses of logical slots in this database that have been - canceled due to old snapshots or a too low + canceled due to old snapshots or too low a This sounds a bit strange to me. A too low wal_level would be a cause for a cancel, hence shouldn't this be "canceled due to old snapshots or due to a too low guc-wal-level? -- Michael signature.asc Description: PGP signature
Re: Direct I/O
On Wed, Apr 12, 2023 at 2:56 PM Christoph Berg wrote: > I'm hitting a panic in t_004_io_direct. The build is running on > overlayfs on tmpfs/ext4 (upper/lower) which is probably a weird > combination but has worked well for building everything over the last > decade. On Debian unstable: > > PANIC: could not open file "pg_wal/00010001": Invalid > argument Hi Christoph, That's an interesting one. I was half expecting to see that on some unusual systems, which is why I made the test check which OS it is and exclude those that are known to fail with EINVAL or ENOTSUPP on their common/typical file systems. But if it's going to be Linux, that's not going to work. I have a new idea: perhaps it is possible to try to open a file with O_DIRECT from perl, and if it fails like that, skip the test. Looking into that now.
Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply
At Wed, 12 Apr 2023 03:36:01 +0900, Fujii Masao
wrote in
> Attached patch fixes this issue. It ensures that postgres_fdw only
> waits
> for a reply if a cancel request is actually issued. Additionally,
> it improves PQgetCancel() to set error messages in certain error
> cases,
> such as when out of memory occurs and malloc() fails. Moreover,
> it enhances postgres_fdw to report a warning message when
> PQgetCancel()
> returns NULL, explaining the reason for the NULL value.
>
> Thought?
I wondered why the connection didn't fail in the first place. After
digging into it, I found (or remembered) that local (or AF_UNIX)
connections ignore the timeout value at making a connection. I think
the real issue here is that PGgetCancel is unnecessarily checking its
value and failing as a result. Otherwise there would be no room for
failure in the call to PQgetCancel() at that point in the example
case.
PQconnectPoll should remove the ignored parameters at connection or
PQgetCancel should ingore the unreferenced (or unchecked)
parameters. For example, the below diff takes the latter way and seems
working (for at least AF_UNIX connections)
diff --git a/src/interfaces/libpq/fe-connect.c
b/src/interfaces/libpq/fe-connect.c
index 40fef0e2c8..30e2ab54ba 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -4718,6 +4718,10 @@ PQgetCancel(PGconn *conn)
cancel->keepalives_idle = -1;
cancel->keepalives_interval = -1;
cancel->keepalives_count = -1;
+
+ if (conn->connip == 0)
+ return cancel;
+
if (conn->pgtcp_user_timeout != NULL)
{
if (!parse_int_param(conn->pgtcp_user_timeout,
Of course, it's not great that pgfdw_cancel_query_begin() ignores the
result from PQgetCancel(), but I think we don't need another ereport.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: Direct I/O
Hi,
I'm hitting a panic in t_004_io_direct. The build is running on
overlayfs on tmpfs/ext4 (upper/lower) which is probably a weird
combination but has worked well for building everything over the last
decade. On Debian unstable:
PANIC: could not open file "pg_wal/00010001": Invalid argument
16:21:16 Bailout called. Further testing stopped: pg_ctl start failed
16:21:16 t/004_io_direct.pl ..
16:21:16 Dubious, test returned 255 (wstat 65280, 0xff00)
16:21:16 No subtests run
16:21:16
16:21:16 Test Summary Report
16:21:16 ---
16:21:16 t/004_io_direct.pl(Wstat: 65280 (exited 255) Tests: 0
Failed: 0)
16:21:16 Non-zero exit status: 255
16:21:16 Parse errors: No plan found in TAP output
16:21:16 Files=4, Tests=65, 9 wallclock secs ( 0.03 usr 0.02 sys + 3.78 cusr
1.48 csys = 5.31 CPU)
16:21:16 Result: FAIL
16:21:16
build/src/test/modules/test_misc/tmp_check/log/004_io_direct_main.log
16:21:16 2023-04-11 23:21:16.431 UTC [25991] LOG: starting PostgreSQL 16devel
(Debian 16~~devel-1.pgdg+~20230411.2256.gc03c2ea) on x86_64-pc-linux-gnu,
compiled by gcc (Debian 12.2.0-14) 12.2.0, 64-bit
16:21:16 2023-04-11 23:21:16.431 UTC [25991] LOG: listening on Unix socket
"/tmp/s0C4hWQq82/.s.PGSQL.54693"
16:21:16 2023-04-11 23:21:16.433 UTC [25994] LOG: database system was shut
down at 2023-04-11 23:21:16 UTC
16:21:16 2023-04-11 23:21:16.434 UTC [25994] PANIC: could not open file
"pg_wal/00010001": Invalid argument
16:21:16 2023-04-11 23:21:16.525 UTC [25991] LOG: startup process (PID 25994)
was terminated by signal 6: Aborted
16:21:16 2023-04-11 23:21:16.525 UTC [25991] LOG: aborting startup due to
startup process failure
16:21:16 2023-04-11 23:21:16.526 UTC [25991] LOG: database system is shut down
16:21:16
build/src/test/modules/test_misc/tmp_check/t_004_io_direct_main_data/pgdata/core
16:21:17
16:21:17 warning: Can't open file /dev/shm/PostgreSQL.3457641370 during
file-backed mapping note processing
16:21:17
16:21:17 warning: Can't open file /dev/shm/PostgreSQL.2391834648 during
file-backed mapping note processing
16:21:17
16:21:17 warning: Can't open file /dev/zero (deleted) during file-backed
mapping note processing
16:21:17
16:21:17 warning: Can't open file /SYSV0dea (deleted) during file-backed
mapping note processing
16:21:17 [New LWP 25994]
16:21:17 [Thread debugging using libthread_db enabled]
16:21:17 Using host libthread_db library
"/lib/x86_64-linux-gnu/libthread_db.so.1".
16:21:17 Core was generated by `postgres: main: startup
'.
16:21:17 Program terminated with signal SIGABRT, Aborted.
16:21:17 #0 0x7f176c591ccc in ?? () from /lib/x86_64-linux-gnu/libc.so.6
16:21:17 #0 0x7f176c591ccc in ?? () from /lib/x86_64-linux-gnu/libc.so.6
16:21:17 No symbol table info available.
16:21:17 #1 0x7f176c542ef2 in raise () from /lib/x86_64-linux-gnu/libc.so.6
16:21:17 No symbol table info available.
16:21:17 #2 0x7f176c52d472 in abort () from /lib/x86_64-linux-gnu/libc.so.6
16:21:17 No symbol table info available.
16:21:17 #3 0x55a7ba7978a1 in errfinish (filename=,
lineno=, funcname=0x55a7ba810560 <__func__.47>
"XLogFileInitInternal") at ./build/../src/backend/utils/error/elog.c:604
16:21:17 edata = 0x55a7baae3e20
16:21:17 elevel = 23
16:21:17 oldcontext = 0x55a7bb471590
16:21:17 econtext = 0x0
16:21:17 __func__ = "errfinish"
16:21:17 #4 0x55a7ba21759c in XLogFileInitInternal (logsegno=1,
logtli=logtli@entry=1, added=added@entry=0x7ffebc6c8a3f,
path=path@entry=0x7ffebc6c8a40 "pg_wal/0001", '0' , "1")
at ./build/../src/backend/access/transam/xlog.c:2944
16:21:17 __errno_location =
16:21:17 tmppath =
"0\214l\274\376\177\000\000\321\330~\272\247U\000\000\005Q\223\272\247U\000\000p\214l\274\376\177\000\000`\214l\274\376\177\000\000\212\335~\000\v",
'\000' ,
"\247U\000\000\000\000\000\000\000\177\000\000*O\202\272\247U\000\000\254\206l\274\376\177\000\000\000\000\000\000\v",
'\000' ,
"0\000\000\000\000\000\000\000\247U\000\000\000\000\000\000\000\000\000\000\001Q\223\272\247U\000\000\240\215l\274\376\177\000\000\376\377\377\377\000\000\000\\207l\274\376\177\000\000[\326~\272\247U\000\\207l\274\376\177\000\000"...
16:21:17 installed_segno = 0
16:21:17 max_segno =
16:21:17 fd =
16:21:17 save_errno =
16:21:17 open_flags = 194
16:21:17 __func__ = "XLogFileInitInternal"
16:21:17 #5 0x55a7ba35a1d5 in XLogFileInit (logsegno=,
logtli=logtli@entry=1) at ./build/../src/backend/access/transam/xlog.c:3099
16:21:17 ignore_added = false
16:21:17 path = "pg_wal/0001", '0' ,
"1\000\220\312P\273\247U\000\000/\375Yl\027\177\000\000\220\252P\273\247U\000\000\001",
'\000' ,
"\220\252P\273\247U\000\000\300\212l\274\376\177\000\000\002\261{\272\247U\000\000\220\252P\2
Re: Non-superuser subscription owners
On Tue, Apr 11, 2023 at 8:21 PM Robert Haas wrote: > > On Tue, Apr 11, 2023 at 5:53 AM Amit Kapila wrote: > > I think additionally, we should check that the new owner of the > > subscription is not a superuser, otherwise, anyway, this parameter is > > ignored. Please find the attached to add this check. > > I don't see why we should check that. It makes this different from all > the other cases and I don't see any benefit. > I thought it would be better if we don't restart the worker unless it is required. In case, the subscription's owner is a superuser, the 'password_required' is ignored, so why restart the apply worker when somebody changes it in such a case? I understand that there may not be a need to change the 'password_required' option when the subscription's owner is the superuser but one may first choose to change the password_required flag and then the owner of a subscription to a non-superuser. Anyway, I don't think as such there is any problem with restarting the worker even when the subscription owner is a superuser, so adjusted the check accordingly. -- With Regards, Amit Kapila. exit_on_sub_opt_change_2.patch Description: Binary data
Re: v15b1: FailedAssertion("segment_map->header->magic == (DSA_SEGMENT_HEADER_MAGIC ^ area->control->handle ^ index)", File: "dsa.c", ..)
On Wed, Apr 12, 2023 at 11:49:51AM +1200, Thomas Munro wrote:
> On Wed, Apr 12, 2023 at 11:37 AM Justin Pryzby wrote:
> > $ ls /dev/shm/ |grep 3696856876 || echo not found
> > not found
>
> Oh, of course it would have restarted after it crashed and unlinked
> that... So the remaining traces of that memory *might* be in the core
> file, depending (IIRC) on the core filter settings (you definitely get
> shared anonymous memory like our main shm region by default, but IIRC
> there's something extra needed if you want the shm_open'd DSM segments
> to be dumped too...)
I scrounged around and found:
/var/spool/abrt/ccpp-2023-03-11-13:07:02-24257/maps
Which has (including the size):
$ sudo cat /var/spool/abrt/ccpp-2023-03-11-13:07:02-24257/maps |awk
--non-decimal-data -F'[- ]' '/shm|zero|SYSV/{a="0x"$1; b="0x"$2; print
$0,0+b-a}'
7fd39c981000-7fd39ca81000 rw-s 00:0f 1698691690
/dev/shm/PostgreSQL.3696856876 1048576
7fd39ca81000-7fd39cc81000 rw-s 00:0f 1699005881
/dev/shm/PostgreSQL.433426374 2097152
7fd39cc81000-7fd39d081000 rw-s 00:0f 2443340900
/dev/shm/PostgreSQL.2754923922 4194304
7fd39d081000-7fd3b6a09000 rw-s 00:04 1698308066
/dev/zero (deleted) 429424640
7fd3bcf58000-7fd3bcf63000 rw-s 00:0f 1698308074
/dev/shm/PostgreSQL.2386569568 45056
7fd3bcf63000-7fd3bcf64000 rw-s 00:04 9732096
/SYSV0001581b (deleted) 4096
But except for the last one, none of these are available in the corefile.
--
Justin
Re: Fix fseek() detection of unseekable files on WIN32
On Tue, Apr 11, 2023 at 02:43:25PM +0900, Michael Paquier wrote: > After going through the installation of a Windows setup with meson and > ninja under VS, I have checked that this is working correctly by > myself, so I am going to apply that. One of the tests I have done > involved feeding a dump of the regression data through a pipe to > pg_restore, and the whole was able to work fine, while head broke when > using a pipe. Applied this one down to 14. The first responses from the buildfarm are good, I'll keep an eye on all that. -- Michael signature.asc Description: PGP signature
Re: [PATCH] Allow Postgres to pick an unused port to listen
Hi Robert, On Tue, Apr 11, 2023 at 12:54 AM Robert Haas wrote: > On Fri, Apr 7, 2023 at 5:34 PM Yurii Rashkovskii wrote: > > I'm trying to understand what's wrong with reading port from the pid > file (if Postgres writes information there, it's surely so that somebody > can read it, otherwise, why write it in the first placd)? The proposed > solution uses operating system's functionality to achieve collision-free > mechanics with none of the complexity introduced in the commit. > > I agree. We don't document the exact format of the postmaster.pid file > to my knowledge, but storage.sgml lists all the things it contains, > and runtime.sgml documents that the first line contains the postmaster > PID, so this is clearly not some totally opaque file that nobody > should ever touch. Consequently, I don't agree with Tom's statement > that this would be a "a horrid way to find out what was picked." There > is some question in my mind about whether this is a feature that we > want PostgreSQL to have, and if we do want it, there may be some room > for debate about how it's implemented, but I reject the idea that > extracting the port number from postmaster.pid is intrinsically a > terrible plan. It seems like a perfectly reasonable plan. > > I appreciate your support on the pid file concern. What questions do you have about this feature with regard to its desirability and/or implementation? I'd love to learn from your insight and address any of those if I can. -- Y.
Re: Protecting allocator headers with Valgrind
On Tue, Apr 11, 2023 at 9:28 PM David Rowley wrote: > Over on [1], Tom mentioned that we might want to rethink the decision > to not protect chunk headers with Valgrind. That thread fixed a bug > that was accessing array element -1, which effectively was reading the > MemoryChunk at the start of the allocated chunk as an array element. Seems the link to the original thread is not pasted. Here it is. [1] https://www.postgresql.org/message-id/1650235.1672694719%40sss.pgh.pa.us Thanks Richard
Re: pg_init_privs corruption.
On Fri, 17 Feb 2023 at 15:38, Tom Lane wrote: > > Hmm, so Stephen was opining that the extension's objects shouldn't > have gotten these privs attached in the first place. I'm not > quite convinced about that one way or the other, but if you buy it > then maybe this situation is unreachable once we fix that. Well pg_dump might still have to deal with it even if it's unreachable in new databases (or rather schemas with extensions newly added... it might be hard to explain what cases are affected). Alternately it'll be a note at the top of every point release pointing at a note explaining how to run a script to fix your schemas. -- greg
Re: longfin missing gssapi_ext.h
Greetings,
* Stephen Frost (sfr...@snowman.net) wrote:
> Greetings,
>
> * Justin Pryzby (pry...@telsasoft.com) wrote:
> > > configure | 27 ++
> > > configure.ac | 2 +
> >
> > Does meson.build need the corresponding change ?
>
> Ah, yes, presumably.
No, more like attached actually. Picks up on the dependency properly
with this when I ran meson/ninja, at least.
I'll include this then (along with any other suggestions, of course).
Thanks!
Stephen
diff --git a/meson.build b/meson.build
index b69aaddb1f..3405cc07ee 100644
--- a/meson.build
+++ b/meson.build
@@ -623,6 +623,16 @@ if not gssapiopt.disabled()
have_gssapi = false
endif
+ if not have_gssapi
+ elif cc.check_header('gssapi/gssapi_ext.h', dependencies: gssapi, required: false,
+ args: test_c_args, include_directories: postgres_inc)
+cdata.set('HAVE_GSSAPI_GSSAPI_EXT_H', 1)
+ elif cc.check_header('gssapi_ext.h', args: test_c_args, dependencies: gssapi, required: gssapiopt)
+cdata.set('HAVE_GSSAPI_EXT_H', 1)
+ else
+have_gssapi = false
+ endif
+
if not have_gssapi
elif cc.has_function('gss_init_sec_context', dependencies: gssapi,
args: test_c_args, include_directories: postgres_inc)
signature.asc
Description: PGP signature
Re: Regarding Plan tree output(Index/Bitmap Scan)
On Tue, Apr 11, 2023 at 06:09:41PM -0700, Ajay P S wrote: > I am trying to understand the Plan tree for select queries. Can you > please help me with the below queries? > > 1) Why is there a difference in plan tree for these two queries? User > table tidx1 has an index on column 'a' . Based on the query planner's cost estimate of the different scans. > 2) Why do we do Index scan and not Bitmap Index Scan for catalog tables? There's no reason why it can't happen in general. But you queried pg_class on a unique column, returning at most one row. A bitmap couldn't help by making the I/O more sequential. It can only add overhead. You can compare the costs of various plans by running EXPLAIN with various enable_* GUCs to off. BTW, your question should be directed to another list - this list is for bug reports and development. -- Justin
Re: longfin missing gssapi_ext.h
Greetings,
* Justin Pryzby (pry...@telsasoft.com) wrote:
> > configure | 27 ++
> > configure.ac | 2 +
>
> Does meson.build need the corresponding change ?
Ah, yes, presumably.
Something like the attached?
Thanks,
Stephen
diff --git a/meson.build b/meson.build
index b69aaddb1f..a171e6dc74 100644
--- a/meson.build
+++ b/meson.build
@@ -619,6 +619,11 @@ if not gssapiopt.disabled()
cdata.set('HAVE_GSSAPI_GSSAPI_H', 1)
elif cc.check_header('gssapi.h', args: test_c_args, dependencies: gssapi, required: gssapiopt)
cdata.set('HAVE_GSSAPI_H', 1)
+ elif cc.check_header('gssapi/gssapi_ext.h', dependencies: gssapi, required: false,
+ args: test_c_args, include_directories: postgres_inc)
+cdata.set('HAVE_GSSAPI_GSSAPI_EXT_H', 1)
+ elif cc.check_header('gssapi_ext.h', args: test_c_args, dependencies: gssapi, required: gssapiopt)
+cdata.set('HAVE_GSSAPI_EXT_H', 1)
else
have_gssapi = false
endif
signature.asc
Description: PGP signature
Re: longfin missing gssapi_ext.h
> configure | 27 ++ > configure.ac | 2 + Does meson.build need the corresponding change ?
Re: ERROR messages in VACUUM's PARALLEL option
At Tue, 11 Apr 2023 18:16:40 -0400, Tom Lane wrote in > David Rowley writes: > > Thanks for chipping in on this. Can you confirm if you think this > > should apply to VACUUM options? We're not talking GUCs here. > > My druthers would be to treat them similarly to GUCs. IMHO I like this direction. > I recognize that I might be in the minority, and that doing > so would entail touching a lot of existing messages in this > area. Nonetheless, I think this area is not being consistent > with our wider conventions, which is why for example defGetString > is out of step with these messages. On the other hand, the documentation write optinos in uppercase [1]. It is why I did not push hard for the normalization. [1] https://www.postgresql.org/docs/devel/sql-vacuum.html regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Add LZ4 compression in pg_dump
On Wed, Apr 12, 2023 at 10:07:08AM +0900, Michael Paquier wrote: > On Tue, Apr 11, 2023 at 07:41:11PM -0500, Justin Pryzby wrote: > > Maybe I would write it as: "if zlib is unavailable, default to no > > compression". But I think that's best done in the leading comment, and > > not inside an empty preprocessor #else. > > > > I was hoping Michael would comment on this. > > (Sorry for the late reply, somewhat missed that.) > > > The placement and phrasing of the comment makes no sense to me. > > Yes, this comment gives no value as it stands. I would be tempted to > follow the suggestion to group the whole code block in a single ifdef, > including the check, and remove this comment. Like the attached > perhaps? +1
Re: longfin missing gssapi_ext.h
Greetings, * Jonathan S. Katz (jk...@postgresql.org) wrote: > On 4/10/23 11:37 AM, Tom Lane wrote: > > Stephen Frost writes: > > > * Tom Lane (t...@sss.pgh.pa.us) wrote: > > > > IOW, maybe it'd be okay to de-revert 3d4fa227b and add documentation > > > > saying that --with-gssapi requires MIT Kerberos not Heimdal. > > > > > I'd be happy with that and can add the appropriate documentation noting > > > that we require MIT Kerberos. Presumably the appropriate step at this > > > point would be to check with the RMT? > > > > Yeah, RMT would have final say at this stage. > > > > If you pull the trigger, a note to buildfarm-members would be > > appropriate too, so people will know if they need to remove > > --with-gssapi from animal configurations. > > The RMT discussed this thread and agrees to a "de-revert" of the "Add > support for Kerberos delegation" patch, provided that: > > 1. The appropriate documentation is added AND > 2. The de-revert occurs no later than 2023-04-15 (upper bound 2023-04-16 > 0:00 AoE). > > There's an open item[1] for this task. Understood. Please find attached the updated patch with changes to the commit message to indicate that we now require MIT Kerberos, an additional explicit check for gssapi_ext.h in configure.ac/configure, along with updated documentation explicitly saying we require MIT Kerberos for GSSAPI support. I'll plan to push this tomorrow. Of course, suggestions/improvements on documentation or anything else always welcome. Thanks all! Stephen From c485794516e9aea2274d8c2c782945d4a10e33bb Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Sat, 8 Apr 2023 08:08:19 -0400 Subject: [PATCH] Add support for Kerberos credential delegation Support GSSAPI/Kerberos credentials being delegated to the server by a client. With this, a user authenticating to PostgreSQL using Kerberos (GSSAPI) credentials can choose to delegate their credentials to the PostgreSQL server (which can choose to accept them, or not), allowing the server to then use those delegated credentials to connect to another service, such as with postgres_fdw or dblink or theoretically any other service which is able to be authenticated using Kerberos. Both postgres_fdw and dblink are changed to allow non-superuser password-less connections but only when GSSAPI credentials have been delegated to the server by the client and GSSAPI is used to authenticate to the remote system. As part of this, we now require MIT Kerberos when building with support for GSSAPI. Authors: Stephen Frost, Peifeng Qiu Reviewed-By: David Christensen Discussion: https://postgr.es/m/co1pr05mb8023cc2cb575e0faad7df4f8a8...@co1pr05mb8023.namprd05.prod.outlook.com --- configure | 27 ++ configure.ac | 2 + contrib/dblink/dblink.c | 127 --- contrib/dblink/expected/dblink.out| 4 +- contrib/postgres_fdw/connection.c | 72 +++- .../postgres_fdw/expected/postgres_fdw.out| 19 +- contrib/postgres_fdw/option.c | 6 + contrib/postgres_fdw/sql/postgres_fdw.sql | 3 +- doc/src/sgml/config.sgml | 17 + doc/src/sgml/dblink.sgml | 5 +- doc/src/sgml/installation.sgml| 15 +- doc/src/sgml/libpq.sgml | 41 +++ doc/src/sgml/monitoring.sgml | 9 + doc/src/sgml/postgres-fdw.sgml| 7 +- src/backend/catalog/system_views.sql | 3 +- src/backend/foreign/foreign.c | 1 + src/backend/libpq/auth.c | 13 +- src/backend/libpq/be-gssapi-common.c | 53 +++ src/backend/libpq/be-secure-gssapi.c | 26 +- src/backend/utils/activity/backend_status.c | 1 + src/backend/utils/adt/pgstatfuncs.c | 21 +- src/backend/utils/init/postinit.c | 8 +- src/backend/utils/misc/guc_tables.c | 10 + src/backend/utils/misc/postgresql.conf.sample | 1 + src/include/catalog/pg_proc.dat | 6 +- src/include/libpq/auth.h | 1 + src/include/libpq/be-gssapi-common.h | 3 + src/include/libpq/libpq-be.h | 2 + src/include/utils/backend_status.h| 1 + src/interfaces/libpq/exports.txt | 1 + src/interfaces/libpq/fe-auth.c| 15 +- src/interfaces/libpq/fe-connect.c | 17 + src/interfaces/libpq/fe-secure-gssapi.c | 23 +- src/interfaces/libpq/libpq-fe.h | 1 + src/interfaces/libpq/libpq-int.h | 2 + src/test/kerberos/Makefile| 3 + src/test/kerberos/t/001_auth.pl | 331 -- src/test/perl/PostgreSQL/Test/Utils.pm| 27 ++ src/test/regress/expected/rules.out | 11 +- 39 files changed, 792 insertions(+), 143 deletions(-) diff --git a/configure b/configure index dbea7eaf5f..08bcf
Regarding Plan tree output(Index/Bitmap Scan)
Hi, I am trying to understand the Plan tree for select queries. Can you please help me with the below queries? 1) Why is there a difference in plan tree for these two queries? User table tidx1 has an index on column 'a' . 2) Why do we do Index scan and not Bitmap Index Scan for catalog tables? postgres=# explain select * from pg_class where oid=2051; QUERY PLAN - Index Scan using pg_class_oid_index on pg_class (cost=0.27..8.29 rows=1 width=265) Index Cond: (oid = '2051'::oid) (2 rows) postgres=# explain select * from tidx1 where a=1; QUERY PLAN Bitmap Heap Scan on tidx1 (cost=4.24..14.91 rows=11 width=8) Recheck Cond: (a = 1) -> Bitmap Index Scan on idx1 (cost=0.00..4.24 rows=11 width=0) Index Cond: (a = 1) (4 rows) postgres=# select * from tidx1; a | b ---+--- 1 | 2 2 | 2 3 | 2 4 | 2 5 | 2 (5 rows) Best, Aj
Re: Add LZ4 compression in pg_dump
On Tue, Apr 11, 2023 at 07:41:11PM -0500, Justin Pryzby wrote:
> Maybe I would write it as: "if zlib is unavailable, default to no
> compression". But I think that's best done in the leading comment, and
> not inside an empty preprocessor #else.
>
> I was hoping Michael would comment on this.
(Sorry for the late reply, somewhat missed that.)
> The placement and phrasing of the comment makes no sense to me.
Yes, this comment gives no value as it stands. I would be tempted to
follow the suggestion to group the whole code block in a single ifdef,
including the check, and remove this comment. Like the attached
perhaps?
--
Michael
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 967ced4eed..b7dda5ab27 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -753,16 +753,14 @@ main(int argc, char **argv)
* Custom and directory formats are compressed by default with gzip when
* available, not the others.
*/
+#ifdef HAVE_LIBZ
if ((archiveFormat == archCustom || archiveFormat == archDirectory) &&
!user_compression_defined)
{
-#ifdef HAVE_LIBZ
parse_compress_specification(PG_COMPRESSION_GZIP, NULL,
&compression_spec);
-#else
- /* Nothing to do in the default case */
-#endif
}
+#endif
/*
* If emitting an archive format, we always want to emit a DATABASE item,
signature.asc
Description: PGP signature
Re: CI and test improvements
On Wed, Mar 15, 2023 at 04:57:34PM +0100, Peter Eisentraut wrote: > On 15.03.23 15:56, Justin Pryzby wrote: > > I'm surprised if there's any question about the merits of making > > documentation easily available for review. Several people have agreed; > > one person mailed me privately specifically to ask how to show HTML docs > > on cirrusci. > > > > Anyway, all this stuff is best addressed either before or after the CF. > > I'll kick the patch forward. Thanks for looking. > > I suppose this depends on what you want to use this for. If your use is to > prepare and lay out as much information as possible about a patch for a > reviewer, some of your ideas make sense. > > I'm using this primarily to quickly test local work in progress. So I want > a quick feedback cycle. I don't need it to show me which HTML docs changed, > for example. > > So maybe there need to be different modes. I'm opened to that - for example, mingw is currently opt-in. Maybe this should be a separate task - it was implemented like that based on an earlier suggestion (and then changed back again based on another suggestion). The task could be triggered manually or by cfbot's message. But a primary goal for cirrus.yml was to allow developers to do the same things as cfbot, and without everyone needing to reimplement it for themselves. You want quick feedback, like everyone else - but I doubt you disable the documentation build when you don't need it, even though that would shave off a whole minute. And I doubt that you'd comment it out even the documentation was built twice. Anyway - I think this patch is probably waiting on Andres' patch to "convert CompilerWarnings to meson". > > 7e09035f588 WIP: ci/meson: allow showing only failed tests .. > > I'm not sure I like this one. I sometimes look up the logs of non-failed > tests to compare them with failed tests, to get context to could lead to > failures. Maybe we can make this behavior adjustable. But I've not been > bothered by the current behavior. I suggest to try the patch; I doubt you'd prefer the existing behavior. The patch is rebased now that meson is updated to avoid the windows python warnings (thanks Andres). -- Justin >From 4695da5b426731a651d90fc41f41434207596848 Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Wed, 25 May 2022 21:53:22 -0500 Subject: [PATCH 1/8] cirrus/windows: add compiler_warnings_script The goal is to fail due to warnings only after running tests. (At least historically, it's too slow to run a separate windows VM to compile with -Werror.) https://www.postgresql.org/message-id/20220212212310.f645c6vw3njkgxka%40alap3.anarazel.de I'm not sure how to write this test in windows shell; it's also easy to write something that doesn't work in posix sh, since windows shell is interpretting && and ||... See also: 8a1ce5e54f6d144e4f8e19af7c767b026ee0c956 https://cirrus-ci.com/task/6241060062494720 https://cirrus-ci.com/task/6496366607204352 ci-os-only: windows --- .cirrus.yml | 11 ++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/.cirrus.yml b/.cirrus.yml index d3f88821a85..13213ffd304 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -589,28 +589,37 @@ task: echo 127.0.0.2 pg-loadbalancetest >> c:\Windows\System32\Drivers\etc\hosts echo 127.0.0.3 pg-loadbalancetest >> c:\Windows\System32\Drivers\etc\hosts type c:\Windows\System32\Drivers\etc\hosts # Use /DEBUG:FASTLINK to avoid high memory usage during linking configure_script: | vcvarsall x64 meson setup --backend ninja --buildtype debug -Dc_link_args=/DEBUG:FASTLINK -Dcassert=true -Db_pch=true -Dextra_lib_dirs=c:\openssl\1.1\lib -Dextra_include_dirs=c:\openssl\1.1\include -DTAR=%TAR% -DPG_TEST_EXTRA="%PG_TEST_EXTRA%" build build_script: | vcvarsall x64 -ninja -C build +ninja -C build |tee build.txt +REM Since pipes lose the exit status of the preceding command, rerun the compilation +REM without the pipe, exiting now if it fails, to avoid trying to run checks +ninja -C build > nul check_world_script: | vcvarsall x64 meson test %MTEST_ARGS% --num-processes %TEST_JOBS% + # This should be last, so check_world is run even if there are warnings + always: +compiler_warnings_script: + # this avoids using metachars which would be interpretted by the windows shell + - sh -c 'if grep ": warning " build.txt; then exit 1; fi; exit 0' + on_failure: <<: *on_failure_meson crashlog_artifacts: path: "crashlog-*.txt" type: text/plain task: << : *WINDOWS_ENVIRONMENT_BASE name: Windows - Server 2019, MinGW64 - Meson -- 2.34.1 >From 62e04da9b12fc8a421957facee60d80c3c4f2ad8 Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Fri, 24 Jun 2022 00:09:12 -0500 Subject: [PATCH 2/8] cirrus/freebsd: run with more CPUs+RAM and do not repartition There was some historic problem where tests under freebsd took 8+ minutes (and before 4a288a37f took 15 minu
Re: Add LZ4 compression in pg_dump
On Mon, Feb 27, 2023 at 02:33:04PM +, gkokola...@pm.me wrote:
> > > - Finally, the "Nothing to do in the default case" comment comes from
> > > Michael's commit 5e73a6048:
> > >
> > > + /*
> > > + * Custom and directory formats are compressed by default with gzip when
> > > + * available, not the others.
> > > + /
> > > + if ((archiveFormat == archCustom || archiveFormat == archDirectory) &&
> > > + !user_compression_defined)
> > > {
> > > #ifdef HAVE_LIBZ
> > > - if (archiveFormat == archCustom || archiveFormat == archDirectory)
> > > - compressLevel = Z_DEFAULT_COMPRESSION;
> > > - else
> > > + parse_compress_specification(PG_COMPRESSION_GZIP, NULL,
> > > + &compression_spec);
> > > +#else
> > > + / Nothing to do in the default case */
> > > #endif
> > > - compressLevel = 0;
> > > }
> > >
> > > As the comment says: for -Fc and -Fd, the compression is set to zlib, if
> > > enabled, and when not otherwise specified by the user.
> > >
> > > Before 5e73a6048, this set compressLevel=0 for -Fp and -Ft, and when
> > > zlib was unavailable.
> > >
> > > But I'm not sure why there's now an empty "#else". I also don't know
> > > what "the default case" refers to.
> > >
> > > Maybe the best thing here is to move the preprocessor #if, since it's no
> > > longer in the middle of a runtime conditional:
> > >
> > > #ifdef HAVE_LIBZ
> > > + if ((archiveFormat == archCustom || archiveFormat == archDirectory) &&
> > > + !user_compression_defined)
> > > + parse_compress_specification(PG_COMPRESSION_GZIP, NULL,
> > > + &compression_spec);
> > > #endif
> > >
> > > ...but that elicits a warning about "variable set but not used"...
> >
> >
> > Not sure, I need to think about this a bit.
> /* Nothing to do for the default case when LIBZ is not available */
> is easier to understand.
Maybe I would write it as: "if zlib is unavailable, default to no
compression". But I think that's best done in the leading comment, and
not inside an empty preprocessor #else.
I was hoping Michael would comment on this.
The placement and phrasing of the comment makes no sense to me.
--
Justin
Re: v15b1: FailedAssertion("segment_map->header->magic == (DSA_SEGMENT_HEADER_MAGIC ^ area->control->handle ^ index)", File: "dsa.c", ..)
On Wed, Apr 12, 2023 at 11:37 AM Justin Pryzby wrote: > $ ls /dev/shm/ |grep 3696856876 || echo not found > not found Oh, of course it would have restarted after it crashed and unlinked that... So the remaining traces of that memory *might* be in the core file, depending (IIRC) on the core filter settings (you definitely get shared anonymous memory like our main shm region by default, but IIRC there's something extra needed if you want the shm_open'd DSM segments to be dumped too...) > (In case it matters: the vm has been up for 1558 days). I will refrain from invoking cosmic radiation at this point :-) > If it's helpful, I could provide the corefile, unstripped binaries, and > libc.so, which would be enough to use gdb on your side with "set > solib-search-path". Sounds good, thanks, please send them over off-list and I'll see if I can figure anything out ...
Re: v15b1: FailedAssertion("segment_map->header->magic == (DSA_SEGMENT_HEADER_MAGIC ^ area->control->handle ^ index)", File: "dsa.c", ..)
On Wed, Apr 12, 2023 at 11:18:36AM +1200, Thomas Munro wrote:
> Can you print *area->control?
(gdb) p *area->control
$1 = {segment_header = {magic = 216163848, usable_pages = 62, size = 1048576,
prev = 1, next = 18446744073709551615, bin = 4, freed = false}, handle = 0,
segment_handles = {0, 3696856876, 433426374, 1403332952, 2754923922,
0 }, segment_bins = {18446744073709551615,
18446744073709551615, 18446744073709551615, 18446744073709551615, 3, 4,
18446744073709551615, 18446744073709551615, 18446744073709551615,
18446744073709551615, 18446744073709551615, 18446744073709551615,
18446744073709551615, 18446744073709551615, 18446744073709551615,
18446744073709551615}, pools = {{lock = {tranche = 72, state = {value =
536870912},
waiters = {head = 2147483647, tail = 2147483647}}, spans = {0,
2199025295360, 8192, 0}}, {lock = {tranche = 72, state = {value = 536870912},
waiters = {head = 2147483647, tail = 2147483647}}, spans = {0,
2199025296088, 0, 0}}, {lock = {tranche = 72, state = {value =
536870912}, waiters = {head = 2147483647, tail = 2147483647}}, spans = {0, 0,
0, 0}}, {lock = {tranche = 72, state = {value = 536870912}, waiters = {
head = 2147483647, tail = 2147483647}}, spans = {0, 0, 0, 0}}, {lock
= {tranche = 72, state = {value = 536870912}, waiters = {head = 2147483647,
tail = 2147483647}}, spans = {0, 0, 0, 0}}, {lock = {tranche = 72,
state = {value = 536870912}, waiters = {head = 2147483647, tail =
2147483647}}, spans = {0, 0, 0, 0}}, {lock = {tranche = 72, state = {value =
536870912}, waiters = {head = 2147483647, tail = 2147483647}}, spans = {0,
0, 0, 0}}, {lock = {tranche = 72, state = {value = 536870912}, waiters
= {head = 2147483647, tail = 2147483647}}, spans = {0, 2199025296648,
2199025298608, 0}}, {lock = {tranche = 72, state = {value = 536870912},
waiters = {head = 2147483647, tail = 2147483647}}, spans = {0, 0, 0,
0}}, {lock = {tranche = 72, state = {value = 536870912}, waiters = {head =
2147483647, tail = 2147483647}}, spans = {0, 0, 0, 0}}, {lock = {
tranche = 72, state = {value = 536870912}, waiters = {head =
2147483647, tail = 2147483647}}, spans = {0, 0, 0, 0}}, {lock = {tranche = 72,
state = {value = 536870912}, waiters = {head = 2147483647,
tail = 2147483647}}, spans = {0, 0, 0, 0}}, {lock = {tranche = 72,
state = {value = 536870912}, waiters = {head = 2147483647, tail = 2147483647}},
spans = {0, 0, 0, 0}}, {lock = {tranche = 72, state = {
value = 536870912}, waiters = {head = 2147483647, tail =
2147483647}}, spans = {0, 0, 0, 0}}, {lock = {tranche = 72, state = {value =
536870912}, waiters = {head = 2147483647, tail = 2147483647}}, spans = {0, 0,
0,
0}}, {lock = {tranche = 72, state = {value = 536870912}, waiters =
{head = 2147483647, tail = 2147483647}}, spans = {0, 2199025298496,
2199025298664, 2199025297936}}, {lock = {tranche = 72, state = {
value = 536870912}, waiters = {head = 2147483647, tail =
2147483647}}, spans = {0, 0, 0, 0}}, {lock = {tranche = 72, state = {value =
536870912}, waiters = {head = 2147483647, tail = 2147483647}}, spans = {0, 0,
0,
0}}, {lock = {tranche = 72, state = {value = 536870912}, waiters =
{head = 2147483647, tail = 2147483647}}, spans = {0, 8416, 0, 0}}, {lock =
{tranche = 72, state = {value = 536870912}, waiters = {head = 2147483647,
tail = 2147483647}}, spans = {0, 0, 0, 0}}, {lock = {tranche = 72,
state = {value = 536870912}, waiters = {head = 2147483647, tail = 2147483647}},
spans = {0, 0, 0, 0}}, {lock = {tranche = 72, state = {
value = 536870912}, waiters = {head = 2147483647, tail =
2147483647}}, spans = {0, 0, 0, 0}}, {lock = {tranche = 72, state = {value =
536870912}, waiters = {head = 2147483647, tail = 2147483647}}, spans = {0, 0,
0,
0}}, {lock = {tranche = 72, state = {value = 536870912}, waiters =
{head = 2147483647, tail = 2147483647}}, spans = {0, 0, 0, 0}}, {lock =
{tranche = 72, state = {value = 536870912}, waiters = {head = 2147483647,
tail = 2147483647}}, spans = {0, 0, 0, 0}}, {lock = {tranche = 72,
state = {value = 536870912}, waiters = {head = 2147483647, tail = 2147483647}},
spans = {0, 8304, 0, 0}}, {lock = {tranche = 72, state = {
value = 536870912}, waiters = {head = 2147483647, tail =
2147483647}}, spans = {0, 0, 0, 0}}, {lock = {tranche = 72, state = {value =
536870912}, waiters = {head = 2147483647, tail = 2147483647}}, spans = {0, 0,
0,
0}}, {lock = {tranche = 72, state = {value = 536870912}, waiters =
{head = 2147483647, tail = 2147483647}}, spans = {0, 0, 0, 0}}, {lock =
{tranche = 72, state = {value = 536870912}, waiters = {head = 2147483647,
tail = 2147483647}}, spans = {0, 8528, 0, 0}}, {lock = {tranche = 72,
state = {value = 536870912}, waiters = {head = 2147483647, tail = 2147483647}},
spans = {0, 0, 0, 0}}, {lock = {tranche = 72, state = {
value = 536870912},
Re: refactoring relation extension and BufferAlloc(), faster COPY
Hi,
On 2023-04-11 22:00:00 +0300, Alexander Lakhin wrote:
> A few days later I've found a new defect introduced with 31966b151.
That's the same issue that Tom also just reported, at
https://postgr.es/m/392271.1681238924%40sss.pgh.pa.us
Attached is my WIP fix, including a test.
Greetings,
Andres Freund
>From fbe84381fa39a48f906358ade0a64e93b81c05c9 Mon Sep 17 00:00:00 2001
From: Andres Freund
Date: Tue, 11 Apr 2023 16:04:44 -0700
Subject: [PATCH v1] WIP: Fix ExtendBufferedRelTo() assert failure in recovery
+ tests
Reported-by: Alexander Lakhin
Reported-by: Tom Lane
Discussion: https://postgr.es/m/392271.1681238924%40sss.pgh.pa.us
Discussion: https://postgr.es/m/0b5eb82b-cb99-e0a4-b932-3dc60e2e3...@gmail.com
---
src/backend/storage/buffer/bufmgr.c | 4 +-
src/test/recovery/meson.build| 1 +
src/test/recovery/t/036_truncated_dropped.pl | 99
3 files changed, 102 insertions(+), 2 deletions(-)
create mode 100644 src/test/recovery/t/036_truncated_dropped.pl
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 7778dde3e57..4f2c46a4339 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -889,7 +889,7 @@ ExtendBufferedRelTo(ExtendBufferedWhat eb,
Assert(eb.smgr == NULL || eb.relpersistence != 0);
Assert(extend_to != InvalidBlockNumber && extend_to > 0);
Assert(mode == RBM_NORMAL || mode == RBM_ZERO_ON_ERROR ||
- mode == RBM_ZERO_AND_LOCK);
+ mode == RBM_ZERO_AND_LOCK || mode == RBM_ZERO_AND_CLEANUP_LOCK);
if (eb.smgr == NULL)
{
@@ -933,7 +933,7 @@ ExtendBufferedRelTo(ExtendBufferedWhat eb,
*/
current_size = smgrnblocks(eb.smgr, fork);
- if (mode == RBM_ZERO_AND_LOCK)
+ if (mode == RBM_ZERO_AND_LOCK || mode == RBM_ZERO_AND_CLEANUP_LOCK)
flags |= EB_LOCK_TARGET;
while (current_size < extend_to)
diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build
index e834ad5e0dc..20089580100 100644
--- a/src/test/recovery/meson.build
+++ b/src/test/recovery/meson.build
@@ -41,6 +41,7 @@ tests += {
't/033_replay_tsp_drops.pl',
't/034_create_database.pl',
't/035_standby_logical_decoding.pl',
+ 't/036_truncated_dropped.pl',
],
},
}
diff --git a/src/test/recovery/t/036_truncated_dropped.pl b/src/test/recovery/t/036_truncated_dropped.pl
new file mode 100644
index 000..571cff9639a
--- /dev/null
+++ b/src/test/recovery/t/036_truncated_dropped.pl
@@ -0,0 +1,99 @@
+# Copyright (c) 2021-2023, PostgreSQL Global Development Group
+
+# Tests recovery scenarios where the files are shorter than in the common
+# cases, e.g. due to replaying WAL records of a relation that was subsequently
+# truncated.
+
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node = PostgreSQL::Test::Cluster->new('n1');
+
+$node->init();
+
+# Disable autovacuum to guarantee VACUUM can remove rows / truncate relations
+$node->append_conf('postgresql.conf', qq[
+wal_level = 'replica'
+autovacuum = off
+]);
+
+$node->start();
+
+
+# Test replay of PRUNE records for blocks that are later truncated. With FPIs
+# used for PRUNE.
+
+$node->safe_psql(
+ 'postgres', qq[
+CREATE TABLE truncme(i int) WITH (fillfactor = 50);
+INSERT INTO truncme SELECT generate_series(1, 1000);
+UPDATE truncme SET i = 1;
+CHECKPOINT; -- generate FPIs
+VACUUM truncme; -- generate prune records
+TRUNCATE truncme; -- make blocks non-existing
+INSERT INTO truncme SELECT generate_series(1, 10);
+]);
+
+$node->stop('immediate');
+
+ok($node->start(), 'replay of PRUNE records affecting truncated block (FPIs)');
+
+is($node->safe_psql('postgres', 'select count(*), sum(i) FROM truncme'),
+ '10|55',
+ 'table contents as expected after recovery');
+$node->safe_psql('postgres', 'DROP TABLE truncme');
+
+
+# Test replay of PRUNE records for blocks that are later truncated. Without
+# FPIs used for PRUNE.
+
+$node->safe_psql(
+ 'postgres', qq[
+CREATE TABLE truncme(i int) WITH (fillfactor = 50);
+INSERT INTO truncme SELECT generate_series(1, 1000);
+UPDATE truncme SET i = 1;
+VACUUM truncme; -- generate prune records
+TRUNCATE truncme; -- make blocks non-existing
+INSERT INTO truncme SELECT generate_series(1, 10);
+]);
+
+$node->stop('immediate');
+
+ok($node->start(), 'replay of PRUNE records affecting truncated block (no FPIs)');
+
+is($node->safe_psql('postgres', 'select count(*), sum(i) FROM truncme'),
+ '10|55',
+ 'table contents as expected after recovery');
+$node->safe_psql('postgres', 'DROP TABLE truncme');
+
+
+# Test replay of partial relation truncation via VACUUM
+
+$node->safe_psql(
+ 'postgres', qq[
+CREATE TABLE truncme(i int) WITH (fillfactor = 50);
+INSERT INTO truncme SELECT generate_series(1, 1000);
+UPDATE truncme SET i = i + 1;
+-- ensure a mix of pre/post truncation rows
+DELETE FROM truncme WHERE i > 500;
+
+VACUUM truncme; -- should truncate relation
+
+-- rows at TIDs that previo
Re: v15b1: FailedAssertion("segment_map->header->magic == (DSA_SEGMENT_HEADER_MAGIC ^ area->control->handle ^ index)", File: "dsa.c", ..)
On Wed, Apr 12, 2023 at 7:46 AM Justin Pryzby wrote: > Unfortunately: > (gdb) p area->control->handle > $3 = 0 > (gdb) p segment_map->header->magic > value has been optimized out > (gdb) p index > $4 = Hmm, well index I can find from parameters: > #2 0x00991470 in ExceptionalCondition (conditionName= optimized out>, errorType=, fileName= out>, lineNumber=1770) at assert.c:69 > #3 0x009b9f97 in get_segment_by_index (area=0x22818c0, index= optimized out>) at dsa.c:1769 > #4 0x009ba192 in dsa_get_address (area=0x22818c0, dp=1099511703168) > at dsa.c:953 We have dp=1099511703168 == 0x1012680, so index == 1 and the rest is the offset into that segment. It's not the initial segment in the main shared memory area created by the postmaster with dsa_create_in_place() (that'd be index 0), it's in an extra segment that was created with shm_open(). We managed to open and mmap() that segment, but it contains unexpected garbage. Can you print *area->control? And then can you see that the DSM handle is in index 1 in "segment_handles" in there? Then can you see if your system has a file with that number in its name under /dev/shm/, and can you tell me what "od -c /dev/shm/..." shows as the first few lines of stuff at the top, so we can see what that unexpected garbage looks like? Side rant: I don't think there's any particular indication that it's the issue here, but while it's on my mind: I really wish we didn't use random numbers for DSM handles. I understand where it came from: the need to manage SysV shmem keyspace (a DSM mode that almost nobody uses, but whose limitations apply to all modes). We've debugged issues relating to handle collisions before, causing unrelated DSM segments to be confused, back when the random seed was not different in each backend making collisions likely. For every other mode, we could instead use something like (slot, generation) to keep collisions as far apart as possible (generation wraparound), and avoid collisions between unrelated clusters by using the pgdata path as a shm_open() prefix. Another idea is to add a new DSM mode that would use memfd and similar things and pass fds between backends, so that the segments are entirely anonymous and don't need to be cleaned up after a crash (I thought about that while studying the reasons why PostgreSQL can't run on Capsicum (a capabilities research project) or Android (a telephone), both of which banned SysV *and* POSIX shm because system-global namespaces are bad).
Re: Show various offset arrays for heap WAL records
On Tue, Apr 11, 2023 at 2:29 PM Melanie Plageman wrote: > > That doesn't seem great to me either. I don't like this ambiguity, > > because it seems like it makes the description hard to parse in a way > > that flies in the face of what we're trying to do here, in general. > > So it seems like it might be worth fixing now, in the scope of this > > patch. > > Agreed. Great -- pushed a fix for this just now, which included that change. > I agree it would be nice for xl_heap_lock->locking_xid to be renamed > xmax for clarity. I would suggest that if you don't intend to put it > in a separate commit, you mention it explicitly in the final commit > message. Its motivation isn't immediately obvious to the reader. What I ended up doing is making that part of a bug fix for a minor buglet I noticed in passing -- it became part of the "Fix xl_heap_lock WAL record field's data type" commit from a bit earlier on. Thanks for your help with the follow-up work. Seems like we're done with this now. -- Peter Geoghegan
Re: When to drop src/tools/msvc support
Hi,
On 2023-04-11 19:44:20 +0200, Alvaro Herrera wrote:
> On 2023-Apr-11, Michael Paquier wrote:
>
> > Getting a CI job able to do some validation for MSVC would be indeed
> > nice. What's the plan in the buildfarm with this coverage? Would all
> > the animals switch to meson (Chocolatey + StrawberryPerl, I assume)
> > for the job or will there still be some coverage with MSVC for v16
> > there?
>
> If we keep MSVC support in 16, then I agree we should have a CI task for
> it -- and IMO we should make ti automatically triggers whenever any
> Makefile or meson.build is modified. Hopefully we won't touch them
> much now that the branch is feature-frozen, but it could still happen.
Once 16 branched, we can just have it always run, I think. It's just the
development branch where it's worth avoiding that (for cfbot and personal
hackery).
I guess we could do something like:
manual: "changesInclude('**.meson.build', '**Makefile*', '**.mk',
'src/tools/msvc/**')"
so the task would be manual triggered if none of those files change. If you
have write rights on the repository in question, you can trigger manual tasks
with a click.
> Do we have code for it already, even if incomplete?
My meson branch has a commit adding a bunch of additional tasks. Including
building with src/tools/msvc, building with meson + msbuild, openbsd, netbsd.
https://github.com/postgres/postgres/commit/8f7c2ffb5a5e8f0ef3722e2439484187c1356416
Currently src/tools/msvc does build successfully, although the tests haven't
finished yet:
https://cirrus-ci.com/build/6298699714789376
(the cause for the opensuse failure is known, need to find cycles to tackle
that, not related to meson)
Greetings,
Andres Freund
RE: [PATCH] Support % wildcard in extension upgrade filenames
> Personally I don't see the benefit of 1 big file vs. many 0-length files to justify > the cost (time and complexity) of a PostgreSQL change, with the > corresponding cost of making use of this new functionality based on > PostgreSQL version. > >From a packaging stand-point 1 big file is better than tons of 0-length files. Fewer files to uninstall and to double check when testing. As to the added complexity agree it's more but in my mind worth it if we could get rid of this mountain of files. But my vote would be the wild-card solution as I think it would serve more than just postgis need. Any project that rarely does anything but add, remove, or modify functions doesn't really need multiple upgrade scripts and I think quite a few extensions fall in that boat. > We'd still have the problem of missing upgrade paths unless we release a new > version of PostGIS even if it's ONLY for the sake of updating that 1 big file (or > adding a new file, in the current situation). > > --strk; I think we always have more releases with newer stable versions than older stable versions. I can't remember a case when we had this ONLY issue. If there is one fix in an older stable, version we usually wait for several more before bothering with a release and then all the stables are released around the same time. So I don't see the ONLY being a real problem.
Re: ERROR messages in VACUUM's PARALLEL option
David Rowley writes: > Thanks for chipping in on this. Can you confirm if you think this > should apply to VACUUM options? We're not talking GUCs here. My druthers would be to treat them similarly to GUCs. I recognize that I might be in the minority, and that doing so would entail touching a lot of existing messages in this area. Nonetheless, I think this area is not being consistent with our wider conventions, which is why for example defGetString is out of step with these messages. regards, tom lane
Re: Various typo fixes
On Tue, Apr 11, 2023 at 09:53:00AM -0500, Justin Pryzby wrote: > On Tue, Apr 11, 2023 at 03:43:12PM +0100, Thom Brown wrote: > > On Tue, 11 Apr 2023 at 15:39, Justin Pryzby wrote: > > > > > > On Tue, Apr 11, 2023 at 03:36:02PM +0100, Thom Brown wrote: > > > > I've attached a patch with a few typo and grammatical fixes. > > > > > > I think you actually sent the "git-diff" manpage :( > > > > Oh dear, well that's a first. Thanks for pointing out. > > Thanks. I think these are all new in v16, right ? > > I noticed some of these too - I'll send a patch pretty soon. The first attachment fixes for typos in user-facing docs new in v16, combining Thom's changes with the ones that I'd found. If that's confusing, I'll resend my patches separately. The other four numbered patches could use extra review. -- Justin diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml index 7cf44e82e23..7064307d9e6 100644 --- a/doc/src/sgml/archive-modules.sgml +++ b/doc/src/sgml/archive-modules.sgml @@ -50,7 +50,7 @@ _PG_archive_module_init. The result of the function must be a pointer to a struct of type ArchiveModuleCallbacks, which contains everything - that the core code needs to know how to make use of the archive module. The + that the core code needs to know to make use of the archive module. The return value needs to be of server lifetime, which is typically achieved by defining it as a static const variable in global scope. @@ -82,7 +82,7 @@ typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void); The startup_cb callback is called shortly after the module is loaded. This callback can be used to perform any additional -initialization required. If the archive module has a state, it can use +initialization required. If the archive module has any state, it can use state->private_data to store it. @@ -143,7 +143,7 @@ typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, cons process exits (e.g., after an error) or the value of changes. If no shutdown_cb is defined, no special action is taken in -these situations. If the archive module has a state, this callback should +these situations. If the archive module has any state, this callback should free it to avoid leaks. diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml index b9d73deced2..bcdc085010a 100644 --- a/doc/src/sgml/client-auth.sgml +++ b/doc/src/sgml/client-auth.sgml @@ -810,7 +810,7 @@ hostall all ::1/128 trust hostall all localhost trust # The same using a regular expression for DATABASE, that allows connection -# to the database db1, db2 and any databases with a name beginning by "db" +# to the database db1, db2 and any databases with a name beginning with "db" # and finishing with a number using two to four digits (like "db1234" or # "db12"). # diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index f81c2045ec4..20b8b5ae1de 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -11835,7 +11835,7 @@ LOG: CleanUpLock: deleting: lock(0xb7acd844) id(24688,24696,0,0,0,1) option of CREATE SUBSCRIPTION is enabled, otherwise, serialize each change. When set to -buffered, the decoding will stream or serialize +buffered, decoding will stream or serialize changes when logical_decoding_work_mem is reached. diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index e6a7514100e..9c3b2018896 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -27084,8 +27084,9 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset Take a snapshot of running transactions and write it to WAL, without -having to wait bgwriter or checkpointer to log one. This is useful for -logical decoding on standby, as logical slot creation has to wait +having to wait for bgwriter or +checkpointer to log one. This is useful for +logical decoding on a standby, as logical slot creation has to wait until such a record is replayed on the standby. diff --git a/doc/src/sgml/indexam.sgml b/doc/src/sgml/indexam.sgml index 29ece7c42ee..e813e2b620a 100644 --- a/doc/src/sgml/indexam.sgml +++ b/doc/src/sgml/indexam.sgml @@ -257,7 +257,7 @@ typedef struct IndexAmRoutine Access methods that do not point to individual tuples, but to block ranges (like BRIN), may allow the HOT optimization to continue. This does not apply to attributes referenced in index - predicates, an update of such attribute always disables HOT. + predicates, an update of such an attribute always disables HOT. diff --git a/doc/src/sgml/install-windows.sgml b/doc/src/sgml/install-windows.sgml index 2db44db2
Re: Assertion being hit during WAL replay
Hi,
On 2023-04-11 16:54:53 -0400, Tom Lane wrote:
> Andres Freund writes:
> > On 2023-04-11 14:48:44 -0400, Tom Lane wrote:
> >> I have seen this failure a couple of times recently while
> >> testing code that caused crashes and restarts:
>
> > Do you have a quick repro recipe?
>
> Here's something related to what I hit that time:
>
> diff --git a/src/backend/optimizer/plan/subselect.c
> b/src/backend/optimizer/plan/subselect.c
> index 052263aea6..d43a7c7bcb 100644
> --- a/src/backend/optimizer/plan/subselect.c
> +++ b/src/backend/optimizer/plan/subselect.c
> @@ -2188,6 +2188,7 @@ SS_charge_for_initplans(PlannerInfo *root, RelOptInfo
> *final_rel)
> void
> SS_attach_initplans(PlannerInfo *root, Plan *plan)
> {
> + Assert(root->init_plans == NIL);
> plan->initPlan = root->init_plans;
> }
>
> You won't get through initdb with this, but if you install this change
> into a successfully init'd database and then "make installcheck-parallel",
> it will crash and then fail to recover, at least a lot of the time.
Ah, that allowed me to reproduce. Thanks.
Took me a bit to understand how we actually get into this situation. A PRUNE
record for relation+block that doesn't exist during recovery. That doesn't
commonly happen outside of PITR or such, because we obviously need a block
with content to generate the PRUNE. The way it does happen here, is that the
relation is vacuumed and then truncated. Then we crash. Thus we end up with a
PRUNE record for a block that doesn't exist on disk.
Which is also why the test is quite timing sensitive.
Seems like it'd be good to have a test that covers this scenario. There's
plenty code around it that doesn't currently get exercised.
None of the existing tests seem like a great fit. I guess it could be added to
013_crash_restart, but that really focuses on something else.
So I guess I'll write a 036_notsureyet.pl...
Greetings,
Andres Freund
Re: Can we do something to help stop users mistakenly using force_parallel_mode?
On Wed, 12 Apr 2023 at 09:53, Tom Lane wrote:
>
> David Rowley writes:
> > In preparation for when that's ticked off, I'd like to gather people's
> > thoughts about if we should remove force_parallel_mode from v16?
>
> To clarify, you just mean removing that alias, right? +1.
> I don't see a reason to wait longer once the buildfarm is on board.
Yip, alias. i.e:
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index ea67cfa5e5..7d3b20168a 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -186,7 +186,6 @@ static const unit_conversion time_unit_conversion_table[] =
static const char *const map_old_guc_names[] = {
"sort_mem", "work_mem",
"vacuum_mem", "maintenance_work_mem",
- "force_parallel_mode", "debug_parallel_query",
NULL
};
David
Re: ERROR messages in VACUUM's PARALLEL option
(On Wed, 12 Apr 2023 at 01:58, Tom Lane wrote: > > David Rowley writes: > > Over in [1], Horiguchisan mentioned a few things about VACUUM's new > > BUFFER_USAGE_LIMIT option. > > > 1) buffer_usage_limit in the ERROR messages should be consistently in > > uppercase. > > FWIW, I think this is exactly backward, and so is whatever code you > based this on. Our usual habit is to write GUC names and suchlike > in lower case in error messages. Add double quotes if you feel you > want to set them off from the surrounding text. Here's a typical > example of longstanding style: Thanks for chipping in on this. Can you confirm if you think this should apply to VACUUM options? We're not talking GUCs here. I think there might be some precedents creeping in here for the legacy VACUUM syntax. Roughly the current ERROR messages are using upper case. e.g: "PROCESS_TOAST required with VACUUM FULL" "VACUUM option DISABLE_PAGE_SKIPPING cannot be used with FULL" "ANALYZE option must be specified when a column list is provided" It's quite possible the newer options copied what VACUUM FULL did, and VACUUM FULL was probably upper case from before we had the newer syntax with the options in parenthesis. I did notice that defGetString() did lower case, but that not exactly terrible. It seems worse to have ExecVacuum() use a random assortment of casings when reporting errors in the specified options. Unless someone thinks are should edit all of those to lower case, then I think the best option is just to make BUFFER_USAGE_LIMIT follow the existing precedent of upper casing. David
Re: Can we do something to help stop users mistakenly using force_parallel_mode?
David Rowley writes: > In preparation for when that's ticked off, I'd like to gather people's > thoughts about if we should remove force_parallel_mode from v16? To clarify, you just mean removing that alias, right? +1. I don't see a reason to wait longer once the buildfarm is on board. regards, tom lane
Re: Can we do something to help stop users mistakenly using force_parallel_mode?
On Thu, 16 Feb 2023 at 00:05, David Rowley wrote: > I pushed the rename patch earlier. > > How should we go about making contact with the owners? After a quick round of making direct contact with the few remaining buildfarm machine owners which are still using force_parallel_mode, we're now down to just one remainder (warbler). In preparation for when that's ticked off, I'd like to gather people's thoughts about if we should remove force_parallel_mode from v16? My thoughts are that providing we can get the remaining animal off it and remove the GUC alias before beta1, we should remove it. Renaming the GUC to debug_parallel_query was entirely aimed at breaking things for people who are (mistakenly) using it, so I don't see why we shouldn't remove it. Does anyone feel differently? David
Re: Documentation for building with meson
Hi, On Tue, Apr 11, 2023 at 10:18 AM Andres Freund wrote: > Hi, > > On 2023-03-28 12:27:26 -0700, samay sharma wrote: > > Subject: [PATCH v9 1/5] Make minor additions and corrections to meson > docs > > > > This commit makes a few corrections to the meson docs > > and adds a few instructions and links for better clarity. > > --- > > doc/src/sgml/installation.sgml | 24 +++- > > 1 file changed, 15 insertions(+), 9 deletions(-) > > > > diff --git a/doc/src/sgml/installation.sgml > b/doc/src/sgml/installation.sgml > > index 70ab5b77d8..e3b9b6c0d0 100644 > > --- a/doc/src/sgml/installation.sgml > > +++ b/doc/src/sgml/installation.sgml > > @@ -2057,8 +2057,7 @@ meson setup build -Dssl=openssl > > > > meson configure -Dcassert=true > > > > -meson configure's commonly used command-line > options > > -are explained in . > > +Commonly used build options for meson configure > (and meson setup) are explained in linkend="meson-options"/>. > > > > > > > > @@ -2078,6 +2077,13 @@ ninja > > processes used with the command line argument -j. > > > > > > + > > +If you want to build the docs, you can type: > > + > > +ninja docs > > + > > + > > "type" sounds a bit too, IDK, process oriented. "To build the docs, use"? > Sure, that works. > > > > Subject: [PATCH v9 2/5] Add data layout options sub-section in > installation > > docs > > > > This commit separates out blocksize, segsize and wal_blocksize > > options into a separate Data layout options sub-section in both > > the make and meson docs. They were earlier in a miscellaneous > > section which included several unrelated options. This change > > also helps reduce the repetition of the warnings that changing > > these parameters breaks on-disk compatibility. > > Makes sense. I'm planning to apply this unless Peter or somebody else has > further feedback. > Cool. > > > > From 11d82aa49efb3d1cbc08f14562a757f115053c8b Mon Sep 17 00:00:00 2001 > > From: Samay Sharma > > Date: Mon, 13 Feb 2023 16:23:52 -0800 > > Subject: [PATCH v9 3/5] Remove Anti-Features section from Installation > from > > source docs > > > > Currently, several meson setup options are listed in anti-features. > > However, they are similar to most other options in the postgres > > features list as they are 'auto' features themselves. Also, other > > options are likely better suited to the developer options section. > > This commit, therefore, moves the options listed in the anti-features > > section into other sections and removes that section. > > > > For consistency, this reorganization has been done on the make section > > of the docs as well. > > Makes sense to me. "Anti-Features" is confusing as a name to start with. > > Greetings, > > Andres Freund >
Re: Show various offset arrays for heap WAL records
On Tue, Apr 11, 2023 at 1:35 PM Peter Geoghegan wrote: > > On Tue, Apr 11, 2023 at 7:40 AM Melanie Plageman > wrote: > > Not the fault of this patch, but I also noticed that heap UPDATE and > > HOT_UPDATE records have xmax twice and don't differentiate between new > > and old. I think that was probably a mistake. > > > > description | off: 119, xmax: 1105, flags: 0x00, old_infobits: > > [], new off: 100, xmax 0 > > That doesn't seem great to me either. I don't like this ambiguity, > because it seems like it makes the description hard to parse in a way > that flies in the face of what we're trying to do here, in general. > So it seems like it might be worth fixing now, in the scope of this > patch. Agreed. On Tue, Apr 11, 2023 at 3:22 PM Peter Geoghegan wrote: > > On Tue, Apr 11, 2023 at 11:48 AM Peter Geoghegan wrote: > > Attached revision deals with this by spelling out the names in full > > (e.g., "old_xmax" and "new_xmax"). It also reorders the output fields > > to match the order from the physical UPDATE, HOT_UPDATE, and LOCK WAL > > record types, on the theory that those should match the physical > > record (unless there is a good reason not to, which doesn't apply > > here). > > I just noticed that we don't even show xmax in the case of DELETE > records. Perhaps the original assumption is that it must match the > record's own XID, but that's not true after the MultiXact enhancements > for foreign key locking added to 9.3 (and in any case there is no > reason at all to show xmax in UPDATE but not in DELETE). > > Attached revision v4 fixes this, making DELETE, UPDATE, HOT_UPDATE, > LOCK, and LOCK_UPDATED record types consistent with each other in > terms of the key names output by the heap desc routine. The field > order also needed a couple of tweaks for struct consistency (and > cross-record consistency) for v4. Code in v4 all seems fine to me. I like the update guidelines comment. I agree it would be nice for xl_heap_lock->locking_xid to be renamed xmax for clarity. I would suggest that if you don't intend to put it in a separate commit, you mention it explicitly in the final commit message. Its motivation isn't immediately obvious to the reader. - Melanie
Re: [PATCH] Support % wildcard in extension upgrade filenames
On Tue, Apr 11, 2023 at 04:36:04PM -0400, Regina Obe wrote: > > > Hey, best would be having support for wildcard wouldn't it ? > > I'm a woman of compromise. Sure 1 file would be ideal, but > I'd rather live with a big file listing all version upgrades > than 1000 files with the same information. Personally I don't see the benefit of 1 big file vs. many 0-length files to justify the cost (time and complexity) of a PostgreSQL change, with the corresponding cost of making use of this new functionality based on PostgreSQL version. We'd still have the problem of missing upgrade paths unless we release a new version of PostGIS even if it's ONLY for the sake of updating that 1 big file (or adding a new file, in the current situation). --strk;
Re: When to drop src/tools/msvc support
On 2023-Apr-11, Michael Paquier wrote: > Getting a CI job able to do some validation for MSVC would be indeed > nice. What's the plan in the buildfarm with this coverage? Would all > the animals switch to meson (Chocolatey + StrawberryPerl, I assume) > for the job or will there still be some coverage with MSVC for v16 > there? If we keep MSVC support in 16, then I agree we should have a CI task for it -- and IMO we should make ti automatically triggers whenever any Makefile or meson.build is modified. Hopefully we won't touch them much now that the branch is feature-frozen, but it could still happen. Do we have code for it already, even if incomplete? -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Re: Various typo fixes
> On 11 Apr 2023, at 16:53, Justin Pryzby wrote: > I think "logical" should be a here. Agree, it should in order to be consistent. -- Daniel Gustafsson
Re: infobits_set WAL record struct field is int8
On Tue, Apr 11, 2023 at 1:48 PM Andres Freund wrote: > Makes sense. Looks like there never was a flag defined for the sign bit, > luckily. I assume you're just going to apply this for HEAD? Yes. I'm also going to rename the TransactionId field to "xmax", for consistency with nearby very similar records (like xl_heap_lock_updated). -- Peter Geoghegan
Re: Assertion being hit during WAL replay
Andres Freund writes:
> On 2023-04-11 14:48:44 -0400, Tom Lane wrote:
>> I have seen this failure a couple of times recently while
>> testing code that caused crashes and restarts:
> Do you have a quick repro recipe?
Here's something related to what I hit that time:
diff --git a/src/backend/optimizer/plan/subselect.c
b/src/backend/optimizer/plan/subselect.c
index 052263aea6..d43a7c7bcb 100644
--- a/src/backend/optimizer/plan/subselect.c
+++ b/src/backend/optimizer/plan/subselect.c
@@ -2188,6 +2188,7 @@ SS_charge_for_initplans(PlannerInfo *root, RelOptInfo
*final_rel)
void
SS_attach_initplans(PlannerInfo *root, Plan *plan)
{
+ Assert(root->init_plans == NIL);
plan->initPlan = root->init_plans;
}
You won't get through initdb with this, but if you install this change
into a successfully init'd database and then "make installcheck-parallel",
it will crash and then fail to recover, at least a lot of the time.
regards, tom lane
Re: infobits_set WAL record struct field is int8
Hi,
On 2023-04-11 13:13:49 -0700, Peter Geoghegan wrote:
> Commit 0ac5ad5134 ("Improve concurrency of foreign key locking") added
> infobits_set fields to certain WAL records. However, in the case of
> xl_heap_lock, it made the data type int8 rather than uint8.
>
> I believe that this was a minor oversight. Attached patch fixes the issue.
Makes sense. Looks like there never was a flag defined for the sign bit,
luckily. I assume you're just going to apply this for HEAD?
Greetings,
Andres Freund
Re: is_superuser is not documented
On Tue, Apr 11, 2023 at 9:37 AM Fujii Masao wrote: >> > Yes, this patch moves the descriptions of is_superuser to config.sgml >> > and changes its group to PRESET_OPTIONS. >> >> is_superuser feels a little out of place in this file. All of >> the options here apply to the entire PostgreSQL server, while >> is_superuser only applies to the current session. > >Aren't other preset options like lc_collate, lc_ctype and server_encoding >similar to is_superuser? They seem to behave in a similar way as their >settings can be different for each connection depending on the connected database. I think the difference is that all of those options are constant for all connections to the same database and once the database is created they are immutable. is_superuser is set on a per session basis and can be changed at any time. Looking through the options it actually looks like all the options are set either when the server is built, the server is started, or the database is created, and once they're set they become immutable. The one exception I see is in_hot_standby mode which can be updated from on to off (I can't remember off the top of my head if it can be updated the other way). I'm moving the goal post a bit but I think preset may imply that the value isn't going to change once it's been set. Having said all that I actually think this is the best place for is_superuser since it doesn't seem to fit in anywhere else. >> I'm not familiar with the origins of is_superuser and it may be too >> late for this, but it seems like is_superuser would fit in much better >> as a system information function [0] rather than a GUC. Particularly >> in the Session Information Functions. > >I understand your point, but I think it would be more confusing to document >is_superuser there because it's defined and behaves differently from >session information functions like current_user. For instance, >the value of is_superuser can be displayed using SHOW command, >while current_user cannot. Therefore, it's better to keep is_superuser >separate from the session information functions. I was implying that I thought it would have made more sense for is_superuser to be implemented as a function, behave as a function, and not be visible via SHOW. However, there may have been a good reason not to do this and it may already be too late for that. In my opinion, this is ready to be committed. However, like I said earlier I'm not very familiar with the GUC code so you may want to wait for another opinion. Thanks, Joe Koshakow
Re: Minimal logical decoding on standbys
Andres Freund writes: > I think we should lower the log level, but perhaps wait for a few more cycles > in case there are random failures? Removing -log_min_messages = 'debug2' -log_error_verbosity = verbose not only reduces 035's log output volume from 1.6MB to 260kB, but also speeds it up nontrivially: on my machine it takes about 8.50 sec as of HEAD, but 8.09 sec after silencing the extra logging. So I definitely want to see that out of there. regards, tom lane
Re: v15b1: FailedAssertion("segment_map->header->magic == (DSA_SEGMENT_HEADER_MAGIC ^ area->control->handle ^ index)", File: "dsa.c", ..)
Hi, On 2023-04-11 13:35:38 -0700, Andres Freund wrote: > On 2023-04-11 14:46:23 -0500, Justin Pryzby wrote: > > Yes, $SUBJECT is correct. > > > > On an old centos6 VM which I'd forgotten about and never removed from > > monitoring, I noticed that a process had recently crashed... > > > > Maybe this is an issue which was already fixed, but I looked and find no > > bug report nor patch about it. Feel free to dismiss the problem report > > if it's not interesting or useful. > > > postgres was compiled locally at 4e54d231a. It'd been running > > continuously since September without crashing until a couple weeks ago > > (and running nearly-continuously for months before that). > > It possibly could be: > > Author: Andres Freund > Branch: master [cb2e7ddfe] 2022-12-02 18:10:30 -0800 > Branch: REL_15_STABLE Release: REL_15_2 [c6a60471a] 2022-12-02 18:07:47 -0800 > Branch: REL_14_STABLE Release: REL_14_7 [6344bc097] 2022-12-02 18:10:30 -0800 > Branch: REL_13_STABLE Release: REL_13_10 [7944d2d8c] 2022-12-02 18:13:40 -0800 > Branch: REL_12_STABLE Release: REL_12_14 [35b99a18f] 2022-12-02 18:16:14 -0800 > Branch: REL_11_STABLE Release: REL_11_19 [af3517c15] 2022-12-02 18:17:54 -0800 > > Prevent pgstats from getting confused when relkind of a relation changes > > But the fact that it's on a catalog table's stats makes it less likely, > although not impossible. > > > Any chance there were conversions from tables to views in that connection? Nope, not possible - the stack trace actually shows this is during connection establishment. Thomas, see stack trace upthread? Greetings, Andres Freund
RE: [PATCH] Support % wildcard in extension upgrade filenames
> Packager might actually know better in that they could ONLY consider the > packages ever packaged by them. > I'm a special case packager cause I'm on the PostGIS project and I only package postgis related extensions, but even I find this painful. But for most packagers, I think they are juggling too many packages and too many OS versions to micro manage the business of each package. In my case my job is simple. I deal just with Windows and that doesn't change from Windows version to Windows version (just PG specific). Think of upgrading from Debian 10 to Debian 12 - what would you as a PG packager expect people to be running and upgrading from? They could be switching from say the main distro to the pgdg distro. > Hey, best would be having support for wildcard wouldn't it ? > For PostGIS yes and any other extension that does nothing but add new functions or replaces existing ones. For others some minor handling would be ideal, though I guess some other projects would be happy with a wildcard (e.g. pgRouting would prefer a wildcard) since most of the changes are just additions of new functions or replacements of existing functions. For something like h3-pg I think a simpler micro handling would be ideal, though not sure. They ship two extensions (one that is a bridge to postgis and their newest takes advantage of postgis_raster too) https://github.com/zachasme/h3-pg/tree/main/h3_postgis/sql/updates https://github.com/zachasme/h3-pg/tree/main/h3/sql/updates Many of their upgrades are No-ops cause they really are just lib upgrades. I'm thinking maybe we should discuss these ideas with projects who would benefit most from this: (many of which I'm familiar with because they are postgis offsprings and I package or plan to package them - pgRouting, h3-pg, pgPointCloud, mobilityDb, Not PostGIS offspring: ZomboDB - https://github.com/zombodb/zombodb/tree/master/sql - (most of those are just changing versioning on the function call, so yah wildcard would be cleaner there) TimescaleDB - https://github.com/timescale/timescaledb/tree/main/sql/updates ( I don't think a wildcard would work here especially since they have some downgrade paths, but is a useful example of a micro-level extension upgrade pattern we should think about if we could make easier) https://github.com/timescale/timescaledb/tree/main/sql/updates > > I much preferred the idea of just listing all our upgrade targets in the > control file. > > Again: how would we know all upgrade targets ? > We already do, remember you wrote it :) https://git.osgeo.org/gitea/postgis/postgis/src/branch/master/extensions/upg radeable_versions.mk Yes it does require manual updating each release cycle (and putting in versions from just released stable branches). I can live with continuing with that exercise. It was a nice to have but is not nearly as annoying as 1000 scripts or as a packager trying to catalog what versions of packages have I released that I need to worry about. > > We need to come up with a convention of how to describe a micro > > update, as it's really a problem with extensions that follow the > > pattern > > I think it's a problem with extensions maintaining stable branches, as if the > history was linear we would possibly need less files (although at this stage > any number bigger than 1 would be too much for me) > > --strk; I'm a woman of compromise. Sure 1 file would be ideal, but I'd rather live with a big file listing all version upgrades than 1000 files with the same information. It's cleaner to read a single file than make sense of a pile of files.
Re: v15b1: FailedAssertion("segment_map->header->magic == (DSA_SEGMENT_HEADER_MAGIC ^ area->control->handle ^ index)", File: "dsa.c", ..)
Hi, On 2023-04-11 14:46:23 -0500, Justin Pryzby wrote: > Yes, $SUBJECT is correct. > > On an old centos6 VM which I'd forgotten about and never removed from > monitoring, I noticed that a process had recently crashed... > > Maybe this is an issue which was already fixed, but I looked and find no > bug report nor patch about it. Feel free to dismiss the problem report > if it's not interesting or useful. > postgres was compiled locally at 4e54d231a. It'd been running > continuously since September without crashing until a couple weeks ago > (and running nearly-continuously for months before that). It possibly could be: Author: Andres Freund Branch: master [cb2e7ddfe] 2022-12-02 18:10:30 -0800 Branch: REL_15_STABLE Release: REL_15_2 [c6a60471a] 2022-12-02 18:07:47 -0800 Branch: REL_14_STABLE Release: REL_14_7 [6344bc097] 2022-12-02 18:10:30 -0800 Branch: REL_13_STABLE Release: REL_13_10 [7944d2d8c] 2022-12-02 18:13:40 -0800 Branch: REL_12_STABLE Release: REL_12_14 [35b99a18f] 2022-12-02 18:16:14 -0800 Branch: REL_11_STABLE Release: REL_11_19 [af3517c15] 2022-12-02 18:17:54 -0800 Prevent pgstats from getting confused when relkind of a relation changes But the fact that it's on a catalog table's stats makes it less likely, although not impossible. Any chance there were conversions from tables to views in that connection? Greetings, Andres Freund
infobits_set WAL record struct field is int8
Commit 0ac5ad5134 ("Improve concurrency of foreign key locking") added
infobits_set fields to certain WAL records. However, in the case of
xl_heap_lock, it made the data type int8 rather than uint8.
I believe that this was a minor oversight. Attached patch fixes the issue.
--
Peter Geoghegan
v1-0001-Fix-xl_heap_lock.infobits_set-type.patch
Description: Binary data
Re: Minimal logical decoding on standbys
Hi, On 2023-04-11 11:04:50 +0200, Drouvot, Bertrand wrote: > On 4/11/23 10:55 AM, Drouvot, Bertrand wrote: > > Hi, > > > > I think we might want to add: > > > > $node_primary->wait_for_replay_catchup($node_standby); > > > > before calling the slot creation. > > > > It's done in the attached, would it be possible to give it a try please? > > Actually, let's also wait for the cascading standby to catchup too, like done > in the attached. Pushed. Seems like a clear race in the test, so I didn't think it was worth waiting for testing it on hoverfly. I think we should lower the log level, but perhaps wait for a few more cycles in case there are random failures? I wonder if we should make the connections in poll_query_until to reduce verbosity - it's pretty annoying how much that can bloat the log. Perhaps also introduce some backoff? It's really annoying to have to trawl through all those logs when there's a problem. Greetings, Andres Freund
Re: [PATCH] Use role name "system_user" instead of "user" for unsafe_tests
Aleksander Alekseev writes:
>> I don't think we can protect against all possible user names. Wouldn't it be
>> better to run the tests under an OS user with a different name, like
>> "marmaduke"? ("user" is a truly terrible default user name).
> 100% agree. The point is not to protect against all possible user
> names but merely to reduce the likelihood of the problem.
It only reduces the likelihood if you assume that "system_user"
is less likely than "user" as a choice of OS user name to run
the tests under. That seems like a debatable assumption;
perhaps it's actually *more* likely.
Whether we need to have a test for this at all is perhaps a
more interesting argument.
regards, tom lane
Re: v12: ERROR: subplan "InitPlan 2 (returns $4)" was not initialized
Justin Pryzby writes:
> postgres=# SET force_parallel_mode =1; CREATE TABLE x (i int) PARTITION BY
> RANGE (i); CREATE TABLE x1 PARTITION OF x DEFAULT ;
> select * from pg_class,
> lateral (select pg_catalog.bit_and(1)
> from pg_class as sample_1
> where case when EXISTS (
> select 1 from x
> where EXISTS (
> select 1 from pg_catalog.pg_depend
> where (sample_1.reltuples is NULL)
> )) then 1 end
>is NULL)x
> where false;
Interesting. The proximate cause is that we end up with a subplan
that is marked parallel_safe, but it has an initplan that is not
parallel_safe. The parallel worker receives, and tries to initialize,
the parallel_safe subplan, and falls over because of its reference
to the unsafe subplan -- which was not transmitted to the worker.
Actually, because of the policy installed by commit ab77a5a45, the
mere fact of having an initplan should be enough to disqualify
the first subplan from being marked parallel-safe.
I dug around and found the culprit: setrefs.c's
clean_up_removed_plan_level() moves initplans down from a parent
to a child plan node, but it forgot the possibility that the
child plan node had been marked parallel_safe before that and
must not be anymore.
The v1 patch attached is enough to fix the immediate issue,
but there's another thing not to like, which is that we're also
discarding the costs associated with the initplans. That's
strictly cosmetic given that all the planning decisions are
already made, but it still seems potentially annoying if you're
trying to understand EXPLAIN output. So I'm inclined to instead
do something like v2 attached, which deals with that as well.
(On the other hand, we aren't bothering to fix up costs when
we move initplans around in materialize_finished_plan or
standard_planner ... so maybe that should be left for a patch
that fixes those things too.)
Another thing worth wondering about is whether we can't loosen
commit ab77a5a45's policy that having an initplan is enough
to make you parallel-unsafe. In the wake of later fixes,
notably 5e6d8d2bb, it seems like maybe we could allow that
as long as the initplans themselves are parallel-safe. That
wouldn't be material for back-patching though, so I'll worry
about it later.
Not sure what if anything to do about a test case. I'm not
excited about memorializing the specific case found by sqlsmith,
because it seems only very accidental that it exposes this
problem. I found that there are existing regression tests
that exercise the situation where clean_up_removed_plan_level
generates an incorrectly-marked plan, but there is accidentally
no bad effect. (The planner itself isn't going to be making
any further decisions with the bogus info; it's only
ExecSerializePlan that pays attention to the flag, and we'd only
notice in this specific cross-reference situation.) Also, any
change we make along the lines speculated about in the previous
para would be highly likely to break a test case, in the sense
that it'd no longer exercise the previously-failing scenario.
So on the whole I'm inclined not to bother with a new test case.
regards, tom lane
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index b5398e..042036b11b 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -1546,6 +1546,10 @@ clean_up_removed_plan_level(Plan *parent, Plan *child)
child->initPlan = list_concat(parent->initPlan,
child->initPlan);
+ /* If we moved down any initplans, child is no longer parallel-safe */
+ if (child->initPlan)
+ child->parallel_safe = false;
+
/*
* We also have to transfer the parent's column labeling info into the
* child, else columns sent to client will be improperly labeled if this
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index b5398e..9a7c8ea07b 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -1542,7 +1542,31 @@ trivial_subqueryscan(SubqueryScan *plan)
static Plan *
clean_up_removed_plan_level(Plan *parent, Plan *child)
{
- /* We have to be sure we don't lose any initplans */
+ ListCell *lc;
+
+ /*
+ * We have to be sure we don't lose any initplans, so move any that were
+ * attached to the parent plan to the child. If we do move any, the child
+ * is no longer parallel-safe. As a cosmetic matter, also add the
+ * initplans' run costs to the child's costs (compare
+ * SS_charge_for_initplans).
+ */
+ foreach(lc, parent->initPlan)
+ {
+ SubPlan*initsubplan = (SubPlan *) lfirst(lc);
+ Cost initplan_cost;
+
+ child->parallel_safe = false;
+ initplan_cost = initsubplan->startup_cost + initsubplan->per_call_cost;
+ child->startup_cost += initplan_cost;
+ child->total_cost += initplan_cost;
+ }
+
+ /*
+ * Attach plans this way so that parent's
Re: Assertion being hit during WAL replay
Hi, On 2023-04-11 14:48:44 -0400, Tom Lane wrote: > I have seen this failure a couple of times recently while > testing code that caused crashes and restarts: Do you have a quick repro recipe? > #2 0x009987e3 in ExceptionalCondition ( > conditionName=conditionName@entry=0xb31bc8 "mode == RBM_NORMAL || mode == > RBM_ZERO_ON_ERROR || mode == RBM_ZERO_AND_LOCK", > fileName=fileName@entry=0xb31c15 "bufmgr.c", > lineNumber=lineNumber@entry=892) at assert.c:66 > #3 0x00842d73 in ExtendBufferedRelTo (eb=..., > fork=fork@entry=MAIN_FORKNUM, strategy=strategy@entry=0x0, > flags=flags@entry=3, extend_to=extend_to@entry=1, > mode=mode@entry=RBM_ZERO_AND_CLEANUP_LOCK) at bufmgr.c:891 > #4 0x005cc398 in XLogReadBufferExtended (rlocator=..., > forknum=MAIN_FORKNUM, blkno=0, mode=mode@entry=RBM_ZERO_AND_CLEANUP_LOCK, > recent_buffer=) at xlogutils.c:527 > #5 0x005cc697 in XLogReadBufferForRedoExtended ( > record=record@entry=0x1183b98, block_id=block_id@entry=0 '\000', > mode=mode@entry=RBM_NORMAL, get_cleanup_lock=get_cleanup_lock@entry=true, > buf=buf@entry=0x7ffd98e3ea94) at xlogutils.c:391 > #6 0x0055df59 in heap_xlog_prune (record=0x1183b98) at heapam.c:8779 > #7 heap2_redo (record=0x1183b98) at heapam.c:10015 > #8 0x005ca430 in ApplyWalRecord (replayTLI=, > record=0x7f8f7afbcb60, xlogreader=) > at ../../../../src/include/access/xlog_internal.h:379 > > It's not clear to me whether this Assert is wrong, or > XLogReadBufferForRedoExtended shouldn't be using > RBM_ZERO_AND_CLEANUP_LOCK, or the Assert is correctly protecting an > unimplemented case in ExtendBufferedRelTo that we now need to implement. Hm. It's not implemented because I didn't quite see how it'd make sense to pass RBM_ZERO_AND_CLEANUP_LOCK when extending the relation, but given how relation extension is done "implicitly" during recovery, that's too narrow a view. It's trivial to add. I wonder if we should eventually redefine the RBM* things into a bitmask. > In any case, I'm pretty sure Andres broke it in 26158b852, because > I hadn't seen it before this weekend. Yea, that's clearly the fault of 26158b852. Greetings, Andres Freund
v15b1: FailedAssertion("segment_map->header->magic == (DSA_SEGMENT_HEADER_MAGIC ^ area->control->handle ^ index)", File: "dsa.c", ..)
Yes, $SUBJECT is correct.
On an old centos6 VM which I'd forgotten about and never removed from
monitoring, I noticed that a process had recently crashed...
Maybe this is an issue which was already fixed, but I looked and find no
bug report nor patch about it. Feel free to dismiss the problem report
if it's not interesting or useful.
postgres was compiled locally at 4e54d231a. It'd been running
continuously since September without crashing until a couple weeks ago
(and running nearly-continuously for months before that).
The VM is essentially idle, so maybe that's related to the crash.
TRAP: FailedAssertion("segment_map->header->magic == (DSA_SEGMENT_HEADER_MAGIC
^ area->control->handle ^ index)", File: "dsa.c", Line: 1770, PID: 24257)
postgres: telsasoft old_ts ::1(50284)
authentication(ExceptionalCondition+0x91)[0x991451]
postgres: telsasoft old_ts ::1(50284) authentication[0x9b9f97]
postgres: telsasoft old_ts ::1(50284)
authentication(dsa_get_address+0x92)[0x9ba192]
postgres: telsasoft old_ts ::1(50284)
authentication(pgstat_get_entry_ref+0x442)[0x868892]
postgres: telsasoft old_ts ::1(50284)
authentication(pgstat_prep_pending_entry+0x54)[0x862b14]
postgres: telsasoft old_ts ::1(50284)
authentication(pgstat_assoc_relation+0x54)[0x866764]
postgres: telsasoft old_ts ::1(50284) authentication(_bt_first+0xb1b)[0x51399b]
postgres: telsasoft old_ts ::1(50284) authentication(btgettuple+0xc2)[0x50e792]
postgres: telsasoft old_ts ::1(50284)
authentication(index_getnext_tid+0x51)[0x4ff271]
postgres: telsasoft old_ts ::1(50284)
authentication(index_getnext_slot+0x72)[0x4ff442]
postgres: telsasoft old_ts ::1(50284)
authentication(systable_getnext+0x132)[0x4fe282]
postgres: telsasoft old_ts ::1(50284) authentication[0x9775cb]
postgres: telsasoft old_ts ::1(50284)
authentication(SearchCatCache+0x20d)[0x978e6d]
postgres: telsasoft old_ts ::1(50284)
authentication(GetSysCacheOid+0x30)[0x98c7c0]
postgres: telsasoft old_ts ::1(50284)
authentication(get_role_oid+0x2d)[0x86b1ad]
postgres: telsasoft old_ts ::1(50284)
authentication(hba_getauthmethod+0x22)[0x6f5592]
postgres: telsasoft old_ts ::1(50284)
authentication(ClientAuthentication+0x39)[0x6f1f59]
postgres: telsasoft old_ts ::1(50284)
authentication(InitPostgres+0x8c6)[0x9a33d6]
postgres: telsasoft old_ts ::1(50284)
authentication(PostgresMain+0x109)[0x84bb79]
postgres: telsasoft old_ts ::1(50284)
authentication(PostmasterMain+0x1a6a)[0x7ac1aa]
postgres: telsasoft old_ts ::1(50284) authentication(main+0x461)[0x6fe5a1]
/lib64/libc.so.6(__libc_start_main+0x100)[0x36e041ed20]
(gdb) bt
#0 0x0036e04324f5 in raise () from /lib64/libc.so.6
#1 0x0036e0433cd5 in abort () from /lib64/libc.so.6
#2 0x00991470 in ExceptionalCondition (conditionName=, errorType=, fileName=,
lineNumber=1770) at assert.c:69
#3 0x009b9f97 in get_segment_by_index (area=0x22818c0, index=) at dsa.c:1769
#4 0x009ba192 in dsa_get_address (area=0x22818c0, dp=1099511703168) at
dsa.c:953
#5 0x00868892 in pgstat_get_entry_ref (kind=PGSTAT_KIND_RELATION,
dboid=, objoid=, create=true,
created_entry=0x0) at pgstat_shmem.c:508
#6 0x00862b14 in pgstat_prep_pending_entry (kind=PGSTAT_KIND_RELATION,
dboid=0, objoid=2676, created_entry=0x0) at pgstat.c:1067
#7 0x00866764 in pgstat_prep_relation_pending (rel=0x22beba8) at
pgstat_relation.c:855
#8 pgstat_assoc_relation (rel=0x22beba8) at pgstat_relation.c:138
#9 0x0051399b in _bt_first (scan=0x22eb5c8, dir=ForwardScanDirection)
at nbtsearch.c:882
#10 0x0050e792 in btgettuple (scan=0x22eb5c8, dir=ForwardScanDirection)
at nbtree.c:243
#11 0x004ff271 in index_getnext_tid (scan=0x22eb5c8, direction=) at indexam.c:533
#12 0x004ff442 in index_getnext_slot (scan=0x22eb5c8,
direction=ForwardScanDirection, slot=0x22eb418) at indexam.c:625
#13 0x004fe282 in systable_getnext (sysscan=0x22eb3c0) at genam.c:511
#14 0x009775cb in SearchCatCacheMiss (cache=0x229e280, nkeys=, hashValue=3877703461, hashIndex=5, v1=,
v2=, v3=0, v4=0) at catcache.c:1364
#15 0x00978e6d in SearchCatCacheInternal (cache=0x229e280, v1=36056248,
v2=0, v3=0, v4=0) at catcache.c:1295
#16 SearchCatCache (cache=0x229e280, v1=36056248, v2=0, v3=0, v4=0) at
catcache.c:1149
#17 0x0098c7c0 in GetSysCacheOid (cacheId=10, oidcol=, key1=, key2=, key3=, key4=) at syscache.c:1293
#18 0x0086b1ad in get_role_oid (rolname=0x2262cb8 "telsasoft",
missing_ok=true) at acl.c:5181
#19 0x006f5592 in check_hba (port=) at hba.c:2100
#20 hba_getauthmethod (port=) at hba.c:2699
#21 0x006f1f59 in ClientAuthentication (port=0x224b5d0) at auth.c:396
#22 0x009a33d6 in PerformAuthentication (in_dbname=0x2268a48 "old_ts",
dboid=0, username=0x2262cb8 "telsasoft", useroid=0, out_dbname=0x0,
override_allow_connections=false) at postinit.c:245
#23 InitPostgres (in_dbname=0x2268a48 "old_ts", dboid=0, username=0x2262cb8
"telsasoft", useroid=0, out_
Re: Show various offset arrays for heap WAL records
On Tue, Apr 11, 2023 at 11:48 AM Peter Geoghegan wrote: > Attached revision deals with this by spelling out the names in full > (e.g., "old_xmax" and "new_xmax"). It also reorders the output fields > to match the order from the physical UPDATE, HOT_UPDATE, and LOCK WAL > record types, on the theory that those should match the physical > record (unless there is a good reason not to, which doesn't apply > here). I just noticed that we don't even show xmax in the case of DELETE records. Perhaps the original assumption is that it must match the record's own XID, but that's not true after the MultiXact enhancements for foreign key locking added to 9.3 (and in any case there is no reason at all to show xmax in UPDATE but not in DELETE). Attached revision v4 fixes this, making DELETE, UPDATE, HOT_UPDATE, LOCK, and LOCK_UPDATED record types consistent with each other in terms of the key names output by the heap desc routine. The field order also needed a couple of tweaks for struct consistency (and cross-record consistency) for v4. -- Peter Geoghegan v4-0001-Fix-heapdesc-infomask-array-output.patch Description: Binary data
Re: [PATCH] Use role name "system_user" instead of "user" for unsafe_tests
Hi Andrew,
> I don't think we can protect against all possible user names. Wouldn't it be
> better to run the tests under an OS user with a different name, like
> "marmaduke"? ("user" is a truly terrible default user name).
100% agree. The point is not to protect against all possible user
names but merely to reduce the likelihood of the problem. For this
particular test there is no difference which keyword to use for the
test. I realize this is a minor problem, however the fix is trivial
too.
--
Best regards,
Aleksander Alekseev
Re: [PATCH] Use role name "system_user" instead of "user" for unsafe_tests
On 2023-04-11 Tu 14:25, Aleksander Alekseev wrote:
Hi,
While playing with a new single board computer (VisionFive 2) I
discovered that postgresql:unsafe_tests suite fails like this:
```
---
/home/user/projects/postgresql/src/test/modules/unsafe_tests/expected/rolenames.out
2023-04-11 14:58:57.844550612 +
+++
/home/user/projects/postgresql/build/testrun/unsafe_tests/regress/results/rolenames.out
2023-04-11 17:54:22.999024391 +
@@ -53,6 +53,7 @@
CREATE ROLE "current_user";
CREATE ROLE "session_user";
CREATE ROLE "user";
+ERROR: role "user" already exists
RESET client_min_messages;
CREATE ROLE current_user; -- error
ERROR: CURRENT_USER cannot be used as a role name here
@@ -1089,4 +1090,5 @@
DROP OWNED BY regress_testrol0, "Public", "current_role",
"current_user", regress_testrol1, regress_testrol2, regress_testrolx
CASCADE;
DROP ROLE regress_testrol0, regress_testrol1, regress_testrol2,
regress_testrolx;
DROP ROLE "Public", "None", "current_role", "current_user",
"session_user", "user";
+ERROR: current user cannot be dropped
DROP ROLE regress_role_haspriv, regress_role_nopriv;
```
This happens because the developers of this SBC choose the default
username "user", which I had no reason to change.
Test merely checks that we can distinguish a username "user" from the
USER keyword. Maybe it's worth replacing "user" with "system_user"? It
is also a keyword but is a less likely choice for the OS user name.
I don't think we can protect against all possible user names. Wouldn't
it be better to run the tests under an OS user with a different name,
like "marmaduke"? ("user" is a truly terrible default user name).
cheers
andrew
--
Andrew Dunstan
EDB:https://www.enterprisedb.com
Re: refactoring relation extension and BufferAlloc(), faster COPY
Hi Andres,
07.04.2023 11:39, Andres Freund wrote:
Hi,
On 2023-04-06 18:15:14 -0700, Andres Freund wrote:
I think it might be worth having a C test for some of the bufmgr.c API. Things
like testing that retrying a failed relation extension works the second time
round.
A few hours after this I hit a stupid copy-pasto (21d7c05a5cf) that would
hopefully have been uncovered by such a test...
A few days later I've found a new defect introduced with 31966b151.
The following script:
echo "
CREATE TABLE tbl(id int);
INSERT INTO tbl(id) SELECT i FROM generate_series(1, 1000) i;
DELETE FROM tbl;
CHECKPOINT;
" | psql -q
sleep 2
grep -C2 'automatic vacuum of table ".*.tbl"' server.log
tf=$(psql -Aqt -c "SELECT format('%s/%s', pg_database.oid, relfilenode) FROM pg_database, pg_class WHERE datname =
current_database() AND relname = 'tbl'")
ls -l "$PGDB/base/$tf"
pg_ctl -D $PGDB stop -m immediate
pg_ctl -D $PGDB -l server.log start
with the autovacuum enabled as follows:
autovacuum = on
log_autovacuum_min_duration = 0
autovacuum_naptime = 1
gives:
2023-04-11 20:56:56.261 MSK [675708] LOG: checkpoint starting: immediate force
wait
2023-04-11 20:56:56.324 MSK [675708] LOG: checkpoint complete: wrote 900 buffers (5.5%); 0 WAL file(s) added, 0
removed, 0 recycled; write=0.016 s, sync=0.034 s, total=0.063 s; sync files=252, longest=0.017 s, average=0.001 s;
distance=4162 kB, estimate=4162 kB; lsn=0/1898588, redo lsn=0/1898550
2023-04-11 20:56:57.558 MSK [676060] LOG: automatic vacuum of table
"testdb.public.tbl": index scans: 0
pages: 5 removed, 0 remain, 5 scanned (100.00% of total)
tuples: 1000 removed, 0 remain, 0 are dead but not yet removable
-rw--- 1 law law 0 апр 11 20:56 .../tmpdb/base/16384/16385
waiting for server to shut down done
server stopped
waiting for server to start stopped waiting
pg_ctl: could not start server
Examine the log output.
server stops with the following stack trace:
Core was generated by `postgres: startup recovering 00010001
'.
Program terminated with signal SIGABRT, Aborted.
warning: Section `.reg-xstate/790626' in core file too small.
#0 __pthread_kill_implementation (no_tid=0, signo=6, threadid=140209454906240)
at ./nptl/pthread_kill.c:44
44 ./nptl/pthread_kill.c: No such file or directory.
(gdb) bt
#0 __pthread_kill_implementation (no_tid=0, signo=6, threadid=140209454906240)
at ./nptl/pthread_kill.c:44
#1 __pthread_kill_internal (signo=6, threadid=140209454906240) at
./nptl/pthread_kill.c:78
#2 __GI___pthread_kill (threadid=140209454906240, signo=signo@entry=6) at
./nptl/pthread_kill.c:89
#3 0x7f850ec53476 in __GI_raise (sig=sig@entry=6) at
../sysdeps/posix/raise.c:26
#4 0x7f850ec397f3 in __GI_abort () at ./stdlib/abort.c:79
#5 0x557950889c0b in ExceptionalCondition (
conditionName=0x557950a67680 "mode == RBM_NORMAL || mode == RBM_ZERO_AND_LOCK ||
mode == RBM_ZERO_ON_ERROR",
fileName=0x557950a673e8 "bufmgr.c", lineNumber=1008) at assert.c:66
#6 0x55795064f2d0 in ReadBuffer_common (smgr=0x557952739f38,
relpersistence=112 'p', forkNum=MAIN_FORKNUM,
blockNum=4294967295, mode=RBM_ZERO_AND_CLEANUP_LOCK, strategy=0x0,
hit=0x7fff22dd648f) at bufmgr.c:1008
#7 0x55795064ebe7 in ReadBufferWithoutRelcache (rlocator=...,
forkNum=MAIN_FORKNUM, blockNum=4294967295,
mode=RBM_ZERO_AND_CLEANUP_LOCK, strategy=0x0, permanent=true) at
bufmgr.c:800
#8 0x55795021c0fa in XLogReadBufferExtended (rlocator=...,
forknum=MAIN_FORKNUM, blkno=0,
mode=RBM_ZERO_AND_CLEANUP_LOCK, recent_buffer=0) at xlogutils.c:536
#9 0x55795021bd92 in XLogReadBufferForRedoExtended (record=0x5579526c4998,
block_id=0 '\000', mode=RBM_NORMAL,
get_cleanup_lock=true, buf=0x7fff22dd6598) at xlogutils.c:391
#10 0x5579501783b1 in heap_xlog_prune (record=0x5579526c4998) at
heapam.c:8726
#11 0x55795017b7db in heap2_redo (record=0x5579526c4998) at heapam.c:9960
#12 0x557950215b34 in ApplyWalRecord (xlogreader=0x5579526c4998,
record=0x7f85053d0120, replayTLI=0x7fff22dd6720)
at xlogrecovery.c:1915
#13 0x557950215611 in PerformWalRecovery () at xlogrecovery.c:1746
#14 0x557950201ce3 in StartupXLOG () at xlog.c:5433
#15 0x5579505cb6d2 in StartupProcessMain () at startup.c:267
#16 0x5579505be9f7 in AuxiliaryProcessMain (auxtype=StartupProcess) at
auxprocess.c:141
#17 0x5579505ca2b5 in StartChildProcess (type=StartupProcess) at
postmaster.c:5369
#18 0x5579505c5224 in PostmasterMain (argc=3, argv=0x5579526c3e70) at
postmaster.c:1455
#19 0x55795047a97d in main (argc=3, argv=0x5579526c3e70) at main.c:200
As I can see, autovacuum removes pages from the table file, and this causes
the crash while replaying the record:
rmgr: Heap2 len (rec/tot): 60/ 988, tx: 0, lsn: 0/01898600, prev 0/01898588, desc: PRUNE
snapshotConflictHorizon 732 nredirected 0 ndead 226, blkref #0: rel 1663/16384/16385 blk 0 FPW
Best regards,
Al
Re: fairywren exiting in ecpg
Hi, On 2023-04-11 07:10:20 -0400, Andrew Dunstan wrote: > The error hasn't been seen since I set this about a week ago. This issue really bothers me, but I am at my wits end how to debug it, given that we get a segfault only if we *disable* getting crash reports / core dumps in some form. There's no debug printout or anything, python just exits with an error code indicating an access violation. Greetings, Andres Freund
Re: When to drop src/tools/msvc support
Andres Freund writes: > Except that we're planning to remove it anyway, the structure of the docs here > seems a bit off... Indeed. We'll have to migrate some of that info elsewhere when the time comes. regards, tom lane
Assertion being hit during WAL replay
I have seen this failure a couple of times recently while testing code that caused crashes and restarts: #2 0x009987e3 in ExceptionalCondition ( conditionName=conditionName@entry=0xb31bc8 "mode == RBM_NORMAL || mode == RBM_ZERO_ON_ERROR || mode == RBM_ZERO_AND_LOCK", fileName=fileName@entry=0xb31c15 "bufmgr.c", lineNumber=lineNumber@entry=892) at assert.c:66 #3 0x00842d73 in ExtendBufferedRelTo (eb=..., fork=fork@entry=MAIN_FORKNUM, strategy=strategy@entry=0x0, flags=flags@entry=3, extend_to=extend_to@entry=1, mode=mode@entry=RBM_ZERO_AND_CLEANUP_LOCK) at bufmgr.c:891 #4 0x005cc398 in XLogReadBufferExtended (rlocator=..., forknum=MAIN_FORKNUM, blkno=0, mode=mode@entry=RBM_ZERO_AND_CLEANUP_LOCK, recent_buffer=) at xlogutils.c:527 #5 0x005cc697 in XLogReadBufferForRedoExtended ( record=record@entry=0x1183b98, block_id=block_id@entry=0 '\000', mode=mode@entry=RBM_NORMAL, get_cleanup_lock=get_cleanup_lock@entry=true, buf=buf@entry=0x7ffd98e3ea94) at xlogutils.c:391 #6 0x0055df59 in heap_xlog_prune (record=0x1183b98) at heapam.c:8779 #7 heap2_redo (record=0x1183b98) at heapam.c:10015 #8 0x005ca430 in ApplyWalRecord (replayTLI=, record=0x7f8f7afbcb60, xlogreader=) at ../../../../src/include/access/xlog_internal.h:379 It's not clear to me whether this Assert is wrong, or XLogReadBufferForRedoExtended shouldn't be using RBM_ZERO_AND_CLEANUP_LOCK, or the Assert is correctly protecting an unimplemented case in ExtendBufferedRelTo that we now need to implement. In any case, I'm pretty sure Andres broke it in 26158b852, because I hadn't seen it before this weekend. regards, tom lane
Re: Show various offset arrays for heap WAL records
On Tue, Apr 11, 2023 at 10:34 AM Peter Geoghegan wrote: > > description | off: 119, xmax: 1105, flags: 0x00, old_infobits: > > [], new off: 100, xmax 0 > > That doesn't seem great to me either. I don't like this ambiguity, > because it seems like it makes the description hard to parse in a way > that flies in the face of what we're trying to do here, in general. > So it seems like it might be worth fixing now, in the scope of this > patch. Attached revision deals with this by spelling out the names in full (e.g., "old_xmax" and "new_xmax"). It also reorders the output fields to match the order from the physical UPDATE, HOT_UPDATE, and LOCK WAL record types, on the theory that those should match the physical record (unless there is a good reason not to, which doesn't apply here). I also removed some inconsistencies between xl_heap_lock_updated and xl_heap_lock, since they're very similar record types. The revision also adds an extra sentence to the guidelines, since this seems like something that we're entitled to take a relatively firm position on. Finally, it also adds a comment about the rules for infobits_desc callers in header comments for the function, per your concern about that. -- Peter Geoghegan v3-0001-Fix-heapdesc-infomask-array-output.patch Description: Binary data
Re: [PATCH] Support % wildcard in extension upgrade filenames
On Mon, Apr 10, 2023 at 11:09:40PM -0400, Regina Obe wrote: > > On Mon, Apr 03, 2023 at 09:26:25AM +0700, Yurii Rashkovskii wrote: > > > I want to chime in on the issue of lower-number releases that are > > > released after higher-number releases. The way I see this particular > > > problem is that we always put upgrade SQL files in release "packages," > > > and they obviously become static resources. > > > > > > While I [intentionally] overlook some details here, what if (as a > > > convention, for projects where it matters) we shipped extensions with > > > non-upgrade SQL files only, and upgrades were available as separate > > > downloads? This way, we're not tying releases themselves to upgrade paths. > > > This also requires no changes to Postgres. > > > > This is actually something that's on the plate, and we recently added a -- > > disable-extension-upgrades-install configure switch and a > > `install-extension- > > upgrades-from-known-versions` make target in PostGIS to help going in that > > direction. I guess the ball would now be in the hands of packagers. > > > > > I know this may be a big delivery layout departure for > > > well-established projects; I also understand that this won't solve the > > > problem of having to have these files in the first place (though in > > > many cases, they can be automatically generated once, I suppose, if they > > > are > > trivial). > > > > We will now also be providing a `postgis` script for administration that > > among > > other things will support a `install-extension-upgrades` command to install > > upgrade paths from specific old versions, but will not hard-code any list of > > "known" old versions as such a list will easily become outdated. > > > > Note that all upgrade scripts installed by the Makefile target or by the > > `postgis` scripts will only be empty upgrade paths from the source version > > to > > the fake "ANY" version, as the ANY-- upgrade path will still be the > > "catch-all" upgrade script. > > Sounds like a user and packaging nightmare to me. > How is a packager to know which versions a user might have installed? Isn't this EXACTLY the same problem WE have ? The problem is we cannot know in advance how many patch-level releases will come out, and we don't want to hurry into cutting a new release in branches NOT having a bug which was fixed in a stable branch... Packager might actually know better in that they could ONLY consider the packages ever packaged by them. Hey, best would be having support for wildcard wouldn't it ? > I much preferred the idea of just listing all our upgrade targets in the > control file. Again: how would we know all upgrade targets ? > We need to come up with a convention of how to describe a micro update, > as it's really a problem with extensions that follow the pattern I think it's a problem with extensions maintaining stable branches, as if the history was linear we would possibly need less files (although at this stage any number bigger than 1 would be too much for me) --strk;
Issue in postgres_fdw causing unnecessary wait for cancel request reply
Hi,
When using postgres_fdw, in the event of a local transaction being
aborted while a query is running on a remote server,
postgres_fdw sends a cancel request to the remote server.
However, if PQgetCancel() returned NULL and no cancel request was issued,
I found that postgres_fdw could still wait for the reply to
the cancel request, causing unnecessary wait time with a 30 second timeout.
For example, the following queries can reproduce the issue:
create extension postgres_fdw;
create server loopback foreign data wrapper postgres_fdw options
(tcp_user_timeout 'a');
create user mapping for public server loopback;
create view t as select 1 as i from pg_sleep(100);
create foreign table ft (i int) server loopback options (table_name 't');
select * from ft;
Press Ctrl-C while running the above SELECT query.
Attached patch fixes this issue. It ensures that postgres_fdw only waits
for a reply if a cancel request is actually issued. Additionally,
it improves PQgetCancel() to set error messages in certain error cases,
such as when out of memory occurs and malloc() fails. Moreover,
it enhances postgres_fdw to report a warning message when PQgetCancel()
returns NULL, explaining the reason for the NULL value.
Thought?
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONFrom 26293ade92ce6fa0bbafac4a95f634e485f4aab6 Mon Sep 17 00:00:00 2001
From: Fujii Masao
Date: Wed, 12 Apr 2023 02:17:56 +0900
Subject: [PATCH v1] postgres_fdw: Fix bug causing unnecessary wait for cancel
request reply.
In the event of a local transaction being aborted while a query is
running on a remote server in postgres_fdw, the extension sends
a cancel request to the remote server. However, if PQgetCancel()
returned NULL and no cancel request was issued, postgres_fdw
could still wait for the reply to the cancel request, causing unnecessary
wait time with a 30 second timeout.
To fix this issue, this commit ensures that postgres_fdw only waits for
a reply if a cancel request was actually issued. Additionally,
this commit improves PQgetCancel() to set error messages in certain
error cases, such as when out of memory happens and malloc() fails.
Moreover, this commit enhances postgres_fdw to report a warning
message when PQgetCancel() returns NULL, explaining the reason for
the NULL value.
---
contrib/postgres_fdw/connection.c | 8
src/interfaces/libpq/fe-connect.c | 6 ++
2 files changed, 14 insertions(+)
diff --git a/contrib/postgres_fdw/connection.c
b/contrib/postgres_fdw/connection.c
index 2969351e9a..e3c79158d8 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -1344,6 +1344,14 @@ pgfdw_cancel_query_begin(PGconn *conn)
}
PQfreeCancel(cancel);
}
+ else
+ {
+ ereport(WARNING,
+ (errcode(ERRCODE_CONNECTION_FAILURE),
+errmsg("could not send cancel request: %s",
+ pchomp(PQerrorMessage(conn);
+ return false;
+ }
return true;
}
diff --git a/src/interfaces/libpq/fe-connect.c
b/src/interfaces/libpq/fe-connect.c
index 40fef0e2c8..eeb4d9a745 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -4703,11 +4703,17 @@ PQgetCancel(PGconn *conn)
return NULL;
if (conn->sock == PGINVALID_SOCKET)
+ {
+ libpq_append_conn_error(conn, "connection is not open");
return NULL;
+ }
cancel = malloc(sizeof(PGcancel));
if (cancel == NULL)
+ {
+ libpq_append_conn_error(conn, "out of memory");
return NULL;
+ }
memcpy(&cancel->raddr, &conn->raddr, sizeof(SockAddr));
cancel->be_pid = conn->be_pid;
--
2.40.0
[PATCH] Use role name "system_user" instead of "user" for unsafe_tests
Hi, While playing with a new single board computer (VisionFive 2) I discovered that postgresql:unsafe_tests suite fails like this: ``` --- /home/user/projects/postgresql/src/test/modules/unsafe_tests/expected/rolenames.out 2023-04-11 14:58:57.844550612 + +++ /home/user/projects/postgresql/build/testrun/unsafe_tests/regress/results/rolenames.out 2023-04-11 17:54:22.999024391 + @@ -53,6 +53,7 @@ CREATE ROLE "current_user"; CREATE ROLE "session_user"; CREATE ROLE "user"; +ERROR: role "user" already exists RESET client_min_messages; CREATE ROLE current_user; -- error ERROR: CURRENT_USER cannot be used as a role name here @@ -1089,4 +1090,5 @@ DROP OWNED BY regress_testrol0, "Public", "current_role", "current_user", regress_testrol1, regress_testrol2, regress_testrolx CASCADE; DROP ROLE regress_testrol0, regress_testrol1, regress_testrol2, regress_testrolx; DROP ROLE "Public", "None", "current_role", "current_user", "session_user", "user"; +ERROR: current user cannot be dropped DROP ROLE regress_role_haspriv, regress_role_nopriv; ``` This happens because the developers of this SBC choose the default username "user", which I had no reason to change. Test merely checks that we can distinguish a username "user" from the USER keyword. Maybe it's worth replacing "user" with "system_user"? It is also a keyword but is a less likely choice for the OS user name. -- Best regards, Aleksander Alekseev v1-0001-Use-role-name-system_user-instead-of-user-for-uns.patch Description: Binary data
Re: Should we remove vacuum_defer_cleanup_age?
Hi,
On 2023-04-11 11:33:01 -0500, Justin Pryzby wrote:
> On Wed, Mar 22, 2023 at 10:00:48AM -0700, Andres Freund wrote:
> > I don't know whether others think we should apply it this release, given the
> > "late submission", but I tend to think it's not worth caring the
> > complication
> > of vacuum_defer_cleanup_age forward.
>
> I don't see any utility in waiting; it just makes the process of
> removing it take longer for no reason.
>
> As long as it's done before the betas, it seems completely reasonable to
> remove it for v16.
Added the RMT.
We really should have a rmt@pg.o alias...
Updated patch attached. I think we should either apply something like that
patch, or at least add a to the docs.
Greetings,
Andres Freund
>From c51c9e450b1b1342c0f6ff32e7e360677ccbc2c6 Mon Sep 17 00:00:00 2001
From: Andres Freund
Date: Tue, 11 Apr 2023 10:21:36 -0700
Subject: [PATCH v2] Remove vacuum_defer_cleanup_age
This commit removes TransactionIdRetreatSafely(), as there are no users
anymore. There might be potential future users, hence noting that here.
Reviewed-by: Daniel Gustafsson
Reviewed-by: Justin Pryzby
Discussion: https://postgr.es/m/20230317230930.nhsgk3qfk7f4a...@awork3.anarazel.de
---
src/include/storage/standby.h | 1 -
src/backend/storage/ipc/procarray.c | 105 ++
src/backend/storage/ipc/standby.c | 1 -
src/backend/utils/misc/guc_tables.c | 9 --
src/backend/utils/misc/postgresql.conf.sample | 1 -
src/bin/pg_upgrade/server.c | 5 +-
doc/src/sgml/config.sgml | 35 --
doc/src/sgml/high-availability.sgml | 28 +
8 files changed, 15 insertions(+), 170 deletions(-)
diff --git a/src/include/storage/standby.h b/src/include/storage/standby.h
index 41f4dc372e6..e8f50569491 100644
--- a/src/include/storage/standby.h
+++ b/src/include/storage/standby.h
@@ -21,7 +21,6 @@
#include "storage/standbydefs.h"
/* User-settable GUC parameters */
-extern PGDLLIMPORT int vacuum_defer_cleanup_age;
extern PGDLLIMPORT int max_standby_archive_delay;
extern PGDLLIMPORT int max_standby_streaming_delay;
extern PGDLLIMPORT bool log_recovery_conflict_waits;
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index ea91ce355f2..106b184a3e6 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -367,9 +367,6 @@ static inline void ProcArrayEndTransactionInternal(PGPROC *proc, TransactionId l
static void ProcArrayGroupClearXid(PGPROC *proc, TransactionId latestXid);
static void MaintainLatestCompletedXid(TransactionId latestXid);
static void MaintainLatestCompletedXidRecovery(TransactionId latestXid);
-static void TransactionIdRetreatSafely(TransactionId *xid,
- int retreat_by,
- FullTransactionId rel);
static inline FullTransactionId FullXidRelativeTo(FullTransactionId rel,
TransactionId xid);
@@ -1709,10 +1706,7 @@ TransactionIdIsActive(TransactionId xid)
* do about that --- data is only protected if the walsender runs continuously
* while queries are executed on the standby. (The Hot Standby code deals
* with such cases by failing standby queries that needed to access
- * already-removed data, so there's no integrity bug.) The computed values
- * are also adjusted with vacuum_defer_cleanup_age, so increasing that setting
- * on the fly is another easy way to make horizons move backwards, with no
- * consequences for data integrity.
+ * already-removed data, so there's no integrity bug.)
*
* Note: the approximate horizons (see definition of GlobalVisState) are
* updated by the computations done here. That's currently required for
@@ -1877,50 +1871,11 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
TransactionIdOlder(h->data_oldest_nonremovable, kaxmin);
/* temp relations cannot be accessed in recovery */
}
- else
- {
- /*
- * Compute the cutoff XID by subtracting vacuum_defer_cleanup_age.
- *
- * vacuum_defer_cleanup_age provides some additional "slop" for the
- * benefit of hot standby queries on standby servers. This is quick
- * and dirty, and perhaps not all that useful unless the primary has a
- * predictable transaction rate, but it offers some protection when
- * there's no walsender connection. Note that we are assuming
- * vacuum_defer_cleanup_age isn't large enough to cause wraparound ---
- * so guc.c should limit it to no more than the xidStopLimit threshold
- * in varsup.c. Also note that we intentionally don't apply
- * vacuum_defer_cleanup_age on standby servers.
- *
- * Need to use TransactionIdRetreatSafely() instead of open-coding the
- * subtraction, to prevent creating an xid before
- * FirstNormalTransactionId.
- */
- Assert(TransactionIdPrecedesOrEquals(h->oldest_considered_running,
- h->shared_oldest_nonremovable));
- Assert(TransactionIdPrecedesOrEquals(h->shared_oldest_non
Re: When to drop src/tools/msvc support
Hi, On 2023-04-11 13:30:15 -0400, Tom Lane wrote: > Andres Freund writes: > > Here's a draft docs change. > > > I added the in two places in install-windows.sgml so it's visible > > on both the generated pages in the chunked output. That does mean it's > > visible > > twice nearby in the single-page output, but I don't think that's commonly > > used. > > I don't agree with placing that first hunk before the para that tells > people to use a binary distribution, as it's completely irrelevant > if they take that advice. Fair point. > I'm not really sure we need it at all, because quite a bit of the text right > after that is not specific to using the src/tools/msvc scripts either. I > think the under "Building with Visual > C++ ..." is sufficient. It seemed nicer to have it on all the "Installation from Source Code on Windows" pages, but... Except that we're planning to remove it anyway, the structure of the docs here seems a bit off... Greetings, Andres Freund
Re: Direct I/O
Hi, On 2023-04-09 16:40:54 -0700, Noah Misch wrote: > On Sun, Apr 09, 2023 at 02:45:16PM -0700, Andres Freund wrote: > > It's not *just* that scenario. With a few concurrent connections you can get > > into problematic territory even with halfway reasonable shared buffers. > > I am not familiar with such cases. You could get there with 64MB shared > buffers and 256 simultaneous commits of new-refilenode-creating transactions, > but I'd still file that under going out of one's way to use tiny shared > buffers relative to the write activity. What combination did you envision? I'd not say it's common, but it's less crazy than running with 128kB of s_b... There's also the issue that log_newpage_range() is used in number of places where we could have a lot of pre-existing buffer pins. So pinning another 64 buffers could tip us over. Greetings, Andres Freund
Re: Fix the miss consideration of tuple_fraction during add_paths_to_append_rel
On Mon, Apr 10, 2023 at 9:56 PM Zhang Mingli wrote: > > There is spare indent at else if. > > - if (childrel->pathlist != NIL && > + if (cheapest_startup_path && cheapest_startup_path->param_info == NULL) > + accumulate_append_subpath(cheapest_startup_path, > + &subpaths, NULL); > + else if (childrel->pathlist != NIL && > childrel->cheapest_total_path->param_info == NULL) > accumulate_append_subpath(childrel->cheapest_total_path, > &subpaths, NULL); > > Could we also consider tuple_fraction in partial_pathlist for parallel > append? > > Thanks for the suggestion, the v2 has fixed the indent issue and I did something about parallel append. Besides that, I restrict the changes happens under bms_equal(rel->relids, root->all_query_rels), which may make this patch safer. I have added this into https://commitfest.postgresql.org/43/4270/ -- Best Regards Andy Fan v2-0001-Considering-root-tuple_fraction-during-create_app.patch Description: Binary data
Re: Show various offset arrays for heap WAL records
On Tue, Apr 11, 2023 at 7:40 AM Melanie Plageman
wrote:
> static void
> infobits_desc(StringInfo buf, uint8 infobits, const char *keyname)
> {
> appendStringInfo(buf, "%s: [", keyname);
>
> Why can we assume that there will be no space at the end here?
I don't think that anybody is going to try that, but if they do then
the assertion will fail reliably.
> I know we need to be able to avoid doing the comma overwriting if no
> flags were set. In general, we expect record description elements to
> prepend themselves with commas and spaces, but these infobits, for
> example, use a trailing comma and space. If someone adds a description
> element with a trailing space, they will trip this assert. We should at
> least add a comment explaining this assertion so someone knows what to
> do if they trip it.
The invariant here is roughly: caller's keyname argument cannot have
trailing spaces or punctuation characters. It looks like it would be
inconvenient to write a precise assertion for that, but it doesn't
feel particularly necessary, given that this is just a static helper
function.
> Otherwise, we can return early if no flags are set. That will probably
> make for slightly messier code since we would still have to construct
> the empty list.
I prefer to keep this as simple as possible for now.
> Also you didn't add the same assert to truncate_flags_desc().
That's because truncate_flags_desc doesn't have a "keyname" argument.
Though it does have an assertion at the end that is almost equivalent:
the "Assert(buf->data[buf->len - 2] == ',') assertion (a matching
assertion appears at the end of infobits_desc).
> Not the fault of this patch, but I also noticed that heap UPDATE and
> HOT_UPDATE records have xmax twice and don't differentiate between new
> and old. I think that was probably a mistake.
>
> description | off: 119, xmax: 1105, flags: 0x00, old_infobits:
> [], new off: 100, xmax 0
That doesn't seem great to me either. I don't like this ambiguity,
because it seems like it makes the description hard to parse in a way
that flies in the face of what we're trying to do here, in general.
So it seems like it might be worth fixing now, in the scope of this
patch.
> Also not the fault of this patch, but looking at the output while using
> this, I realized truncate record type has a stringified version of its
> flags while other record types, like update, don't. Do you think this
> makes sense? Perhaps not something we can change now, though...
You don't have to look at the truncate record type (which is a
relatively obscure and unimportant record type) to see these kinds of
inconsistencies. You can see the same thing with HEAP_UPDATE and
HEAP_HOT_UPDATE, which have stringified constants for infomask bits,
but not for the xl_heap_update.flags status bits.
I don't see any principled reason why such an inconsistency should
exist -- and we're talking about a pretty glaring inconsistency here.
On the other hand I don't think that we're obligated to do anything
about it for 16.
> Also not the fault of this patch, but I noticed that leaftopparent is
> often InvalidBlockNumber--which shows up as 4294967295. I wonder if
> anyone would be confused by this. Maybe devs know that this value is
> InvalidBlockNumber. In the future, perhaps we should interpolate the
> string "InvalidBlockNumber"?
>
> description | left: 436, right: 389, level: 0, safexid: 0:1091,
> leafleft: 436, leafright: 389, leaftopparent: 4294967295
In my personal opinion (this is a totally subjective question), the
current approach here is okay because (on its own) "leaftopparent:
4294967295" isn't any more or less meaningful than "leaftopparent:
InvalidBlockNumber". It's not as if the REDO routine actually relies
on the value ever being InvalidBlockNumber at all (except in an
assertion).
Plus it's easier to parse as-is. That's what swings it for me, in fact
(as with the "two xmax fields in update records" question).
This is the kind of question that tends to lead to bikeshedding. The
guidelines should avoid taking a firm position on these more
subjective questions, where we must make a subjective trade-off.
Especially a trade-off around how faithfully we represent the physical
WAL record versus readability (whatever "readability" means). I
pondered a similar trade-off in comments added to delvacuum_desc. That
contributed to my feeling that this is best left up to individual rmgr
desc routines.
--
Peter Geoghegan
Re: When to drop src/tools/msvc support
Andres Freund writes: > Here's a draft docs change. > I added the in two places in install-windows.sgml so it's visible > on both the generated pages in the chunked output. That does mean it's visible > twice nearby in the single-page output, but I don't think that's commonly > used. I don't agree with placing that first hunk before the para that tells people to use a binary distribution, as it's completely irrelevant if they take that advice. I'm not really sure we need it at all, because quite a bit of the text right after that is not specific to using the src/tools/msvc scripts either. I think the under "Building with Visual C++ ..." is sufficient. The other two changes look fine. regards, tom lane
Re: Documentation for building with meson
Hi, On 2023-03-28 12:27:26 -0700, samay sharma wrote: > Subject: [PATCH v9 1/5] Make minor additions and corrections to meson docs > > This commit makes a few corrections to the meson docs > and adds a few instructions and links for better clarity. > --- > doc/src/sgml/installation.sgml | 24 +++- > 1 file changed, 15 insertions(+), 9 deletions(-) > > diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml > index 70ab5b77d8..e3b9b6c0d0 100644 > --- a/doc/src/sgml/installation.sgml > +++ b/doc/src/sgml/installation.sgml > @@ -2057,8 +2057,7 @@ meson setup build -Dssl=openssl > > meson configure -Dcassert=true > > -meson configure's commonly used command-line options > -are explained in . > +Commonly used build options for meson configure (and > meson setup) are explained in linkend="meson-options"/>. > > > > @@ -2078,6 +2077,13 @@ ninja > processes used with the command line argument -j. > > > + > +If you want to build the docs, you can type: > + > +ninja docs > + > + "type" sounds a bit too, IDK, process oriented. "To build the docs, use"? > Subject: [PATCH v9 2/5] Add data layout options sub-section in installation > docs > > This commit separates out blocksize, segsize and wal_blocksize > options into a separate Data layout options sub-section in both > the make and meson docs. They were earlier in a miscellaneous > section which included several unrelated options. This change > also helps reduce the repetition of the warnings that changing > these parameters breaks on-disk compatibility. Makes sense. I'm planning to apply this unless Peter or somebody else has further feedback. > From 11d82aa49efb3d1cbc08f14562a757f115053c8b Mon Sep 17 00:00:00 2001 > From: Samay Sharma > Date: Mon, 13 Feb 2023 16:23:52 -0800 > Subject: [PATCH v9 3/5] Remove Anti-Features section from Installation from > source docs > > Currently, several meson setup options are listed in anti-features. > However, they are similar to most other options in the postgres > features list as they are 'auto' features themselves. Also, other > options are likely better suited to the developer options section. > This commit, therefore, moves the options listed in the anti-features > section into other sections and removes that section. > > For consistency, this reorganization has been done on the make section > of the docs as well. Makes sense to me. "Anti-Features" is confusing as a name to start with. Greetings, Andres Freund
Re: Memory leak from ExecutorState context?
On Sat, 8 Apr 2023 02:01:19 +0200 Jehan-Guillaume de Rorthais wrote: > On Fri, 31 Mar 2023 14:06:11 +0200 > Jehan-Guillaume de Rorthais wrote: > > [...] > > After rebasing Tomas' memory balancing patch, I did some memory measures > to answer some of my questions. Please, find in attachment the resulting > charts "HJ-HEAD.png" and "balancing-v3.png" to compare memory consumption > between HEAD and Tomas' patch. They shows an alternance of numbers > before/after calling ExecHashIncreaseNumBatches (see the debug patch). I > didn't try to find the exact last total peak of memory consumption during the > join phase and before all the BufFiles are destroyed. So the last number > might be underestimated. I did some more analysis about the total memory consumption in filecxt of HEAD, v3 and v4 patches. My previous debug numbers only prints memory metrics during batch increments or hash table destruction. That means: * for HEAD: we miss the batches consumed during the outer scan * for v3: adds twice nbatch in spaceUsed, which is a rough estimation * for v4: batches are tracked in spaceUsed, so they are reflected in spacePeak Using a breakpoint in ExecHashJoinSaveTuple to print "filecxt->mem_allocated" from there, here are the maximum allocated memory for bufFile context for each branch: batches max bufFiles total spaceAllowed rise HEAD16384 199966960 ~194MB v3 4096 65419456 ~78MB v4(*3) 2048 3427328048MB nbatch*sizeof(PGAlignedBlock)*3 v4(*4) 1024 17170160 60.6MB nbatch*sizeof(PGAlignedBlock)*4 v4(*5) 2048 34273280 42.5MB nbatch*sizeof(PGAlignedBlock)*5 It seems account for bufFile in spaceUsed allows a better memory balancing and management. The precise factor to rise spaceAllowed is yet to be defined. *3 or *4 looks good, but this is based on a single artificial test case. Also, note that HEAD is currently reporting ~4MB of memory usage. This is by far wrong with the reality. So even if we don't commit the balancing memory patch in v16, maybe we could account for filecxt in spaceUsed as a bugfix? Regards,
Re: When to drop src/tools/msvc support
Hi, On 2023-04-11 10:44:09 -0400, Jonathan S. Katz wrote: > On 4/11/23 10:12 AM, Tom Lane wrote: > > "Jonathan S. Katz" writes: > > > On 4/11/23 9:49 AM, Tom Lane wrote: > > > > Sadly, I think we really have to ship both build systems in v16. > > > > > But maybe we can make it clear in the release notes + docs that this is > > > slated for deprecation and will be removed from v17? That way we can say > > > "we provided ample warning to move to the new build system." > > > > Yes, we absolutely should do that, as already discussed upthread. > > Ah yes, I saw Andres' notep[1] yesterday and had already forgotten. +1 on > that plan. Here's a draft docs change. I added the in two places in install-windows.sgml so it's visible on both the generated pages in the chunked output. That does mean it's visible twice nearby in the single-page output, but I don't think that's commonly used. Greetings, Andres Freund >From 11b7229f499917fb1902ce226a31c5e2e7463390 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Tue, 11 Apr 2023 10:05:18 -0700 Subject: [PATCH v1] docs: Building with src/tools/msvc is deprecated I added the in two places in install-windows.sgml so it's visible on both the generated pages in the chunked output. --- doc/src/sgml/install-windows.sgml | 16 doc/src/sgml/installation.sgml| 11 --- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/doc/src/sgml/install-windows.sgml b/doc/src/sgml/install-windows.sgml index 2db44db2fd9..27a5889d733 100644 --- a/doc/src/sgml/install-windows.sgml +++ b/doc/src/sgml/install-windows.sgml @@ -8,6 +8,14 @@ on Windows + + + Building with this infrastructure is deprecated. It + will be removed in PostgreSQL 17. Consider + building with meson instead. + + + It is recommended that most users download the binary distribution for Windows, available as a graphical installer package @@ -62,6 +70,14 @@ Building with Visual C++ or the Microsoft Windows SDK + + + Building with this infrastructure is deprecated. It + will be removed in PostgreSQL 17. Consider + building with meson instead. + + + PostgreSQL can be built using the Visual C++ compiler suite from Microsoft. These compilers can be either from Visual Studio, diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml index f451204854c..b9d3fdeafdb 100644 --- a/doc/src/sgml/installation.sgml +++ b/doc/src/sgml/installation.sgml @@ -26,9 +26,14 @@ documentation. See standalone-profile.xsl for details. If you are building PostgreSQL for Microsoft - Windows, read this chapter if you intend to build with MinGW or Cygwin; - but if you intend to build with Microsoft's Visual - C++, see instead. + Windows, read if you intend to build + with make and autoconf (using MinGW or Cygwin build environments); read + if you intend to build with + meson (using Microsoft Visual + C++, MinGW or Cygwin build environments); if you intend to + build with Microsoft's Visual C++ using the + deprecated perl scripts, see instead. -- 2.38.0
Re: Should we remove vacuum_defer_cleanup_age?
On Wed, Mar 22, 2023 at 10:00:48AM -0700, Andres Freund wrote: > I don't know whether others think we should apply it this release, given the > "late submission", but I tend to think it's not worth caring the complication > of vacuum_defer_cleanup_age forward. I don't see any utility in waiting; it just makes the process of removing it take longer for no reason. As long as it's done before the betas, it seems completely reasonable to remove it for v16. -- Justin
Re: When to drop src/tools/msvc support
Hi, On 2023-04-11 09:05:31 +0100, Dave Page wrote: > Probably my main concern is that the Meson build can use the same version > of the VC++ compiler that we use (v14), which is carefully matched for > compatibility with all the various components, just in case anything passes > CRT pointers around. FWIW, Independent of meson, I don't think support for VC 2015 in postgres is long for the world. Later versions of msvc have increased the C standard compliance a fair bit... It's also a few years out of normal support. I've not tested building with 2015, but I don't know of anything that should prevent building with meson with it. I am fairly sure that it can't build tab-complete.c, but you're presumably not building with tab completion support anyway? Greetings, Andres Freund
[BUG #17888] Incorrect memory access in gist__int_ops for an input array with many elements
Hi,
I was looking at this issue:
https://www.postgresql.org/message-id/flat/17888-f72930e6b5ce8c14%40postgresql.org
pfree call on contrib/intarray/_int_gist.c:345
```
if (in != (ArrayType *) DatumGetPointer(entry->key))
pfree(in);
```
leads to BogusFree function call and server crash.
This is when we use array size of 1001.
If we increase array size to 3001, we get index size error,
NOTICE: input array is too big (199 maximum allowed, 3001 current), use
gist__intbig_ops opclass instead
ERROR: index row requires 12040 bytes, maximum size is 8191
When array size is 1001 is concerned, we raise elog about input array is
too big, multiple of times. Wouldn't it make more sense to error out
instead of proceeding further and getting bogus pointer errors messages
(as we do when size is 3001)?
Changing elog to ereport makes behavior consistent (between array size
of 1001 vs 3001), so I have
attached a patch for that.
It errors out like this:
ERROR: input array is too big (199 maximum allowed, 1001 current), use
gist__intbig_ops opclass instead
This is same error which was raised as notice earlier.
Let me know if it makes sense.
Also, comments on BogusFree mentions `As a possible
aid in debugging, we report the header word along with the pointer
address`. How can we interpret useful debugging information from this?
`pfree called with invalid pointer 0x7ff1706d0030 (header
0x4fc80001)`
Regards,
Ankit
diff --git a/contrib/intarray/_int_gist.c b/contrib/intarray/_int_gist.c
index 7a48ce624d..79c81127f6 100644
--- a/contrib/intarray/_int_gist.c
+++ b/contrib/intarray/_int_gist.c
@@ -181,8 +181,10 @@ g_int_compress(PG_FUNCTION_ARGS)
PREPAREARR(r);
if (ARRNELEMS(r) >= 2 * num_ranges)
- elog(NOTICE, "input array is too big (%d maximum allowed, %d current), use gist__intbig_ops opclass instead",
- 2 * num_ranges - 1, ARRNELEMS(r));
+ ereport(ERROR,
+(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("input array is too big (%d maximum allowed, %d current), use gist__intbig_ops opclass instead",
+ 2 * num_ranges - 1, ARRNELEMS(r;
retval = palloc(sizeof(GISTENTRY));
gistentryinit(*retval, PointerGetDatum(r),
Re: ERROR messages in VACUUM's PARALLEL option
On Tue, Apr 11, 2023 at 4:00 AM David Rowley wrote:
>
> Over in [1], Horiguchisan mentioned a few things about VACUUM's new
> BUFFER_USAGE_LIMIT option.
>
> 1) buffer_usage_limit in the ERROR messages should be consistently in
> uppercase.
I did notice that all the other VACUUM options don't do this:
postgres=# vacuum (process_toast 'boom') foo;
ERROR: process_toast requires a Boolean value
postgres=# vacuum (full 2) foo;
ERROR: full requires a Boolean value
postgres=# vacuum (verbose 2) foo;
ERROR: verbose requires a Boolean value
Though, I actually prefer uppercase. They are all documented in
uppercase, so I don't see why they would all be lowercase in the error
messages. Additionally, for BUFFER_USAGE_LIMIT, I find the uppercase
helpful to differentiate it from the GUC vacuum_buffer_usage_limit.
> 2) defGetString() already checks for opt->args == NULL and raises an
> ERROR when it is.
>
> I suspect that Melanie might have followed the lead of the PARALLEL
> option when she was working on adding the BUFFER_USAGE_LIMIT patch.
Yes, I mostly imitated parallel since it was the other VACUUM option
with a valid range (as opposed to a boolean or enum of acceptable
values).
> What I'm wondering now is:
>
> a) Is it worth changing this for the PARALLEL option too? and;
> b) I see we lose the parse position indicator, and;
I'm not too worried about the parse position indicator, as we don't get
it for the majority of the errors about valid values. And, as you say
later in your email, the VACUUM options are pretty simple so it should
be easy for the user to figure out without a parse position indicator.
postgres=# vacuum (SKIP_LOCKED 2) foo;
ERROR: skip_locked requires a Boolean value
While trying different combinations, I noticed that defGetInt32 produces
a somewhat unsatisfactory error message when I provide an argument which
would overflow an int32
postgres=# vacuum (parallel 33) foo;
ERROR: parallel requires an integer value
In defGetInt32(), the def->arg nodeTag is T_Float, which is why we get
this error message. Perhaps it is worth checking somewhere in the stack
for integer overflow (instead of assuming it is a float) and giving a
more helpful message like the one from parse_int()?
if (val > INT_MAX || val < INT_MIN)
{
if (hintmsg)
*hintmsg = gettext_noop("Value exceeds integer range.");
return false;
}
Probably not a trivial task and not one for 16, however.
I will say that I prefer the new error message introduced by this
patch's usage of defGetInt32() when an argument is not specified to the
previous error message.
postgres=# vacuum (parallel) foo;
ERROR: parallel requires an integer value
On Tue, Apr 11, 2023 at 9:58 AM Tom Lane wrote:
>
> David Rowley writes:
> > Over in [1], Horiguchisan mentioned a few things about VACUUM's new
> > BUFFER_USAGE_LIMIT option.
>
> > 1) buffer_usage_limit in the ERROR messages should be consistently in
> > uppercase.
>
> FWIW, I think this is exactly backward, and so is whatever code you
> based this on. Our usual habit is to write GUC names and suchlike
> in lower case in error messages. Add double quotes if you feel you
> want to set them off from the surrounding text. Here's a typical
> example of longstanding style:
>
> regression=# set max_parallel_workers_per_gather to -1;
> ERROR: -1 is outside the valid range for parameter
> "max_parallel_workers_per_gather" (0 .. 1024)
It seems it also is true for VACUUM options currently, but you didn't
mention command options. Do you also feel these should be lowercase?
- Melanie
Re: running logical replication as the subscription owner
On Mon, Apr 10, 2023 at 10:09 PM Shinoda, Noriyoshi (PN Japan FSIP) wrote: > Hi hackers, > Thank you for developing a great feature. > The following commit added a column to the pg_subscription catalog. > > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=482675987bcdffb390ae735cfd5f34b485ae97c6 > > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=c3afe8cf5a1e465bd71e48e4bc717f5bfdc7a7d6 > > I found that the documentation of the pg_subscription catalog was missing an > explanation of the columns subrunasowner and subpasswordrequired, so I > attached a patch. Please fix the patch if you have a better explanation. Thank you. Committed. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Should vacuum process config file reload more often
On Fri, Apr 7, 2023 at 10:23 PM Daniel Gustafsson wrote: > > > On 7 Apr 2023, at 15:07, Melanie Plageman wrote: > > On Fri, Apr 7, 2023 at 2:53 AM Masahiko Sawada > > wrote: > > >> + /* Only log updates to cost-related variables */ > >> + if (vacuum_cost_delay == original_cost_delay && > >> + vacuum_cost_limit == original_cost_limit) > >> + return; > >> > >> IIUC by default, we log not only before starting the vacuum but also > >> when changing cost-related variables. Which is good, I think, because > >> logging the initial values would also be helpful for investigation. > >> However, I think that we don't log the initial vacuum cost values > >> depending on the values. For example, if the > >> autovacuum_vacuum_cost_delay storage option is set to 0, we don't log > >> the initial values. I think that instead of comparing old and new > >> values, we can write the log only if > >> message_level_is_interesting(DEBUG2) is true. That way, we don't need > >> to acquire the lwlock unnecessarily. And the code looks cleaner to me. > >> I've attached the patch (use_message_level_is_interesting.patch) > > > > Thanks for coming up with the case you thought of with storage param for > > cost delay = 0. In that case we wouldn't print the message initially and > > we should fix that. > > > > I disagree, however, that we should condition it only on > > message_level_is_interesting(). > > I think we should keep the logging frequency as committed, but condition > taking > the lock on message_level_is_interesting(). > > > Actually, outside of printing initial values when the autovacuum worker > > first starts (before vacuuming all tables), I don't think we should log > > these values except when they are being updated. Autovacuum workers > > could vacuum tons of small tables and having this print out at least > > once per table (which I know is how it is on master) would be > > distracting. Also, you could be reloading the config to update some > > other GUCs and be oblivious to an ongoing autovacuum and get these > > messages printed out, which I would also find distracting. > > > > You will have to stare very hard at the logs to tell if your changes to > > vacuum cost delay and limit took effect when you reload config. I think > > with our changes to update the values more often, we should take the > > opportunity to make this logging more useful by making it happen only > > when the values are changed. > > For debugging purposes, I think it could also be important information that the cost values are not changed. Personally, I prefer to log the current state rather than deciding for ourselves which events are important. If always logging these values in DEBUG2 had been distracting, we might want to lower it to DEBUG3. > > I would be open to elevating the log level to DEBUG1 for logging only > > updates and, perhaps, having an option if you set log level to DEBUG2, > > for example, to always log these values in VacuumUpdateCosts(). I'm not really sure it's a good idea to change the log messages and events depending on elevel. Do you know we have any precedents ? > > > > I'd even argue that, potentially, having the cost-delay related > > parameters printed at the beginning of vacuuming could be interesting to > > regular VACUUM as well (even though it doesn't benefit from config > > reload while in progress). > > > > To fix the issue you mentioned and ensure the logging is printed when > > autovacuum workers start up before vacuuming tables, we could either > > initialize vacuum_cost_delay and vacuum_cost_limit to something invalid > > that will always be different than what they are set to in > > VacuumUpdateCosts() (not sure if this poses a problem for VACUUM using > > these values since they are set to the defaults for VACUUM). Or, we > > could duplicate this logging message in do_autovacuum(). > > Duplicating logging, maybe with a slightly tailored message, seem the least > bad option. > > > Finally, one other point about message_level_is_interesting(). I liked > > the idea of using it a lot, since log level DEBUG2 will not be the > > common case. I thought of it but hesitated because all other users of > > message_level_is_interesting() are avoiding some memory allocation or > > string copying -- not avoiding take a lock. Making this conditioned on > > log level made me a bit uncomfortable. I can't think of a situation when > > it would be a problem, but it felt a bit off. > > Considering how uncommon DEBUG2 will be in production, I think conditioning > taking a lock on it makes sense. The comment of message_level_is_interesting() says: * This is useful to short-circuit any expensive preparatory work that * might be needed for a logging message. Which can apply to taking a lwlock, I think. > > >> Also, while testing the autovacuum delay with relopt > >> autovacuum_vacuum_cost_delay = 0, I realized that even if we set > >> autovacuum_vacuum_cost_delay = 0 to a table, wi_dob
Re: Various typo fixes
On Tue, Apr 11, 2023 at 03:43:12PM +0100, Thom Brown wrote:
> On Tue, 11 Apr 2023 at 15:39, Justin Pryzby wrote:
> >
> > On Tue, Apr 11, 2023 at 03:36:02PM +0100, Thom Brown wrote:
> > > I've attached a patch with a few typo and grammatical fixes.
> >
> > I think you actually sent the "git-diff" manpage :(
>
> Oh dear, well that's a first. Thanks for pointing out.
Thanks. I think these are all new in v16, right ?
I noticed some of these too - I'll send a patch pretty soon.
|+++ b/doc/src/sgml/logicaldecoding.sgml
|@@ -326,11 +326,11 @@ postgres=# select * from
pg_logical_slot_get_changes('regression_slot', NULL, NU
| connection is alive (for example a node restart would break it). Then,
the
| primary may delete system catalog rows that could be needed by the
logical
| decoding on the standby (as it does not know about the catalog_xmin on
the
|- standby). Existing logical slots on standby also get invalidated if
wal_level
|- on primary is reduced to less than 'logical'. This is done as soon as the
|- standby detects such a change in the WAL stream. It means, that for
walsenders
|- that are lagging (if any), some WAL records up to the wal_level
parameter change
|- on the primary won't be decoded.
|+ standby). Existing logical slots on standby also get invalidated if
|+ wal_level on the primary is reduced to less than
'logical'.
|+ This is done as soon as the standby detects such a change in the WAL
stream.
|+ It means that, for walsenders which are lagging (if any), some WAL
records up
|+ to the wal_level parameter change on the primary won't be decoded.
|
I think "logical" should be a here.
Re: Non-superuser subscription owners
On Tue, Apr 11, 2023 at 5:53 AM Amit Kapila wrote: > I think additionally, we should check that the new owner of the > subscription is not a superuser, otherwise, anyway, this parameter is > ignored. Please find the attached to add this check. I don't see why we should check that. It makes this different from all the other cases and I don't see any benefit. -- Robert Haas EDB: http://www.enterprisedb.com
Re: When to drop src/tools/msvc support
On 4/11/23 10:12 AM, Tom Lane wrote: "Jonathan S. Katz" writes: On 4/11/23 9:49 AM, Tom Lane wrote: Sadly, I think we really have to ship both build systems in v16. But maybe we can make it clear in the release notes + docs that this is slated for deprecation and will be removed from v17? That way we can say "we provided ample warning to move to the new build system." Yes, we absolutely should do that, as already discussed upthread. Ah yes, I saw Andres' notep[1] yesterday and had already forgotten. +1 on that plan. Jonathan [1] https://www.postgresql.org/message-id/20230410223219.4tllxhz3hgwhh4tm%40awork3.anarazel.de OpenPGP_signature Description: OpenPGP digital signature
Re: Various typo fixes
On Tue, 11 Apr 2023 at 15:39, Justin Pryzby wrote: > > On Tue, Apr 11, 2023 at 03:36:02PM +0100, Thom Brown wrote: > > I've attached a patch with a few typo and grammatical fixes. > > I think you actually sent the "git-diff" manpage :( Oh dear, well that's a first. Thanks for pointing out. Re-attached. Thom various_typos_and_grammar_fixes.patch Description: Binary data
Re: Show various offset arrays for heap WAL records
Hi,
static void
infobits_desc(StringInfo buf, uint8 infobits, const char *keyname)
{
appendStringInfo(buf, "%s: [", keyname);
Why can we assume that there will be no space at the end here?
I know we need to be able to avoid doing the comma overwriting if no
flags were set. In general, we expect record description elements to
prepend themselves with commas and spaces, but these infobits, for
example, use a trailing comma and space. If someone adds a description
element with a trailing space, they will trip this assert. We should at
least add a comment explaining this assertion so someone knows what to
do if they trip it.
Otherwise, we can return early if no flags are set. That will probably
make for slightly messier code since we would still have to construct
the empty list.
Assert(buf->data[buf->len - 1] != ' ');
if (infobits & XLHL_XMAX_IS_MULTI)
appendStringInfoString(buf, "IS_MULTI, ");
if (infobits & XLHL_XMAX_LOCK_ONLY)
appendStringInfoString(buf, "LOCK_ONLY, ");
if (infobits & XLHL_XMAX_EXCL_LOCK)
appendStringInfoString(buf, "EXCL_LOCK, ");
if (infobits & XLHL_XMAX_KEYSHR_LOCK)
appendStringInfoString(buf, "KEYSHR_LOCK, ");
if (infobits & XLHL_KEYS_UPDATED)
appendStringInfoString(buf, "KEYS_UPDATED, ");
if (buf->data[buf->len - 1] == ' ')
{
/* Truncate-away final unneeded ", " */
Assert(buf->data[buf->len - 2] == ',');
buf->len -= 2;
buf->data[buf->len] = '\0';
}
appendStringInfoString(buf, "]");
}
Also you didn't add the same assert to truncate_flags_desc().
static void
truncate_flags_desc(StringInfo buf, uint8 flags)
{
appendStringInfoString(buf, "flags: [");
if (flags & XLH_TRUNCATE_CASCADE)
appendStringInfoString(buf, "CASCADE, ");
if (flags & XLH_TRUNCATE_RESTART_SEQS)
appendStringInfoString(buf, "RESTART_SEQS, ");
if (buf->data[buf->len - 1] == ' ')
{
/* Truncate-away final unneeded ", " */
Assert(buf->data[buf->len - 2] == ',');
buf->len -= 2;
buf->data[buf->len] = '\0';
}
appendStringInfoString(buf, "]");
}
Not the fault of this patch, but I also noticed that heap UPDATE and
HOT_UPDATE records have xmax twice and don't differentiate between new
and old. I think that was probably a mistake.
description | off: 119, xmax: 1105, flags: 0x00, old_infobits:
[], new off: 100, xmax 0
else if (info == XLOG_HEAP_UPDATE)
{
xl_heap_update *xlrec = (xl_heap_update *) rec;
appendStringInfo(buf, "off: %u, xmax: %u, flags: 0x%02X, ",
xlrec->old_offnum,
xlrec->old_xmax,
xlrec->flags);
infobits_desc(buf, xlrec->old_infobits_set, "old_infobits");
appendStringInfo(buf, ", new off: %u, xmax %u",
xlrec->new_offnum,
xlrec->new_xmax);
}
else if (info == XLOG_HEAP_HOT_UPDATE)
{
xl_heap_update *xlrec = (xl_heap_update *) rec;
appendStringInfo(buf, "off: %u, xmax: %u, flags: 0x%02X, ",
xlrec->old_offnum,
xlrec->old_xmax,
xlrec->flags);
infobits_desc(buf, xlrec->old_infobits_set, "old_infobits");
appendStringInfo(buf, ", new off: %u, xmax: %u",
xlrec->new_offnum,
xlrec->new_xmax);
}
Also not the fault of this patch, but looking at the output while using
this, I realized truncate record type has a stringified version of its
flags while other record types, like update, don't. Do you think this
makes sense? Perhaps not something we can change now, though...
description | off: 1, xmax: 1183, flags: 0x00, old_infobits: [],
new off: 119, xmax 0
Also not the fault of this patch, but I noticed that leaftopparent is
often InvalidBlockNumber--which shows up as 4294967295. I wonder if
anyone would be confused by this. Maybe devs know that this value is
InvalidBlockNumber. In the future, perhaps we should interpolate the
string "InvalidBlockNumber"?
description | left: 436, right: 389, level: 0, safexid: 0:1091,
leafleft: 436, leafright: 389, leaftopparent: 4294967295
- Melanie
Re: Various typo fixes
On Tue, Apr 11, 2023 at 03:36:02PM +0100, Thom Brown wrote: > I've attached a patch with a few typo and grammatical fixes. I think you actually sent the "git-diff" manpage :( -- Justin
Various typo fixes
Hi, I've attached a patch with a few typo and grammatical fixes. Regards Thom various_typos_and_grammar_fixes.patch Description: Binary data
v12: ERROR: subplan "InitPlan 2 (returns $4)" was not initialized
Reduced from sqlsmith, this query fails under debug_parallel_query=1. The elog was added at: 55416b26a98fcf354af88cdd27fc2e045453b68a But (I'm not sure) the faulty commit may be 8edd0e7946 (Suppress Append and MergeAppend plan nodes that have a single child). postgres=# SET force_parallel_mode =1; CREATE TABLE x (i int) PARTITION BY RANGE (i); CREATE TABLE x1 PARTITION OF x DEFAULT ; select * from pg_class, lateral (select pg_catalog.bit_and(1) from pg_class as sample_1 where case when EXISTS ( select 1 from x where EXISTS ( select 1 from pg_catalog.pg_depend where (sample_1.reltuples is NULL) )) then 1 end is NULL)x where false; -- Justin
Re: When to drop src/tools/msvc support
"Jonathan S. Katz" writes: > On 4/11/23 9:49 AM, Tom Lane wrote: >> Sadly, I think we really have to ship both build systems in v16. > But maybe we can make it clear in the release notes + docs that this is > slated for deprecation and will be removed from v17? That way we can say > "we provided ample warning to move to the new build system." Yes, we absolutely should do that, as already discussed upthread. regards, tom lane
Re: When to drop src/tools/msvc support
On 4/11/23 9:49 AM, Tom Lane wrote: Dave Page writes: On Tue, 11 Apr 2023 at 13:52, Jonathan S. Katz wrote: Do you think we'll have enough info by end of this week to make a decision on whether we can drop MSVC in v16? There's no way I can test anything this week - I'm on leave for most of it and AFK. But, my point was more that there are almost certainly more projects using the MSVC build system than the EDB installers; pgAdmin being just one example. Yeah. Even if EDB can manage this, we're talking about going from "src/tools/msvc is the only option" in v15 to "meson is the only option" in v16. That seems pretty abrupt. Notably, it's assuming that there are no big problems in the meson build system that will take awhile to fix once discovered by users. That's a large assumption for code that hasn't even reached beta yet. [Personal hat] We'll probably see some of this for non-Windows builds, too. Granted, autoconf still seems to work, at least based on my tests. Sadly, I think we really have to ship both build systems in v16. [Personal hat] I've come to this conclusion, too -- it does mean 5 more years of supporting it. But maybe we can make it clear in the release notes + docs that this is slated for deprecation and will be removed from v17? That way we can say "we provided ample warning to move to the new build system." Thanks, Jonathan OpenPGP_signature Description: OpenPGP digital signature
