Re: pgsql: Add support for building with ZSTD.
On Sun, Feb 20, 2022 at 09:45:05AM -0500, Robert Haas wrote: > No issues at all with you adjusting this, but I think that sentence > reads a little awkwardly. Thanks. > Perhaps instead of "The default is > zstd, that would be the command found in > PATH." you could write something like "The default > is zstd, which will search for a command by that > name in the configured PATH." Or maybe something > else is better, not sure exactly, your version just seems a little odd > to me. Okay, done then. We've been using the same wording for all the other variables, and what you are suggesting here sounds much better to me, so I have adjusted all the descriptions this way, and added the ZSTD part. -- Michael signature.asc Description: PGP signature
Re: Uniforms the errors msgs at tuplestore paths
On Sun, Feb 20, 2022 at 11:37:33AM -0300, Ranier Vilela wrote: > I can't see: > plperl.c > pl_exec.c > pttcl.c > > Only jsonfuncs.c, but the error about "materialized mode" is not reported. Melanie has done a nice analysis of all the code paths doing materialization checks for her patch with SRF functions. Though there are parts that can be simplified to reduce the differences in check patterns before doing the overall refactoring, I think that we'd better keep any discussion related to this topic on the other thread rather than splitting the effort across more threads. -- Michael signature.asc Description: PGP signature
Re: Trap errors from streaming child in pg_basebackup to exit early
On Fri, Feb 18, 2022 at 10:00:43PM +0100, Daniel Gustafsson wrote:
> This is good idea, I was going in a different direction earlier with a test
> but
> this is cleaner. The attached 0001 refactors pump_until; 0002 fixes a trivial
> spelling error found while hacking; and 0003 is the previous patch complete
> with a test that passes on Cirrus CI.
This looks rather sane to me, and I can confirm that this passes
the CI and a manual run of MSVC tests with my own box.
+is($node->poll_query_until('postgres',
+ "SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE " .
+ "application_name = '010_pg_basebackup.pl' AND wait_event =
'WalSenderMain' " .
+ "AND backend_type = 'walsender'"), "1", "Walsender killed");
If you do that, don't you have a risk to kill the WAL sender doing the
BASE_BACKUP? That could falsify the test. It seems to me that it
would be safer to add a check on query ~ 'START_REPLICATION' or
something like that.
- diag("aborting wait: program timed out");
- diag("stream contents: >>", $$stream, "<<");
- diag("pattern searched for: ", $untl);
Keeping some of this information around would be useful for
debugging in the refactored routine.
+my $sigchld_bb = IPC::Run::start(
+ [
+ @pg_basebackup_defs, '-X', 'stream', '-D', "$tempdir/sigchld",
+ '-r', '32', '-d', $node->connstr('postgres')
+ ],
I would recommend the use of long options here as a matter to
self-document what this does, and add a comment explaining why
--max-rate is preferable, mainly for fast machines.
--
Michael
signature.asc
Description: PGP signature
pg_stat_get_replication_slot and pg_stat_get_subscription_worker incorrectly marked as proretset
Hi all,
(Author and committer added in CC.)
While reviewing the code of a bunch of SRF functions in the core code,
I have noticed that the two functions mentioned in $subject are marked
as proretset but both functions don't return a set of tuples, just one
record for the object given in input. It is also worth noting that
prorows is set to 1.
This looks like a copy-pasto error that has spread around. The error
on pg_stat_get_subscription_worker is recent as of 8d74fc9, and the
one on pg_stat_get_replication_slot has been introduced in 3fa17d3,
meaning that REL_14_STABLE got it wrong for the second part.
I am aware about the discussions on the parent view for the first
case and its design issues, but it does not change the fact that we'd
better address the second case on HEAD IMO.
Thoughts?
--
Michael
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 7f1ee97f55..aa05b9665f 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5370,16 +5370,16 @@
proargnames => '{pid,status,receive_start_lsn,receive_start_tli,written_lsn,flushed_lsn,received_tli,last_msg_send_time,last_msg_receipt_time,latest_end_lsn,latest_end_time,slot_name,sender_host,sender_port,conninfo}',
prosrc => 'pg_stat_get_wal_receiver' },
{ oid => '6169', descr => 'statistics: information about replication slot',
- proname => 'pg_stat_get_replication_slot', prorows => '1', proisstrict => 'f',
- proretset => 't', provolatile => 's', proparallel => 'r',
+ proname => 'pg_stat_get_replication_slot', proisstrict => 'f',
+ provolatile => 's', proparallel => 'r',
prorettype => 'record', proargtypes => 'text',
proallargtypes => '{text,text,int8,int8,int8,int8,int8,int8,int8,int8,timestamptz}',
proargmodes => '{i,o,o,o,o,o,o,o,o,o,o}',
proargnames => '{slot_name,slot_name,spill_txns,spill_count,spill_bytes,stream_txns,stream_count,stream_bytes,total_txns,total_bytes,stats_reset}',
prosrc => 'pg_stat_get_replication_slot' },
{ oid => '8523', descr => 'statistics: information about subscription worker',
- proname => 'pg_stat_get_subscription_worker', prorows => '1', proisstrict => 'f',
- proretset => 't', provolatile => 's', proparallel => 'r',
+ proname => 'pg_stat_get_subscription_worker', proisstrict => 'f',
+ provolatile => 's', proparallel => 'r',
prorettype => 'record', proargtypes => 'oid oid',
proallargtypes => '{oid,oid,oid,oid,oid,text,xid,int8,text,timestamptz}',
proargmodes => '{i,i,o,o,o,o,o,o,o,o}',
signature.asc
Description: PGP signature
Re: make tuplestore helper function
On Thu, Feb 17, 2022 at 04:10:01PM +0900, Michael Paquier wrote:
> Asserting that we are in the correct memory context in when calling
> MakeFuncResultTuplestore() sounds rather sensible from here as per the
> magics done in the various json functions. Still, it really feels
> like we could do a more centralized consolidation of the whole.
So, I got my hands on this area, and found myself applying 07daca5 as
a first piece of the puzzle. Anyway, after more review today, I have
bumped into more pieces that could be consolidated, and finished with
the following patch set:
- 0001 changes a couple of SRFs that I found worth simplifying. These
refer mostly to the second and fourth group mentioned upthread by
Melanie, with two exceptions: pg_tablespace_databases() where it is
not worth changing to keep it backward-compatible and pg_ls_dir() as
per its one-arg version. That's a nice first cut in itself.
- 0002 changes a couple of places to unify some existing SRF checks,
that I bumped into on the way. The value here is in using the same
error messages everywhere, reducing the translation effort and
generating the same errors for all cases based on the same conditions.
This eases the review of the next patch.
- 0003 is the actual refactoring meat, where I have been able to move
the check on expectedDesc into MakeFuncResultTuplestore(), shaving
more code than previously. If you discard the cases of patch 0001, it
should actually be possible to set setResult, setDesc and returnMode
within the new function, which would feel natural as that's the place
where we create the function's tuplestore and we want to materialize
its contents. The cases with the JSON code are also a bit hairy and
need more thoughts, but we could also cut this part of the code from
the initial refactoring effort.
So, for now, 0001 and 0002 look like rather committable pieces. 0003
needs a bit more thoughts about all the corner cases we need to
consider, mostly what I am mentioning above. Perhaps the pieces of
0003 related to pg_options_to_table() should be moved to 0001 as a
matter of clarity.
--
Michael
From 0b72ccfe50dfb9e8a3d7f71fd15b6c653ead22a7 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Mon, 21 Feb 2022 16:15:26 +0900
Subject: [PATCH v7 1/3] Clean up and simplify some code related to SRFs
---
src/backend/commands/prepare.c | 31 +++---
src/backend/utils/misc/guc.c | 20 ++---
src/backend/utils/misc/pg_config.c | 69 --
src/backend/utils/mmgr/portalmem.c | 23 +++---
4 files changed, 35 insertions(+), 108 deletions(-)
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index e0c985ef8b..dce30aed6c 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -22,6 +22,7 @@
#include "catalog/pg_type.h"
#include "commands/createas.h"
#include "commands/prepare.h"
+#include "funcapi.h"
#include "miscadmin.h"
#include "nodes/nodeFuncs.h"
#include "parser/analyze.h"
@@ -716,30 +717,13 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("materialize mode required, but it is not allowed in this context")));
+ if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
+ elog(ERROR, "return type must be a row type");
+
/* need to build tuplestore in query context */
per_query_ctx = rsinfo->econtext->ecxt_per_query_memory;
oldcontext = MemoryContextSwitchTo(per_query_ctx);
- /*
- * build tupdesc for result tuples. This must match the definition of the
- * pg_prepared_statements view in system_views.sql
- */
- tupdesc = CreateTemplateTupleDesc(7);
- TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name",
- TEXTOID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 2, "statement",
- TEXTOID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 3, "prepare_time",
- TIMESTAMPTZOID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 4, "parameter_types",
- REGTYPEARRAYOID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 5, "from_sql",
- BOOLOID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 6, "generic_plans",
- INT8OID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 7, "custom_plans",
- INT8OID, -1, 0);
-
/*
* We put all the tuples into a tuplestore in one scan of the hashtable.
* This avoids any issue of the hashtable possibly changing between calls.
@@ -747,6 +731,9 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
tupstore =
tuplestore_begin_heap(rsinfo->allowedModes & SFRM_Materialize_Random,
false, work_mem);
+ rsinfo->returnMode = SFRM_Materialize;
+ rsinfo->setResult = tupstore;
+ rsinfo->setDesc = tupdesc;
/* generate junk in short-term context */
Memory
Re: Assert in pageinspect with NULL pages
On Mon, Feb 21, 2022 at 10:00:00AM +0300, Alexander Lakhin wrote: > Could you please confirm before committing the patchset that it fixes > the bug #16527 [1]? Or maybe I could check it? > (Original patch proposed by Daria doesn't cover that case, but if the > patch going to be improved, probably it's worth fixing that bug too.) I have not directly checked, but that looks like the same issue to me. By the way, I was waiting for an updated patch set, based on the review provided upthread: https://www.postgresql.org/message-id/Yg8MPrV49/9us...@paquier.xyz And it seems better to group everything in a single commit as the same areas are touched. -- Michael signature.asc Description: PGP signature
Re: pg_stat_get_replication_slot and pg_stat_get_subscription_worker incorrectly marked as proretset
On Mon, Feb 21, 2022 at 04:19:59PM +0530, Amit Kapila wrote: > On Mon, Feb 21, 2022 at 11:21 AM Masahiko Sawada > wrote: >> Agreed. >> > > +1. How about attached? That's the same thing as what I sent upthread, so that's correct to me, except that I have fixed both functions :) You are not touching pg_stat_get_subscription_worker() because the plan is to revert it from HEAD? I have not followed the other discussion closely. -- Michael signature.asc Description: PGP signature
Re: Trap errors from streaming child in pg_basebackup to exit early
On Mon, Feb 21, 2022 at 03:11:30PM +0100, Daniel Gustafsson wrote:
>On 21 Feb 2022, at 03:03, Michael Paquier wrote:
>> +is($node->poll_query_until('postgres',
>> + "SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE " .
>> + "application_name = '010_pg_basebackup.pl' AND wait_event =
>> 'WalSenderMain' " .
>> + "AND backend_type = 'walsender'"), "1", "Walsender killed");
>> If you do that, don't you have a risk to kill the WAL sender doing the
>> BASE_BACKUP? That could falsify the test. It seems to me that it
>> would be safer to add a check on query ~ 'START_REPLICATION' or
>> something like that.
>
> I don't think there's a risk, but I've added the check on query as well since
> it also makes it more readable.
Okay, thanks.
>> - diag("aborting wait: program timed out");
>> - diag("stream contents: >>", $$stream, "<<");
>> - diag("pattern searched for: ", $untl);
>> Keeping some of this information around would be useful for
>> debugging in the refactored routine.
>
> Maybe, but we don't really have diag output anywhere in the modules or the
> tests so I didn't see much of a precedent for keeping it. Inspectig the repo
> I
> think we can remove two more in pg_rewind, which I just started a thread for.
Hmm. If you think this is better this way, I won't fight hard on this
point, either.
The patch set looks fine overall.
--
Michael
signature.asc
Description: PGP signature
Re: shared_preload_libraries = 'pg_stat_statements' failing with installcheck (compute_query_id = auto)
On Fri, Feb 18, 2022 at 05:38:56PM +0800, Julien Rouhaud wrote: > On Fri, Feb 18, 2022 at 05:22:36PM +0900, Michael Paquier wrote: >> So, I have been looking at this problem, and I don't see a problem in >> doing something like the attached, where we add a "regress" mode to >> compute_query_id that is a synonym of "auto". Or, in short, we have >> the default of letting a module decide if a query ID can be computed >> or not, at the exception that we hide its result in EXPLAIN outputs. >> >> Julien, what do you think? > > I don't see any problem with that patch. Okay, thanks. I have worked on that today and applied the patch down to 14, but without the change to enforce the parameter in the databases created by pg_regress. Perhaps we should do that in the long-term, but it is also possible to pass down the option to PGOPTIONS via command line as compute_query_id is a SUSET or just set it in postgresql.conf, and being able to do so is enough to address all the cases I have seen and/or could think about. -- Michael signature.asc Description: PGP signature
Re: pg_stat_get_replication_slot and pg_stat_get_subscription_worker incorrectly marked as proretset
On Mon, Feb 21, 2022 at 05:00:43PM +0530, Amit Kapila wrote: > On Mon, Feb 21, 2022 at 4:56 PM Michael Paquier wrote: >> That's the same thing as what I sent upthread, so that's correct to >> me, except that I have fixed both functions :) > > Sorry, I hadn't looked at your patch. That's fine. This is something you have committed, after all. >> You are not touching pg_stat_get_subscription_worker() because the >> plan is to revert it from HEAD? I have not followed the other >> discussion closely. >> > > It is still under discussion. I'll take the necessary action along > with other things related to that view based on the conclusion on that > thread. Sounds good to me. The patch you are proposing upthread is OK for me. -- Michael signature.asc Description: PGP signature
Re: pg_stat_get_replication_slot and pg_stat_get_subscription_worker incorrectly marked as proretset
On Wed, Feb 23, 2022 at 09:52:02AM +0530, Amit Kapila wrote: > Thanks, so you are okay with me pushing that patch just to HEAD. Yes, I am fine with that. I am wondering about patching the second function though, to avoid any risk of forgetting it, but I am fine to leave that to your judgement. > We don't want to backpatch this to 14 as this is a catalog change and > won't cause any user-visible issue, is that correct? Yup, that's a HEAD-only cleanup, I am afraid. -- Michael signature.asc Description: PGP signature
Re: make tuplestore helper function
On Mon, Feb 21, 2022 at 04:41:17PM +0900, Michael Paquier wrote:
> So, I got my hands on this area, and found myself applying 07daca5 as
> a first piece of the puzzle. Anyway, after more review today, I have
> bumped into more pieces that could be consolidated, and finished with
> the following patch set:
> - 0001 changes a couple of SRFs that I found worth simplifying. These
> refer mostly to the second and fourth group mentioned upthread by
> Melanie, with two exceptions: pg_tablespace_databases() where it is
> not worth changing to keep it backward-compatible and pg_ls_dir() as
> per its one-arg version. That's a nice first cut in itself.
> - 0002 changes a couple of places to unify some existing SRF checks,
> that I bumped into on the way. The value here is in using the same
> error messages everywhere, reducing the translation effort and
> generating the same errors for all cases based on the same conditions.
> This eases the review of the next patch.
These two have been now applied, with some comment improvements and
the cleanup of pg_options_to_table() done in 0001, and a slight change
in 0002 for pageinspect where a check was not necessary for a BRIN
code path.
> - 0003 is the actual refactoring meat, where I have been able to move
> the check on expectedDesc into MakeFuncResultTuplestore(), shaving
> more code than previously. If you discard the cases of patch 0001, it
> should actually be possible to set setResult, setDesc and returnMode
> within the new function, which would feel natural as that's the place
> where we create the function's tuplestore and we want to materialize
> its contents. The cases with the JSON code are also a bit hairy and
> need more thoughts, but we could also cut this part of the code from
> the initial refactoring effort.
This is the remaining piece, as attached, that I have not been able to
poke much at yet.
--
Michael
From f23525ef3492ecaf310a59c431be7d3f3f5237af Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Thu, 24 Feb 2022 17:27:55 +0900
Subject: [PATCH v8] Introduce MakeFuncResultTuplestore()
This is the main patch from Melanie, that I have tweaked a bit. Note
that I am not completely done with it ;p
---
src/include/funcapi.h | 2 +
src/backend/access/transam/xlogfuncs.c| 16 +---
src/backend/commands/event_trigger.c | 32 +---
src/backend/commands/extension.c | 48 +---
src/backend/commands/prepare.c| 17 +
src/backend/foreign/foreign.c | 14 +---
src/backend/libpq/hba.c | 19 +
src/backend/replication/logical/launcher.c| 16 +---
.../replication/logical/logicalfuncs.c| 16 +---
src/backend/replication/logical/origin.c | 19 +
src/backend/replication/slotfuncs.c | 16 +---
src/backend/replication/walsender.c | 16 +---
src/backend/storage/ipc/shmem.c | 16 +---
src/backend/utils/adt/datetime.c | 17 +
src/backend/utils/adt/genfile.c | 31 +---
src/backend/utils/adt/jsonfuncs.c | 73 +++
src/backend/utils/adt/mcxtfuncs.c | 16 +---
src/backend/utils/adt/misc.c | 14 +---
src/backend/utils/adt/pgstatfuncs.c | 48 +---
src/backend/utils/adt/varlena.c | 14 +---
src/backend/utils/fmgr/funcapi.c | 38 ++
src/backend/utils/misc/guc.c | 15 +---
src/backend/utils/misc/pg_config.c| 13 +---
src/backend/utils/mmgr/portalmem.c| 17 +
24 files changed, 82 insertions(+), 461 deletions(-)
diff --git a/src/include/funcapi.h b/src/include/funcapi.h
index ba927c2f33..b9f9e92d1a 100644
--- a/src/include/funcapi.h
+++ b/src/include/funcapi.h
@@ -229,6 +229,8 @@ extern TupleDesc BlessTupleDesc(TupleDesc tupdesc);
extern AttInMetadata *TupleDescGetAttInMetadata(TupleDesc tupdesc);
extern HeapTuple BuildTupleFromCStrings(AttInMetadata *attinmeta, char **values);
extern Datum HeapTupleHeaderGetDatum(HeapTupleHeader tuple);
+extern Tuplestorestate *MakeFuncResultTuplestore(FunctionCallInfo fcinfo,
+ TupleDesc *result_tupdesc);
/*--
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index 12e2bf4135..2fc1ed023c 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -178,24 +178,10 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS)
XLogRecPtr stoppoint;
SessionBackupState status = get_backup_status();
- /* check to see if caller supports us returning a tuplestore */
- if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo))
- ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("set-valued function called in context that cannot accept a set")));
- if (!(rsinfo->allowedModes & SFRM_Materia
Re: [PATCH] Expose port->authn_id to extensions and triggers
On Thu, Feb 24, 2022 at 12:15:40AM +, Jacob Champion wrote:
> Stephen pointed out [1] that the authenticated identity that's stored
> in MyProcPort can't be retrieved by extensions or triggers. Attached is
> a patch that provides both a C API and a SQL function for retrieving
> it.
>
> GetAuthenticatedIdentityString() is a mouthful but I wanted to
> differentiate it from the existing GetAuthenticatedUserId(); better
> names welcome. It only exists as an accessor because I wasn't sure if
> extensions outside of contrib were encouraged to rely on the internal
> layout of struct Port. (If they can, then that call can go away
> entirely.)
+char *
+GetAuthenticatedIdentityString(void)
+{
+ if (!MyProcPort || !MyProcPort->authn_id)
+ return NULL;
+
+ return pstrdup(MyProcPort->authn_id);
I don't quite see the additional value that this API brings as
MyProcPort is directly accessible, and contrib modules like
postgres_fdw and sslinfo just use that to find the data of the current
backend. Cannot a module like pgaudit, through the elog hook, do its
work with what we have already in place?
What's the use case for a given session to be able to report back
only its own authn through SQL?
I could still see a use case for that at a more global level with
beentrys, but it looked like there was not much interest the last time
I dropped this idea.
--
Michael
signature.asc
Description: PGP signature
Re: Typo in pgbench messages.
On Thu, Feb 24, 2022 at 06:38:57PM +0900, Tatsuo Ishii wrote: > >> I think you are right. In English there's should be no space between > >> number and "%". > >> AFAIK other parts of PostgreSQL follow the rule. > > I think it's better to back-patch this to stable branches if there's > no objection. Thought? That's only cosmetic, so I would just bother about HEAD if I were to change something like that (I would not bother at all, personally). One argument against a backpatch is that this would be disruptive with tools that parse and analyze the output generated by pgbench. Fabien, don't you have some tools and/or wrappers doing exactly that? -- Michael signature.asc Description: PGP signature
Re: Uniforms the errors msgs at tuplestore paths
On Thu, Feb 24, 2022 at 08:30:02AM -0300, Ranier Vilela wrote: > Thanks for the commit Michael. No problem. For the archives, this is e77216f. -- Michael signature.asc Description: PGP signature
Re: Is it correct to update db state in control file as "shutting down" during end-of-recovery checkpoint?
On Fri, Feb 25, 2022 at 01:09:53PM -0800, Nathan Bossart wrote: > This one has been quiet for a while. Should we mark it as > returned-with-feedback? Yes, that's my feeling and I got cold feet about this change. This patch would bring some extra visibility for something that's not incorrect either on HEAD, as end-of-recovery checkpoints are the same things as shutdown checkpoints. And there is an extra argument where back-patching would become a bit more tricky in an area that's already a lot sensitive. -- Michael signature.asc Description: PGP signature
Re: Frontend error logging style
On Fri, Feb 25, 2022 at 12:15:25PM -0500, Tom Lane wrote:
> I feel that the reasonable alternatives are either to drop the FATAL
> log level, or try to make it actually mean something by consistently
> using it for errors that are indeed fatal. I had a go at doing the
> latter, but eventually concluded that that way madness lies. It's
> not too hard to use "pg_log_fatal" when there's an exit(1) right
> after it, but there are quite a lot of cases where a subroutine
> reports an error and returns a failure code to its caller, whereupon
> the caller exits. Either the subroutine has to make an unwarranted
> assumption about what its callers will do, or we need to make an API
> change to allow the subroutine itself to exit(), or we are going to
> present a user experience that is inconsistently different depending
> on internal implementation details.
Nice code cut.
> I conclude that we ought to drop the separate FATAL level and just
> use ERROR instead.
FWIW, I have found the uses of pg_log_error() and pg_log_fatal()
rather confusing in some of the src/bin/ tools, so I am in favor of
removing completely one or the other, and getting rid of FATAL is the
best choice.
> The attached revision does that, standardizes on pg_fatal() as the
> abbreviation for pg_log_error() + exit(1), and invents detail/hint
> features as per previous discussion.
+#define pg_log_warning_hint(...) do { \
+ if (likely(__pg_log_level <= PG_LOG_WARNING)) \
+ pg_log_generic(PG_LOG_WARNING, PG_LOG_HINT, __VA_ARGS__);
\
} while(0)
Hm. I am not sure to like much this abstraction by having so many
macros that understand the log level through their name. Wouldn't it
be cleaner to have only one pg_log_hint() and one pg_log_detail() with
the log level passed as argument of the macro?
--
Michael
signature.asc
Description: PGP signature
Re: [PATCH] Expose port->authn_id to extensions and triggers
On Fri, Feb 25, 2022 at 01:23:49PM -0800, Andres Freund wrote: > Looks to me like authn_id isn't synchronized to parallel workers right now. So > the function will return the wrong thing when executed as part of a parallel > query. FWIW, I am not completely sure what's the use case for being able to see the authn of the current session through a trigger. We expose that when log_connections is enabled, for audit purposes. I can also get behind something more central so as one can get a full picture of the authn used by a bunch of session, particularly with complex HBA policies, but this looks rather limited to me in usability. Perhaps that's not enough to stand as an objection, though, and the patch is dead simple. > I don't think we should add further functions not prefixed with pg_. Yep. > Perhaps a few tests for less trivial authn_ids could be worthwhile? > E.g. certificate DNs. Yes, src/test/ssl would handle that just fine. Now, this stuff already looks after authn results with log_connections=on, so that feels like a duplicate. -- Michael signature.asc Description: PGP signature
Re: Commitfest manager for 2022-03
On Fri, Feb 25, 2022 at 01:58:55PM -0600, David Steele wrote: > On 2/25/22 12:39, Greg Stark wrote: >> I would like to volunteer. > > Since I've done the March CF for the last seven years, I thought it would be > good if I chimed in. I've been agonizing a bit because I have travel planned > during March and while I *could* do it I don't think I'd be able to do a > good job. That's a time-consuming task, and there have been little occasions to enjoy travelling lately, so have a good time :) > I've been hoping somebody would volunteer, so I'm all in favor of you being > CF. Greg as CFM would be fine. It's nice to see someone volunteer. -- Michael signature.asc Description: PGP signature
Re: PATCH: add "--config-file=" option to pg_rewind
On Fri, Feb 25, 2022 at 10:35:49AM +0100, Gunnar "Nick" Bluth wrote:
> Am 24.02.22 um 14:46 schrieb Daniel Gustafsson:
>> Actually, I think this looks like a saner approach. Putting a config setting
>> in two place (postgresql.conf and on the commandline for pg_rewind) is a
>> recipe
>> for them diverging.
FWIW, I have a bad feeling about passing down directly a command
through an option itself part of a command, so what's discussed on
this thread is refreshing.
+ } else {
+ snprintf(postgres_cmd, sizeof(postgres_cmd),
+"\"%s\" -D \"%s\" --config_file=\"%s\" -C
restore_command",
+postgres_exec_path, datadir_target, config_file);
+ }
Shouldn't this one use appendShellString() on config_file?
--
Michael
signature.asc
Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
On Wed, Feb 23, 2022 at 09:44:58AM -0800, Nathan Bossart wrote: > On Wed, Feb 23, 2022 at 12:59:59PM +0800, Julien Rouhaud wrote: >> 0001 adds a new pg_ident_file_mappings view, which is basically the same as >> pg_hba_file_rules view but for mappings. It's probably already useful, for >> instance if you need to tweak some regexp. > > This seems reasonable. Interesting. One can note that hba.c is already large, and this makes the file larger. I'd like to think that it would be better to move all the code related to the SQL functions for pg_hba.conf and such to a new hbafuncs.c under adt/. Would that make sense? -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
On Sat, Feb 26, 2022 at 02:27:15PM +0800, Julien Rouhaud wrote: > I'm fine with it. Assuming that you meant to move also the underlying > functions that goes with it (fill_hba_line and such), that would end up > removing about 15% of hba.c (after applying 0001, 0002 and 0003). Cool. Thanks for the number. > Note that in order to do so we would need to expose quite a lot more about hba > internals, like tokenize_file() and parse_hba_line(), along with structs > HbaToken and TokenizedLine. I'd be rather fine with exposing all that in the shape of a clean split, renaming some of those structures and/or function with an Hba-like prefix, for consistency. -- Michael signature.asc Description: PGP signature
Re: Commitfest manager for 2022-03
On Sun, Feb 27, 2022 at 04:51:16PM +0800, Julien Rouhaud wrote: > I don't really understand what that field is supposed to mean. But now that > we're in the final pg15 commit fest, wouldn't it be simpler to actually move > the patches for which there's a agreement that they can't make it to pg15? > Tagging them and letting them rot for a month isn't helpful for the authors or > the CFMs, especially when there are already 250 patches that needs to be > handled. Yes, I don't see any point in having a new tag just to mark patches that will have to be moved to the next CF anyway. These should just be moved to the July one rather than stay in the March one. -- Michael signature.asc Description: PGP signature
Re: Checkpointer sync queue fills up / loops around pg_usleep() are bad
On Sat, Feb 26, 2022 at 01:39:42PM -0800, Andres Freund wrote: > I suspect the easiest is to just convert that usleep to a WaitLatch(). That'd > require adding a new enum value to WaitEventTimeout in 14. Which probably is > fine? We've added wait events in back-branches in the past, so this does not strike me as a problem as long as you add the new entry at the end of the enum, while keeping things ordered on HEAD. In recent memory, I think that only some of the extensions published by PostgresPro rely on the enums in this area. -- Michael signature.asc Description: PGP signature
Re: PATCH: add "--config-file=" option to pg_rewind
On Sat, Feb 26, 2022 at 09:55:20AM +0100, Gunnar "Nick" Bluth wrote: > Am 26.02.22 um 06:51 schrieb Michael Paquier: >> Shouldn't this one use appendShellString() on config_file? > > It probably should, yes. I don't fancy this repetitive code myself. > But then, pg_rewind as a whole could use an overhaul. I don't see any use of > PQExpBuffer in it, but a lot of char* ... Having a lot of char* does not necessarily mean that all of them have to be changed to accomodate with PQExpBuffer. My guess that this is a case-by-case, where we should apply that in places where it makes the code cleaner to follow. In the case of your patch though, the two areas changed would make the logic correct, instead, because we have to apply correct quoting rules to any commands executed. > GSOC project? ;-) It does not seem so, though there are surely more areas that could gain in clarity. -- Michael signature.asc Description: PGP signature
Re: pg_stat_statements: remove redundant function call in pg_stat_statements_internal
On Sun, Feb 27, 2022 at 07:55:26PM +0800, Julien Rouhaud wrote: > Indeed. I doubt it will make any real difference but it doesn't hurt to fix > it. > > Patch looks good to me. Yes, let's clean up that on HEAD. No objections from here. I'll do that tomorrow or so. -- Michael signature.asc Description: PGP signature
Re: Is it correct to update db state in control file as "shutting down" during end-of-recovery checkpoint?
On Mon, Feb 28, 2022 at 10:51:06AM +0900, Kyotaro Horiguchi wrote: > That sounds like we should reject the patch as we don't agree to its > objective. If someday end-of-recovery checkpoints functionally > diverge from shutdown checkpoints but leave (somehow) the transition > alone, we may visit this again but it would be another proposal. The patch has been already withdrawn in the CF app. -- Michael signature.asc Description: PGP signature
Re: pg_stat_statements: remove redundant function call in pg_stat_statements_internal
On Sun, Feb 27, 2022 at 09:08:56PM +0900, Michael Paquier wrote: > Yes, let's clean up that on HEAD. No objections from here. I'll do > that tomorrow or so. And done. -- Michael signature.asc Description: PGP signature
Re: make tuplestore helper function
On Thu, Feb 24, 2022 at 08:25:06PM +0900, Michael Paquier wrote:
> This is the remaining piece, as attached, that I have not been able to
> poke much at yet.
So, I have finally poked at this last part of the patch set, and I
found that we can be more aggressive with the refactoring, by moving
into MakeFuncResultTuplestore() the parts where we save the tuplestore
and the tupledesc in the per-query memory context. There are two
pieces that matter once things are reshaped:
- The tuple descriptor may need some extra validation via
BlessTupleDesc() when it comes from a transient record datatype,
something that happens for most of the subroutines related to the JSON
functions.
- expectedDesc is sometimes required by the caller, though most of the
time just needs to be built with the more expensive
get_call_result_type().
In order to keep things pluggable at will, MakeFuncResultTuplestore()
has been changed to access a set of bits32 flags, able to control the
two options above. With this facility in place, I have been able to
cut much more code than the initial patch, roughly twice as of:
24 files changed, 157 insertions(+), 893 deletions(-)
This seems in rather good shape to me, the changes are
straight-forward and the code cut is really good, so I'd like to move
on with that. 0001 is the initial patch, and 0002 is the extra
refactoring I have been working on. The plan would be to merge both,
but I am sending a split to ease any checks on what I have changed.
Comments or objections?
--
Michael
From d45e9a7031017e13e5429ff985b36ddafdfdb443 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Thu, 24 Feb 2022 17:27:55 +0900
Subject: [PATCH v9 1/2] Introduce MakeFuncResultTuplestore()
This is the main patch from Melanie, that I have tweaked a bit. Note
that I am not completely done with it ;p
---
src/include/funcapi.h | 2 +
src/backend/access/transam/xlogfuncs.c| 16 +---
src/backend/commands/event_trigger.c | 32 +---
src/backend/commands/extension.c | 48 +---
src/backend/commands/prepare.c| 17 +
src/backend/foreign/foreign.c | 14 +---
src/backend/libpq/hba.c | 19 +
src/backend/replication/logical/launcher.c| 16 +---
.../replication/logical/logicalfuncs.c| 16 +---
src/backend/replication/logical/origin.c | 19 +
src/backend/replication/slotfuncs.c | 16 +---
src/backend/replication/walsender.c | 16 +---
src/backend/storage/ipc/shmem.c | 16 +---
src/backend/utils/adt/datetime.c | 17 +
src/backend/utils/adt/genfile.c | 31 +---
src/backend/utils/adt/jsonfuncs.c | 73 +++
src/backend/utils/adt/mcxtfuncs.c | 16 +---
src/backend/utils/adt/misc.c | 14 +---
src/backend/utils/adt/pgstatfuncs.c | 48 +---
src/backend/utils/adt/varlena.c | 14 +---
src/backend/utils/fmgr/funcapi.c | 38 ++
src/backend/utils/misc/guc.c | 15 +---
src/backend/utils/misc/pg_config.c| 13 +---
src/backend/utils/mmgr/portalmem.c| 17 +
24 files changed, 82 insertions(+), 461 deletions(-)
diff --git a/src/include/funcapi.h b/src/include/funcapi.h
index ba927c2f33..b9f9e92d1a 100644
--- a/src/include/funcapi.h
+++ b/src/include/funcapi.h
@@ -229,6 +229,8 @@ extern TupleDesc BlessTupleDesc(TupleDesc tupdesc);
extern AttInMetadata *TupleDescGetAttInMetadata(TupleDesc tupdesc);
extern HeapTuple BuildTupleFromCStrings(AttInMetadata *attinmeta, char **values);
extern Datum HeapTupleHeaderGetDatum(HeapTupleHeader tuple);
+extern Tuplestorestate *MakeFuncResultTuplestore(FunctionCallInfo fcinfo,
+ TupleDesc *result_tupdesc);
/*--
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index 12e2bf4135..2fc1ed023c 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -178,24 +178,10 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS)
XLogRecPtr stoppoint;
SessionBackupState status = get_backup_status();
- /* check to see if caller supports us returning a tuplestore */
- if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo))
- ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("set-valued function called in context that cannot accept a set")));
- if (!(rsinfo->allowedModes & SFRM_Materialize))
- ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("materialize mode required, but it is not allowed in this context")));
-
- /* Build a tuple descriptor for our result type */
- if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
- elog(ERROR, "return type must be a row type");
-
per_query_ctx = rsinfo->econtext->ecxt_per_query_memory;
oldco
Re: Allow file inclusion in pg_hba and pg_ident files
On Sat, Feb 26, 2022 at 02:50:33PM +0800, Julien Rouhaud wrote: > Of course. I was thinking using "auth" for something that's common to pg_hba > and pg_ident (like e.g. TokenizeAuthFile()), and otherwise keep the current > hba/ident prefix. Okay, thanks. > Unless someone object or suggest better naming in the next few days I will > take > care of that. I don't have an opinion to share about 0002 and 0003 yet, but 0001 seems like a good idea on its own. -- Michael signature.asc Description: PGP signature
Re: psql: Make SSL info display more compact
On Mon, Feb 28, 2022 at 10:50:01AM +, Dagfinn Ilmari Mannsåker wrote: > Daniel Gustafsson writes: >> On 28 Feb 2022, at 10:02, Peter Eisentraut >> wrote: >> This was originally done, but all client side changes reverted as there still >> are server versions in production which allow compression. > > How about making it show "compression: on" if compression is on, but > nothing in the common "off" case? Hm, no, as it can be useful to know that compression is disabled when connecting to an old server. What about that making the information shown version-aware? I would choose to show the compression part only for server versions where it is settable. -- Michael signature.asc Description: PGP signature
Re: PATCH: add "--config-file=" option to pg_rewind
On Sun, Feb 27, 2022 at 02:30:33PM +0100, Gunnar "Nick" Bluth wrote:
> That's universally true ;-)
-# Internal routine to enable archive recovery command on a standby node
+# Internal routine to enable archive recovery command on a standby node.
+# Returns generated restore_command.
sub enable_restoring
{
my ($self, $root_node, $standby) = @_;
@@ -1103,7 +1104,7 @@ restore_command = '$copy_command'
{
$self->set_recovery_mode();
}
- return;
+ return $copy_command;
I don't like this change. This makes an existing routine do some
extra work for something it is not designed for. You could get this
information with a postgres -C command, for example. Now, you cannot
use postgres -C because of.. Reasons (1a9d802). But you could use a
pg_ctl command for the same purpose. I guess that we'd better
refactor this into a new routine of Cluster.pm where we execute a
pg_ctl command and return both stdout and stderr back to the caller?
The TAP part could be refactored on its own.
+In case the postgresql.conf of your
target cluster is not in the
+target pgdata and you want to use the -c /
--restore-target-wal option,
+provide a (relative or absolute) path to the
postgresql.conf using this option.
This could do a better job to explain in which cases this option can
be used for. You could say something like that:
"This option specifies a path to a configuration file to be used when
starting the target cluster. This makes easier the use of
-c/--restore-target-wal by setting restore_command in the extra
configuration file specified via this option."
Hmm. What about the target cluster started in --single mode as of
ensureCleanShutdown()? Should we try to apply this option also in
this case?
--
Michael
signature.asc
Description: PGP signature
Re: PATCH: add "--config-file=" option to pg_rewind
On Mon, Feb 28, 2022 at 08:48:23PM +0100, Gunnar "Nick" Bluth wrote: > So, how should we call the global "find the * 'postgres' executable and > boil out if that fails" function? > char postgres_exec_path[MAXPGPATH] = findPostgresExec(); > ? That would mean only a couple of lines gained, and the readability gained is minimal, so I'd be fine to keep the code as-is. I am not sure about the full patch set yet, but the refactoring of the commands to use PQExpBuffer is good by itself, so I have extracted this part of the patch and applied that for now. -- Michael signature.asc Description: PGP signature
Re: Proposal: Support custom authentication methods using hooks
On Mon, Feb 28, 2022 at 04:42:55PM -0500, Stephen Frost wrote: > Keeping it around will just push out the point at which everyone will > finally be done with it, as there's really only two groups: those who > have already moved to scram, and those who won't move until they want to > upgrade to a release that doesn't have md5. FWIW, I am not sure if we are at this point yet. An extra reason to remove it would be that it is a support burden, but I don't have seen in recent memory any problems related to it that required any deep changes in the way to use it, and its code paths are independent. The last time I played with this area is the recent error handling improvement with cryptohashes but MD5 has actually helped here in detecting the problem as a patched OpenSSL would complain if trying to use MD5 as hash function when FIPS is enabled. -- Michael signature.asc Description: PGP signature
Re: [PATCH] Expose port->authn_id to extensions and triggers
On Mon, Feb 28, 2022 at 04:00:36PM -0500, Stephen Frost wrote: > * Jacob Champion (pchamp...@vmware.com) wrote: >> On Fri, Feb 25, 2022 at 01:23:49PM -0800, Andres Freund wrote: >>> Looks to me like authn_id isn't synchronized to parallel workers right now. >>> So >>> the function will return the wrong thing when executed as part of a parallel >>> query. >> >> Thanks for the catch. It looks like MyProcPort is left empty, and other >> functions that rely on like inet_server_addr() are marked parallel- >> restricted, so I've done the same in v4. > > That's probably alright. I'd say as well that this is right as-is. If it happens that there is a use-case for making this parallel aware in the future, it could be done. Now, it may be a bit weird to make parallel workers inherit the authn ID of the parent as these did not go through authentication, no? Letting this function being run only by the leader feels intuitive. > Yeah, we really should make this available to trigger-based auditing > systems too and not just through log_connections which involves a great > deal more log parsing and work external to the database to figure out > who did what. Okay, I won't fight hard on that if all of you think that this is useful for a given session. >> Subject: [PATCH v4] Add API to retrieve authn_id from SQL >> >> The authn_id field in MyProcPort is currently only accessible to the >> backend itself. Add a SQL function, pg_session_authn_id(), to expose >> the field to triggers that may want to make use of it. > > Only did a quick look but generally looks reasonable to me. The function and the test are fine, pgperltidy complains a bit about the format of the tests. Ayway, this function needs to be documented. I think that you should just add that in "Session Information Functions" in func.sgml, same area as current_user(). The last time we talked about the authn ID, one thing we discussed about was how to describe that in a good way to the user, which is why the section of log_connections was reworked a bit. And we don't have yet any references to what an authenticated identity is in the docs. There is no need to update catversion.h in the patch, committers usually take care of that and that's an area of the code that conflicts a lot. -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
On Mon, Feb 28, 2022 at 07:42:17PM +0800, Julien Rouhaud wrote: > Done in attached v2. I did the split in a separate commit, as the diff is > otherwise unreadable. While at it I also fixed a few minor issues (I missed a > MemoryContextDelete, and now avoid relying on inet_net_pton which apparently > doesn't exist in cygwin). Hmm. The diffs of 0001 are really hard to read. Do you know why this is happening? Is that because some code has been moved around? I have been doing a comparison of all the routines showing up in the diffs, to note that the contents of load_hba(), load_ident(), hba_getauthmethod() & friends are actually the same, but this makes the change history harder to follow. Moving around fill_hba_line() and fill_hba_view() should be enough, indeed. +#include "utils/guc.h" +//#include "utils/tuplestore.h" Ditto. + /* Build a tuple descriptor for our result type */ + if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE) + elog(ERROR, "return type must be a row type"); Worth noting that I was planning to apply a patch from Melanie Plageman to simplify the creation of tupledesc and tuplestores for set-returning functions like this one, so this would cut a bit of code here. This is not directly related to your patch, though, that's my business :) Well, as of 0002, one thing that makes things harder to follow is that parse_ident_line() is moved at a different place in hba.c, one slight difference being err_msg to store the error message in the token line.. But shouldn't the extension of parse_ident_line() with its elevel be included in 0001? Or, well, it could just be done in its own patch to make for a cleaner history, so as 0002 could be shaped as two commits itself. Also, it seems to me that we'd better have some TAP tests for that to make sure of its contents? One place would be src/test/auth/. Another place where we make use of user mapping is the krb5 tests but these are run in a limited fashion in the buildfarm. We also set some mappings for SSPI on Windows all the time, so we'd better be careful about that and paint some $windows_os in the tests when looking at the output of the view. +-- We expect no user mapping in this test +select count(*) = 0 as ok from pg_ident_file_mappings; It could be possible to do installcheck on an instance that has user mappings meaning that this had better be ">= 0", no? Does this pass on Windows where pg_regress sets some mappings for SSPI when creating one or more roles? -- Michael signature.asc Description: PGP signature
Re: Add LZ4 compression in pg_dump
On Fri, Feb 25, 2022 at 12:05:31PM +, Georgios wrote:
> The first commit does the heavy lifting required for additional compression
> methods.
> It expands testing coverage for the already supported gzip compression. Commit
> bf9aa490db introduced cfp in compress_io.{c,h} with the intent of unifying
> compression related code and allow for the introduction of additional archive
> formats. However, pg_backup_archiver.c was not using that API. This commit
> teaches pg_backup_archiver.c about cfp and is using it through out.
Thanks for the patch. I have a few high-level comments.
+ # Do not use --no-sync to give test coverage for data sync.
+ compression_gzip_directory_format => {
+ test_key => 'compression',
The tests for GZIP had better be split into their own commit, as
that's a coverage improvement for the existing code.
I was assuming that this was going to be much larger :)
+/* Routines that support LZ4 compressed data I/O */
+#ifdef HAVE_LIBLZ4
+static void InitCompressorLZ4(CompressorState *cs);
+static void ReadDataFromArchiveLZ4(ArchiveHandle *AH, ReadFunc
readF);
+static void WriteDataToArchiveLZ4(ArchiveHandle *AH, CompressorState *cs,
+ const char *data, size_t dLen);
+static void EndCompressorLZ4(ArchiveHandle *AH, CompressorState *cs);
+#endif
Hmm. This is the same set of APIs as ZLIB and NONE to init, read,
write and end, but for the LZ4 compressor (NONE has no init/end).
Wouldn't it be better to refactor the existing pg_dump code to have a
central structure holding all the function definitions in a common
structure so as all those function signatures are set in stone in the
shape of a catalog of callbacks, making the addition of more
compression formats easier? I would imagine that we'd split the code
of each compression method into their own file with their own context
data. This would lead to a removal of compress_io.c, with its entry
points ReadDataFromArchive(), WriteDataToArchive() & co replaced by
pointers to each per-compression callback.
> Furthermore, compression was chosen based on the value of the level passed
> as an argument during the invocation of pg_dump or some hardcoded defaults.
> This
> does not scale for more than one compression methods. Now the method used for
> compression can be explicitly requested during command invocation, or set
> during
> hardcoded defaults. Then it is stored in the relevant structs and passed in
> the
> relevant functions, along side compression level which has lost it's special
> meaning. The method for compression is not yet stored in the actual archive.
> This is done in the next commit which does introduce a new method.
That's one thing Robert was arguing about with pg_basebackup, so that
would be consistent, and the option set is backward-compatible as far
as I get it by reading the code.
--
Michael
signature.asc
Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
On Tue, Mar 01, 2022 at 05:19:50PM +0800, Julien Rouhaud wrote: > On Tue, Mar 01, 2022 at 04:45:48PM +0900, Michael Paquier wrote: >> Hmm. The diffs of 0001 are really hard to read. Do you know why this >> is happening? Is that because some code has been moved around? > > Yes, I followed the file convention to put the static functions first and then > the exposed functions, and git-diff makes a terrible mess out of it :( I'd like to think that not doing such a thing would be more helpful in this case. As the diffs show, anyone is going to have a hard time to figure out if there are any differences in any of those routines, and if these are the origin of a different problem. A second thing is that this is going to make back-patching unnecessarily harder. > There's no functional change apart from exposing some functions and moving > some > in another file, so I though it's still ok to keep some consistency. There > isn't much changes backpatched in that file, so it shouldn't create more > maintenance burden than simply splitting the file. A lot of files do that already. History clarity matters most IMO. > As I mentioned in my initial email, I intentionally didn't add any test in the > patchset yet, except the exact same coverage for the new view as there's for > pg_hba_file_rules. Ideally I'd like to add tests only once, to cover both 002 > and 0003. But I don't want to waste time for that right now, especially since > no one seems to be interested in 0003. But that would be helpful for 0002. I think that we should have a bit more coverage in this area. pg_hba_file_rules could gain in coverage, additionally, but this is unrelated to what you are proposing here.. >> Does this pass >> on Windows where pg_regress sets some mappings for SSPI when creating >> one or more roles? > > According to CI and cfbot yes. E.g. > https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/37/3558. > Note that the failed runs are the warning I mentioned for mingw32 and the POC > 0004, which is now fixed. Interesting, I would not have expected that. I may poke at that more seriously. -- Michael signature.asc Description: PGP signature
Re: make tuplestore helper function
On Mon, Feb 28, 2022 at 04:49:41PM +0900, Michael Paquier wrote:
> In order to keep things pluggable at will, MakeFuncResultTuplestore()
> has been changed to access a set of bits32 flags, able to control the
> two options above. With this facility in place, I have been able to
> cut much more code than the initial patch, roughly twice as of:
> 24 files changed, 157 insertions(+), 893 deletions(-)
So, I have been thinking about this patch once again, and after
pondering more on it, MakeFuncResultTuplestore() is actually a wrong
name now that it does much more than just creating a tuplestore, by
assigning the TupleDesc and the TupleStore into the function's
ReturnSetInfo.
This is actually setting up a function in the context of a single call
where we fill the tuplestore with all its values, so instead I have
settled down to name that SetSingleFuncCall(), to make a parallel with
the existing MultiFuncCall*(). funcapi.c is the right place for
that, and I have added more documentation in the fmgr's README as well
as funcapi.h.
--
Michael
From 5777ec80ee45fb2975a7e61224734892bbd3503e Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Thu, 24 Feb 2022 17:27:55 +0900
Subject: [PATCH v10 1/2] Introduce MakeFuncResultTuplestore()
This is the main patch from Melanie, that I have tweaked a bit. Note
that I am not completely done with it ;p
---
src/include/funcapi.h | 2 +
src/backend/access/transam/xlogfuncs.c| 16 +---
src/backend/commands/event_trigger.c | 32 +---
src/backend/commands/extension.c | 48 +---
src/backend/commands/prepare.c| 17 +
src/backend/foreign/foreign.c | 14 +---
src/backend/libpq/hba.c | 19 +
src/backend/replication/logical/launcher.c| 16 +---
.../replication/logical/logicalfuncs.c| 16 +---
src/backend/replication/logical/origin.c | 19 +
src/backend/replication/slotfuncs.c | 16 +---
src/backend/replication/walsender.c | 16 +---
src/backend/storage/ipc/shmem.c | 16 +---
src/backend/utils/adt/datetime.c | 17 +
src/backend/utils/adt/genfile.c | 31 +---
src/backend/utils/adt/jsonfuncs.c | 73 +++
src/backend/utils/adt/mcxtfuncs.c | 16 +---
src/backend/utils/adt/misc.c | 14 +---
src/backend/utils/adt/pgstatfuncs.c | 48 +---
src/backend/utils/adt/varlena.c | 14 +---
src/backend/utils/fmgr/funcapi.c | 38 ++
src/backend/utils/misc/guc.c | 15 +---
src/backend/utils/misc/pg_config.c| 13 +---
src/backend/utils/mmgr/portalmem.c| 17 +
24 files changed, 82 insertions(+), 461 deletions(-)
diff --git a/src/include/funcapi.h b/src/include/funcapi.h
index ba927c2f33..b9f9e92d1a 100644
--- a/src/include/funcapi.h
+++ b/src/include/funcapi.h
@@ -229,6 +229,8 @@ extern TupleDesc BlessTupleDesc(TupleDesc tupdesc);
extern AttInMetadata *TupleDescGetAttInMetadata(TupleDesc tupdesc);
extern HeapTuple BuildTupleFromCStrings(AttInMetadata *attinmeta, char **values);
extern Datum HeapTupleHeaderGetDatum(HeapTupleHeader tuple);
+extern Tuplestorestate *MakeFuncResultTuplestore(FunctionCallInfo fcinfo,
+ TupleDesc *result_tupdesc);
/*--
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index 12e2bf4135..2fc1ed023c 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -178,24 +178,10 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS)
XLogRecPtr stoppoint;
SessionBackupState status = get_backup_status();
- /* check to see if caller supports us returning a tuplestore */
- if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo))
- ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("set-valued function called in context that cannot accept a set")));
- if (!(rsinfo->allowedModes & SFRM_Materialize))
- ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("materialize mode required, but it is not allowed in this context")));
-
- /* Build a tuple descriptor for our result type */
- if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
- elog(ERROR, "return type must be a row type");
-
per_query_ctx = rsinfo->econtext->ecxt_per_query_memory;
oldcontext = MemoryContextSwitchTo(per_query_ctx);
- tupstore = tuplestore_begin_heap(true, false, work_mem);
+ tupstore = MakeFuncResultTuplestore(fcinfo, &tupdesc);
rsinfo->returnMode = SFRM_Materialize;
rsinfo->setResult = tupstore;
rsinfo->setDesc = tupdesc;
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index 1e8587502e..68cad0a580 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/back
Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
On Wed, Feb 16, 2022 at 01:58:10PM +0900, Michael Paquier wrote: > I have been looking at how much simplicity this brings, and I have to > admit that it is tempting to just support the loading of dumps when > setting up the old instance to upgrade from. We'd still need to do an > extra effort in terms of cleaning up the diffs for the dump of the old > instance with older versions once/if this is plugged into the > buildfarm, but that could be addressed later depending on the versions > that need to be covered. The bug related to the detection of Windows and temporary paths for pg_upgrade's server.c has been fixed as of dc57366, so attached is the remaining rebased piece as perl2host has been recently removed. Do others have an opinion about a backpatch of the bugfix? Nobody has complained about that since pg_upgrade exists, so I have just done the change on HEAD. -- Michael From 66b6961866d215344aa836963e5bdbfb237ed606 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Wed, 2 Mar 2022 15:55:32 +0900 Subject: [PATCH v10] Switch tests of pg_upgrade to use TAP --- src/bin/pg_upgrade/Makefile| 21 +- src/bin/pg_upgrade/TESTING | 85 ++-- src/bin/pg_upgrade/t/001_basic.pl | 9 + src/bin/pg_upgrade/t/002_pg_upgrade.pl | 251 ++ src/bin/pg_upgrade/test.sh | 279 - src/tools/msvc/vcregress.pl| 92 +--- 6 files changed, 290 insertions(+), 447 deletions(-) create mode 100644 src/bin/pg_upgrade/t/001_basic.pl create mode 100644 src/bin/pg_upgrade/t/002_pg_upgrade.pl delete mode 100644 src/bin/pg_upgrade/test.sh diff --git a/src/bin/pg_upgrade/Makefile b/src/bin/pg_upgrade/Makefile index 49b94f0ac7..1f5d757548 100644 --- a/src/bin/pg_upgrade/Makefile +++ b/src/bin/pg_upgrade/Makefile @@ -28,6 +28,10 @@ OBJS = \ override CPPFLAGS := -DDLSUFFIX=\"$(DLSUFFIX)\" -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS) LDFLAGS_INTERNAL += -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport) +# required for 002_pg_upgrade.pl +REGRESS_SHLIB=$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX) +export REGRESS_SHLIB + all: pg_upgrade pg_upgrade: $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils @@ -47,17 +51,8 @@ clean distclean maintainer-clean: rm -rf delete_old_cluster.sh log/ tmp_check/ \ reindex_hash.sql -# When $(MAKE) is present, make automatically infers that this is a -# recursive make. which is not actually what we want here, as that -# e.g. prevents output synchronization from working (as make thinks -# that the subsidiary make knows how to deal with that itself, but -# we're invoking a shell script that doesn't know). Referencing -# $(MAKE) indirectly avoids that behaviour. -# See https://www.gnu.org/software/make/manual/html_node/MAKE-Variable.html#MAKE-Variable -NOTSUBMAKEMAKE=$(MAKE) +check: + $(prove_check) -check: test.sh all temp-install - MAKE=$(NOTSUBMAKEMAKE) $(with_temp_install) bindir=$(abs_top_builddir)/tmp_install/$(bindir) EXTRA_REGRESS_OPTS="$(EXTRA_REGRESS_OPTS)" $(SHELL) $< - -# installcheck is not supported because there's no meaningful way to test -# pg_upgrade against a single already-running server +installcheck: + $(prove_installcheck) diff --git a/src/bin/pg_upgrade/TESTING b/src/bin/pg_upgrade/TESTING index 78b9747908..3718483a1c 100644 --- a/src/bin/pg_upgrade/TESTING +++ b/src/bin/pg_upgrade/TESTING @@ -2,78 +2,23 @@ THE SHORT VERSION - On non-Windows machines, you can execute the testing process -described below by running +described below by running the following command in this directory: make check -in this directory. This will run the shell script test.sh, performing -an upgrade from the version in this source tree to a new instance of -the same version. -To test an upgrade from a different version, you must have a built -source tree for the old version as well as this version, and you -must have done "make install" for both versions. Then do: +This will run the TAP tests to run pg_upgrade, performing an upgrade +from the version in this source tree to a new instance of the same +version. -export oldsrc=...somewhere/postgresql (old version's source tree) -export oldbindir=...otherversion/bin (old version's installed bin dir) -export bindir=...thisversion/bin (this version's installed bin dir) -export libdir=...thisversion/lib (this version's installed lib dir) -sh test.sh +Testing an upgrade from a different version requires a dump to set up +the contents of this instance, with its set of binaries. The following +variables are available to control the test: +export olddump=...somewhere/dump.sql (old version's dump) +export oldinstall=...otherversion/ (old version's install base path) -In this case, you will have to manually eyeball the resulting dump -diff for version-specific differences,
Re: [PATCH] Expose port->authn_id to extensions and triggers
On Tue, Mar 01, 2022 at 10:03:20PM +, Jacob Champion wrote: > Added a first draft in v5, alongside the perltidy fixups mentioned by > Michael. +The authenticated identity is an immutable identifier for the user +presented during the connection handshake; the exact format depends on +the authentication method in use. (For example, when using the +scram-sha-256 auth method, the authenticated identity +is simply the username. When using the cert auth +method, the authenticated identity is the Distinguished Name of the +client certificate.) Even for auth methods which use the username as +the authenticated identity, this function differs from +session_user in that its return value cannot be +changed after login. That looks enough seen from here. Thanks! Nit: "auth method" would be a first in the documentation, so this had better be "authentication method". (No need to send an updated patch just for that). So, any comments and/or opinions from others? -- Michael signature.asc Description: PGP signature
Re: pg_stat_get_replication_slot and pg_stat_get_subscription_worker incorrectly marked as proretset
On Wed, Mar 02, 2022 at 07:42:50AM +0530, Amit Kapila wrote: > This is done as part of commit: > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=7a85073290856554416353a89799a4c04d09b74b Thanks for taking care of it! -- Michael signature.asc Description: PGP signature
pg_stop_backup() v2 incorrectly marked as proretset
Hi all,
In my hunt looking for incorrect SRFs, I have noticed a new case of a
system function marked as proretset while it builds and returns only
one record. And this is a popular one: pg_stop_backup(), labelled
v2.
This leads to a lot of unnecessary work, as the function creates a
tuplestore it has no need for with the usual set of checks related to
SRFs. The logic can be be simplified as of the attached.
Thoughts?
--
Michael
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index bf88858171..d8e8715ed1 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6275,9 +6275,9 @@
proname => 'pg_stop_backup', provolatile => 'v', proparallel => 'r',
prorettype => 'pg_lsn', proargtypes => '', prosrc => 'pg_stop_backup' },
{ oid => '2739', descr => 'finish taking an online backup',
- proname => 'pg_stop_backup', prorows => '1', proretset => 't',
- provolatile => 'v', proparallel => 'r', prorettype => 'record',
- proargtypes => 'bool bool', proallargtypes => '{bool,bool,pg_lsn,text,text}',
+ proname => 'pg_stop_backup', provolatile => 'v', proparallel => 'r',
+ prorettype => 'record', proargtypes => 'bool bool',
+ proallargtypes => '{bool,bool,pg_lsn,text,text}',
proargmodes => '{i,i,o,o,o}',
proargnames => '{exclusive,wait_for_archive,lsn,labelfile,spcmapfile}',
prosrc => 'pg_stop_backup_v2' },
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index 12e2bf4135..2f292e2bd8 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -165,11 +165,9 @@ pg_stop_backup(PG_FUNCTION_ARGS)
Datum
pg_stop_backup_v2(PG_FUNCTION_ARGS)
{
+#define PG_STOP_BACKUP_V2_COLS 3
ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
TupleDesc tupdesc;
- Tuplestorestate *tupstore;
- MemoryContext per_query_ctx;
- MemoryContext oldcontext;
Datum values[3];
bool nulls[3];
@@ -178,29 +176,15 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS)
XLogRecPtr stoppoint;
SessionBackupState status = get_backup_status();
- /* check to see if caller supports us returning a tuplestore */
- if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo))
- ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("set-valued function called in context that cannot accept a set")));
- if (!(rsinfo->allowedModes & SFRM_Materialize))
- ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("materialize mode required, but it is not allowed in this context")));
-
- /* Build a tuple descriptor for our result type */
- if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
- elog(ERROR, "return type must be a row type");
-
- per_query_ctx = rsinfo->econtext->ecxt_per_query_memory;
- oldcontext = MemoryContextSwitchTo(per_query_ctx);
-
- tupstore = tuplestore_begin_heap(true, false, work_mem);
- rsinfo->returnMode = SFRM_Materialize;
- rsinfo->setResult = tupstore;
- rsinfo->setDesc = tupdesc;
-
- MemoryContextSwitchTo(oldcontext);
+ /* Initialise attributes information in the tuple descriptor */
+ tupdesc = CreateTemplateTupleDesc(PG_STOP_BACKUP_V2_COLS);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 1, "lsn",
+ PG_LSNOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 2, "labelfile",
+ TEXTOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 3, "spcmapfile",
+ TEXTOID, -1, 0);
+ BlessTupleDesc(tupdesc);
MemSet(values, 0, sizeof(values));
MemSet(nulls, 0, sizeof(nulls));
@@ -251,9 +235,8 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS)
/* Stoppoint is included on both exclusive and nonexclusive backups */
values[0] = LSNGetDatum(stoppoint);
- tuplestore_putvalues(tupstore, tupdesc, values, nulls);
-
- return (Datum) 0;
+ /* Returns the record as Datum */
+ PG_RETURN_DATUM(HeapTupleGetDatum(heap_form_tuple(tupdesc, values, nulls)));
}
/*
diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql
index 758ab6e25a..81bac6f581 100644
--- a/src/backend/catalog/system_functions.sql
+++ b/src/backend/catalog/system_functions.sql
@@ -384,7 +384,7 @@ CREATE OR REPLACE FUNCTION
CREATE OR REPLACE FUNCTION pg_stop_backup (
exclusive boolean, wait_for_archive boolean DEFAULT true,
OUT lsn pg_lsn, OUT labelfile text, OUT spcmapfile text)
- RETURNS SETOF record STRICT VOLATILE LANGUAGE internal as 'pg_stop_backup_v2'
+ RETURNS record STRICT VOLATILE LANGUAGE internal as 'pg_stop_backup_v2'
PARALLEL RESTRICTED;
CREATE OR REPLACE FUNCTION
signature.asc
Description: PGP signature
Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
On Wed, Mar 02, 2022 at 12:01:17AM -0800, Andres Freund wrote: > But in a bad way, because EXTRA_REGRESS_OPTS now always wins, even for stuff > we want to override. Note how test.sh explicitly specifies port, bindir etc > after the pre-existing EXTRA_REGRESS_OPTS. Ah, right. Will fix. -- Michael signature.asc Description: PGP signature
Re: pg_stop_backup() v2 incorrectly marked as proretset
On Wed, Mar 02, 2022 at 05:22:35PM +0900, Kyotaro Horiguchi wrote:
> But the patch forgets to remove an useless variable.
Indeed. I forgot to look at stderr.
>> /* Initialise attributes information in the tuple descriptor */
>> tupdesc = CreateTemplateTupleDesc(PG_STOP_BACKUP_V2_COLS);
>> TupleDescInitEntry(tupdesc, (AttrNumber) 1, "lsn",
>> PG_LSNOID, -1, 0);
>
> I think we can use get_call_resuilt_type here.
Yes, I don't mind doing so here.
--
Michael
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index bf88858171..d8e8715ed1 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6275,9 +6275,9 @@
proname => 'pg_stop_backup', provolatile => 'v', proparallel => 'r',
prorettype => 'pg_lsn', proargtypes => '', prosrc => 'pg_stop_backup' },
{ oid => '2739', descr => 'finish taking an online backup',
- proname => 'pg_stop_backup', prorows => '1', proretset => 't',
- provolatile => 'v', proparallel => 'r', prorettype => 'record',
- proargtypes => 'bool bool', proallargtypes => '{bool,bool,pg_lsn,text,text}',
+ proname => 'pg_stop_backup', provolatile => 'v', proparallel => 'r',
+ prorettype => 'record', proargtypes => 'bool bool',
+ proallargtypes => '{bool,bool,pg_lsn,text,text}',
proargmodes => '{i,i,o,o,o}',
proargnames => '{exclusive,wait_for_archive,lsn,labelfile,spcmapfile}',
prosrc => 'pg_stop_backup_v2' },
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index 12e2bf4135..9f7a282ed2 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -165,43 +165,20 @@ pg_stop_backup(PG_FUNCTION_ARGS)
Datum
pg_stop_backup_v2(PG_FUNCTION_ARGS)
{
- ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
+#define PG_STOP_BACKUP_V2_COLS 3
TupleDesc tupdesc;
- Tuplestorestate *tupstore;
- MemoryContext per_query_ctx;
- MemoryContext oldcontext;
- Datum values[3];
- bool nulls[3];
+ Datum values[PG_STOP_BACKUP_V2_COLS];
+ bool nulls[PG_STOP_BACKUP_V2_COLS];
bool exclusive = PG_GETARG_BOOL(0);
bool waitforarchive = PG_GETARG_BOOL(1);
XLogRecPtr stoppoint;
SessionBackupState status = get_backup_status();
- /* check to see if caller supports us returning a tuplestore */
- if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo))
- ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("set-valued function called in context that cannot accept a set")));
- if (!(rsinfo->allowedModes & SFRM_Materialize))
- ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("materialize mode required, but it is not allowed in this context")));
-
- /* Build a tuple descriptor for our result type */
+ /* Initialise attributes information in the tuple descriptor */
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
- per_query_ctx = rsinfo->econtext->ecxt_per_query_memory;
- oldcontext = MemoryContextSwitchTo(per_query_ctx);
-
- tupstore = tuplestore_begin_heap(true, false, work_mem);
- rsinfo->returnMode = SFRM_Materialize;
- rsinfo->setResult = tupstore;
- rsinfo->setDesc = tupdesc;
-
- MemoryContextSwitchTo(oldcontext);
-
MemSet(values, 0, sizeof(values));
MemSet(nulls, 0, sizeof(nulls));
@@ -251,9 +228,8 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS)
/* Stoppoint is included on both exclusive and nonexclusive backups */
values[0] = LSNGetDatum(stoppoint);
- tuplestore_putvalues(tupstore, tupdesc, values, nulls);
-
- return (Datum) 0;
+ /* Returns the record as Datum */
+ PG_RETURN_DATUM(HeapTupleGetDatum(heap_form_tuple(tupdesc, values, nulls)));
}
/*
diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql
index 758ab6e25a..81bac6f581 100644
--- a/src/backend/catalog/system_functions.sql
+++ b/src/backend/catalog/system_functions.sql
@@ -384,7 +384,7 @@ CREATE OR REPLACE FUNCTION
CREATE OR REPLACE FUNCTION pg_stop_backup (
exclusive boolean, wait_for_archive boolean DEFAULT true,
OUT lsn pg_lsn, OUT labelfile text, OUT spcmapfile text)
- RETURNS SETOF record STRICT VOLATILE LANGUAGE internal as 'pg_stop_backup_v2'
+ RETURNS record STRICT VOLATILE LANGUAGE internal as 'pg_stop_backup_v2'
PARALLEL RESTRICTED;
CREATE OR REPLACE FUNCTION
signature.asc
Description: PGP signature
Re: pg_stop_backup() v2 incorrectly marked as proretset
On Thu, Mar 03, 2022 at 12:36:32AM +0800, Julien Rouhaud wrote: > I don't see strong evidence for that pattern being wildly used with some naive > grepping: Yes, I don't recall either seeing the style with an undef a lot when it came to system functions. I'll move on and apply the fix in a minute using this style. -- Michael signature.asc Description: PGP signature
Re: pg_stop_backup() v2 incorrectly marked as proretset
On Wed, Mar 02, 2022 at 12:04:59PM -0500, Chapman Flack wrote: > I had just recently noticed that while reviewing [0], but shrugged, > as I didn't know what the history was. Okay. I did not see you mention it on the thread, but the discussion is long so it is easy to miss some of its details. > Is this best handled as a separate patch, or folded into [0], which is > going to be altering and renaming that function anyway? No idea where this is leading, but I'd rather fix what is at hands now rather than assuming that something may or may not happen. If, as you say, this code gets removed, rebasing this conflict is just a matter of removing the existing code again so that's trivial. -- Michael signature.asc Description: PGP signature
Re: Add CHECKPOINT_REQUESTED flag to the log message in LogCheckpointStart()
On Thu, Mar 03, 2022 at 09:39:37AM +0900, Kyotaro Horiguchi wrote: > At Wed, 2 Mar 2022 18:18:10 +0530, Bharath Rupireddy > wrote in >> I don't think that's useful. Being in LogCheckpointStart >> (CreateCheckPoint or CreateRestartPoint) itself means that somebody >> has requested a checkpoint. Having CHECKPOINT_REQUESTED doesn't add >> any value. > > Agreed. Exactly my impression. This would apply now to the WAL shutdown code paths, and I'd suspect that the callers of CreateCheckPoint() are not going to increase soon. The point is: the logs already provide some contexts for any of those callers so I see no need for this additional information. > Actually no one does but RequestCheckpoint() accepts 0 as flags. > Checkpointer would be a bit more complex without CHECKPOINT_REQUESTED. > I don't think it does us any good to get rid of the flag value. I'd rather keep this code as-is. -- Michael signature.asc Description: PGP signature
Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
On Wed, Mar 02, 2022 at 12:07:29AM -0800, Andres Freund wrote:
>> +++ b/src/bin/pg_upgrade/t/001_basic.pl
>> @@ -0,0 +1,9 @@
>> +use strict;
>> +use warnings;
>> +
>> +use PostgreSQL::Test::Utils;
>> +use Test::More tests => 8;
>
> Outdated.
Fixed.
>> +program_help_ok('pg_upgrade');
>> +program_version_ok('pg_upgrade');
>> +program_options_handling_ok('pg_upgrade');
>
> Unrelated. But I kinda wish we'd do this in a saner manner than copying this
> test into every binary. E.g. by ensuring that all tools installed in the temp
> install are tested or such.
Perhaps. I am sticking with the existing style for now.
>> +# The test of pg_upgrade consists in setting up an instance. This is the
>> +# source instance used for the upgrade. Then a new and fresh instance is
>> +# created, and is used as the target instance for the upgrade.
>
> This seems a bit repetitive. Lots of "instance".
Indeed. I have reworked the whole, rather than just those three
sentences.
>> +if ( (defined($ENV{olddump}) && !defined($ENV{oldinstall}))
>> +|| (!defined($ENV{olddump}) && defined($ENV{oldinstall})))
>
> Odd indentation. Spaces between parens?
Well, perltidy tells me that this is right.
>> +$newnode->init(extra => [ '--wal-segsize', '1', '--allow-group-access' ]);
>
> I'd copy the comments from test.sh wrt --wal-segsize,
> --allow-group-access.
Done.
--
Michael
From ba2cf7a7adf9cf50406d8bbcaa12167137ba29c3 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Thu, 3 Mar 2022 14:01:56 +0900
Subject: [PATCH v11] Switch tests of pg_upgrade to use TAP
---
src/bin/pg_upgrade/Makefile| 21 +-
src/bin/pg_upgrade/TESTING | 85 ++--
src/bin/pg_upgrade/t/001_basic.pl | 11 +
src/bin/pg_upgrade/t/002_pg_upgrade.pl | 248 ++
src/bin/pg_upgrade/test.sh | 279 -
src/tools/msvc/vcregress.pl| 92 +---
6 files changed, 289 insertions(+), 447 deletions(-)
create mode 100644 src/bin/pg_upgrade/t/001_basic.pl
create mode 100644 src/bin/pg_upgrade/t/002_pg_upgrade.pl
delete mode 100644 src/bin/pg_upgrade/test.sh
diff --git a/src/bin/pg_upgrade/Makefile b/src/bin/pg_upgrade/Makefile
index 49b94f0ac7..1f5d757548 100644
--- a/src/bin/pg_upgrade/Makefile
+++ b/src/bin/pg_upgrade/Makefile
@@ -28,6 +28,10 @@ OBJS = \
override CPPFLAGS := -DDLSUFFIX=\"$(DLSUFFIX)\" -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
LDFLAGS_INTERNAL += -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport)
+# required for 002_pg_upgrade.pl
+REGRESS_SHLIB=$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX)
+export REGRESS_SHLIB
+
all: pg_upgrade
pg_upgrade: $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils
@@ -47,17 +51,8 @@ clean distclean maintainer-clean:
rm -rf delete_old_cluster.sh log/ tmp_check/ \
reindex_hash.sql
-# When $(MAKE) is present, make automatically infers that this is a
-# recursive make. which is not actually what we want here, as that
-# e.g. prevents output synchronization from working (as make thinks
-# that the subsidiary make knows how to deal with that itself, but
-# we're invoking a shell script that doesn't know). Referencing
-# $(MAKE) indirectly avoids that behaviour.
-# See https://www.gnu.org/software/make/manual/html_node/MAKE-Variable.html#MAKE-Variable
-NOTSUBMAKEMAKE=$(MAKE)
+check:
+ $(prove_check)
-check: test.sh all temp-install
- MAKE=$(NOTSUBMAKEMAKE) $(with_temp_install) bindir=$(abs_top_builddir)/tmp_install/$(bindir) EXTRA_REGRESS_OPTS="$(EXTRA_REGRESS_OPTS)" $(SHELL) $<
-
-# installcheck is not supported because there's no meaningful way to test
-# pg_upgrade against a single already-running server
+installcheck:
+ $(prove_installcheck)
diff --git a/src/bin/pg_upgrade/TESTING b/src/bin/pg_upgrade/TESTING
index 78b9747908..3718483a1c 100644
--- a/src/bin/pg_upgrade/TESTING
+++ b/src/bin/pg_upgrade/TESTING
@@ -2,78 +2,23 @@ THE SHORT VERSION
-
On non-Windows machines, you can execute the testing process
-described below by running
+described below by running the following command in this directory:
make check
-in this directory. This will run the shell script test.sh, performing
-an upgrade from the version in this source tree to a new instance of
-the same version.
-To test an upgrade from a different version, you must have a built
-source tree for the old version as well as this version, and you
-must have done "make install" for both versions. Then do:
+This will run the TAP tests to run pg_upgrade, performing an upgrade
+from the version in this source tree to a new instance of the same
+version.
-export oldsrc=...somewhere/postgresql (old version&#x
Re: Proposal: Support custom authentication methods using hooks
On Wed, Mar 02, 2022 at 11:15:28AM -0500, Stephen Frost wrote: > With... which? We removed recovery.conf without any warning between > major releases, yet it was used by every single PG file-based backup and > restore solution out there and by every single organization that had > ever done a restore of PG since it was introduced in 8.0. There was much more to the changes around recovery.conf, with the integration of its parameters as GUCs so it had a lot of benefits, and that's why it has baked for 3~4 years as far as I recall. There is SCRAM as a replacement of MD5 already in core, but as Jonathan said upthread the upgrade from an instance that still has MD5 hashes may finish by being tricky for some users because this does not concern only pg_upgrade (this could fail if it detects MD5 hashes in pg_authid), and we don't really know how to solve the pg_dump bit, do we? And it seems to me that this is not a matter of just blocking the load of MD5 hashes in the backend in the case of logical dumps. > Passing > around cleartext passwords with the LDAP authentication method is > clearly bad from a security perspective and it's a bunch of code to > support that, along with it being quite complicated to configure and get > set up (arguably harder than Kerberos, if you want my 2c). If you want > to say that it's valuable for us to continue to maintain that code > because it's good and useful and might even be the only option for some > people, fine, though I disagree, but I don't think my argument that we > shouldn't keep it just because *someone* is using it is any different > from our general project policy about features. MD5 is still used at auth method by many people, as is LDAP. They have their design flaws (backend LDAP, MD5 hashes in old backups), but those code areas are pretty mature as well, so a simple removal could hurt the user base of PG quite a lot IMO. -- Michael signature.asc Description: PGP signature
Re: [PATCH] Expose port->authn_id to extensions and triggers
On Wed, Mar 02, 2022 at 01:27:40PM -0800, Andres Freund wrote: > I don't think we should commit this without synchronizing the authn between > worker / leader (in a separate commit). Too likely that some function that's > marked parallel ok queries the authn_id, opening up a security/monitoring hole > or such because of a bogus return value. Hmm, OK. Using the same authn ID for the leader and the workers still looks a bit strange to me as the worker is not the one that does the authentication, only the leader does that. Anyway, FixedParallelState includes some authentication data passed down by the leader when spawning a worker. So, if we were to pass down the authn, we are going to need a new PARALLEL_KEY_* to serialize and restore the data passed down via a DSM like any other states as per the business in parallel.c. Jacob, what do you think? -- Michael signature.asc Description: PGP signature
Re: pg_stop_backup() v2 incorrectly marked as proretset
On Thu, Mar 03, 2022 at 04:40:42PM -0500, Tom Lane wrote: > The point is to make it clear that the macro isn't intended to affect > code outside the function. Since C lacks block-scoped macros, > there's no other way to do that. > > I concede that a lot of our code is pretty sloppy about this, but > that doesn't make it a good practice. Well, if we change that, better to do that in all the places where this would be affected, but I am not sure to see a style appealing enough on this thread. From what I can see, history shows that the style of using a #define for the number of columns originates from da2c1b8, aka 9.0. Its use inside a function originates from a755ea3 as of 9.1 and then it has just spread around without any undefs, so it looks like people like that way of doing things. -- Michael signature.asc Description: PGP signature
Re: [PATCH] Expose port->authn_id to extensions and triggers
On Thu, Mar 03, 2022 at 07:16:17PM +, Jacob Champion wrote: > I guess it depends on what we want MyProcPort to look like in a > parallel worker. Are we comfortable having most of it be blank/useless? > Does it need to be filled in? Good question. It depends on the definition of how much authentication information makes sense for the parallel workers to inherit from the parent. And as I mentioned upthread, this definition is not completely clear to me because the parallel workers do *not* go through the authentication paths of the parent, they are just spawned in their own dedicated paths that the leader invokes. Inheriting all this information from the leader has also an impact on the PgBackendStatus entries of the workers as these are reported in pg_stat_activity as far as I recall, and it could be confusing to see, for example, some SSL or some GSS information for automatically spawned processes because these don't use SSL or GSS when they pass back data to the leader. I have been looking at the commit history, and found about 6b7d11f that switched all the functions of sslinfo to be parallel-restricted especially because of this. So if we inherit all this information the restriction on the sslinfo functions could be lifted, though the interest is honestly limited in this case. postgres_fdw has introduced recently the concept of cached connections, as of v14 with 411ae64 and 708d165, with a set of parallel-restricted functions. Some of the code paths related to appname look at MyProcPort, so there could be a risk of having some inconsistent information if this is accessed in a parallel worker. Looking at the code, I don't think that it would happen now but copying some of the data of MyProcPort to the worker could avoid any future issues if this code gets extended. At the end of the day, Port is an interface used for the communication between the postmaster with the frontends, so I'd like to say that it is correct to not apply this concept to parallel workers because they are not designed to contact any frontend-side things. -- Michael signature.asc Description: PGP signature
Re: standby recovery fails (tablespace related) (tentative patch and discussion)
On Fri, Mar 04, 2022 at 09:10:48AM +0900, Kyotaro Horiguchi wrote: > And same function contained a maybe-should-have-been-removed line > which makes Windows build unhappy. > > This should make all platforms in the CI happy. d6d317d as solved the issue of tablespace paths across multiple nodes with the new GUC called allow_in_place_tablespaces, and is getting successfully used in the recovery tests as of 027_stream_regress.pl. Shouldn't we rely on that rather than extending more our test perl modules? One tricky part is the emulation of readlink for junction points on Windows (dir_readlink in your patch), and the root of the problem is that 0003 cares about the path structure of the tablespaces so we have no need, as far as I can see, for any dependency with link follow-up in the scope of this patch. This means that you should be able to simplify the patch set, as we could entirely drop 0001 in favor of enforcing the new dev GUC in the nodes created in the TAP test of 0002. Speaking of 0002, perhaps this had better be in its own file rather than extending more 011_crash_recovery.pl. 0003 looks like a good idea to check after the consistency of the path structures created during replay, and it touches paths I'd expect it to touch, as of database and tbspace redos. + if (!reachedConsistency) + XLogForgetMissingDir(xlrec->ts_id, InvalidOid); + + XLogFlush(record->EndRecPtr); Not sure to understand why this is required. A comment may be in order to explain the hows and the whys. -- Michael signature.asc Description: PGP signature
pg_tablespace_location() failure with allow_in_place_tablespaces
Hi all,
While playing with tablespaces and recovery in a TAP test, I have
noticed that retrieving the location of a tablespace created with
allow_in_place_tablespaces enabled fails in pg_tablespace_location(),
because readlink() sees a directory in this case.
The use may be limited to any automated testing and
allow_in_place_tablespaces is a developer GUC, still it seems to me
that there is an argument to allow the case rather than tweak any
tests to hardcode a path with the tablespace OID. And any other code
paths are able to handle such tablespaces, be they in recovery or in
tablespace create/drop.
A junction point is a directory on WIN32 as far as I recall, but
pgreadlink() is here to ensure that we get the correct path on
a source found as pgwin32_is_junction(), so we can rely on that. This
stuff has led me to the attached.
Thoughts?
--
Michael
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index e79eb6b478..59b8f8196c 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -15,6 +15,7 @@
#include "postgres.h"
#include
+#include
#include
#include
#include
@@ -307,8 +308,7 @@ pg_tablespace_location(PG_FUNCTION_ARGS)
{
Oid tablespaceOid = PG_GETARG_OID(0);
char sourcepath[MAXPGPATH];
- char targetpath[MAXPGPATH];
- int rllen;
+ struct stat st;
/*
* It's useful to apply this function to pg_class.reltablespace, wherein
@@ -333,20 +333,57 @@ pg_tablespace_location(PG_FUNCTION_ARGS)
*/
snprintf(sourcepath, sizeof(sourcepath), "pg_tblspc/%u", tablespaceOid);
- rllen = readlink(sourcepath, targetpath, sizeof(targetpath));
- if (rllen < 0)
+ /*
+ * Before reading the link, check if it is a link or a directory.
+ * A directory is possible for a tablespace created with
+ * allow_in_place_tablespaces enabled. On Windows, junction points
+ * are directories, which is why this is checked first.
+ */
+ if (lstat(sourcepath, &st) < 0)
+ {
ereport(ERROR,
(errcode_for_file_access(),
- errmsg("could not read symbolic link \"%s\": %m",
+ errmsg("could not stat file \"%s\": %m",
sourcepath)));
- if (rllen >= sizeof(targetpath))
- ereport(ERROR,
-(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
- errmsg("symbolic link \"%s\" target is too long",
- sourcepath)));
- targetpath[rllen] = '\0';
+ }
+ else if (
+#ifndef WIN32
+ S_ISLNK(st.st_mode)
+#else
+ pgwin32_is_junction(pathbuf)
+#endif
+ )
+ {
+ char targetpath[MAXPGPATH];
+ int rllen;
- PG_RETURN_TEXT_P(cstring_to_text(targetpath));
+ rllen = readlink(sourcepath, targetpath, sizeof(targetpath));
+ if (rllen < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not read symbolic link \"%s\": %m",
+ sourcepath)));
+ if (rllen >= sizeof(targetpath))
+ ereport(ERROR,
+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("symbolic link \"%s\" target is too long",
+ sourcepath)));
+ targetpath[rllen] = '\0';
+
+ PG_RETURN_TEXT_P(cstring_to_text(targetpath));
+ }
+ else if (S_ISDIR(st.st_mode))
+ {
+ /*
+ * For a directory, return the relative path of the source,
+ * as created by allow_in_place_tablespaces. This is useful
+ * for regression tests.
+ */
+ PG_RETURN_TEXT_P(cstring_to_text(sourcepath));
+ }
+ else
+ elog(ERROR, "\"%s\" is not a directory or symbolic link",
+ sourcepath);
#else
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
diff --git a/src/test/regress/expected/tablespace.out b/src/test/regress/expected/tablespace.out
index 2dfbcfdebe..473fe8c28e 100644
--- a/src/test/regress/expected/tablespace.out
+++ b/src/test/regress/expected/tablespace.out
@@ -24,6 +24,15 @@ SELECT spcoptions FROM pg_tablespace WHERE spcname = 'regress_tblspacewith';
DROP TABLESPACE regress_tblspacewith;
-- create a tablespace we can use
CREATE TABLESPACE regress_tblspace LOCATION '';
+-- This returns a relative path as of an effect of allow_in_place_tablespaces,
+-- masking the tablespace OID used in the path name.
+SELECT regexp_replace(pg_tablespace_location(oid), '(pg_tblspc)/(\d+)', '\1/NNN')
+ FROM pg_tablespace WHERE spcname = 'regress_tblspace';
+ regexp_replace
+
+ pg_tblspc/NNN
+(1 row)
+
-- try setting and resetting some properties for the new tablespace
ALTER TABLESPACE regress_tblspace SET (random_page_cost = 1.0, seq_page_cost = 1.1);
ALTER TABLESPACE regress_tblspace SET (some_nonexistent_parameter = true); -- fail
diff --git a/src/test/regress/sql/tablespace.sql b/src/test/regress/sql/tablespace.sql
index 896f05cea3..0949a28488 100644
--- a/src/test/regress/sql/tablespace.sql
+++ b/src/test/regress/sql/tablespace.sql
@@ -22,6 +22,10 @@ DROP TABLESPACE regress_tblspacewith;
-- create a tablespace we can use
CREATE TABLESPACE regress_tblspace LOCATION '';
+-- This returns a relative path as of an effect of allow_in_place_tablespaces,
+-- masking the tablespace OID used in the path name.
+SELECT regexp_replace(pg_tablespace_locatio
Re: wal_compression=zstd
On Tue, Feb 22, 2022 at 05:19:48PM -0600, Justin Pryzby wrote: > I am not claiming that zstd is generally better for WAL. Rather, if we're > going to support alternate compression methods, it's nice to give a couple > options (in addition to pglz). Some use cases would certainly suffer from > slower WAL. But providing the option will benefit some others. Note that a > superuser can set wal_compression for a given session - this would probably > benefit bulk-loading in an otherwise OLTP environment. Well, I cannot say which one is better either as it depends on your workload, mostly, but we know for sure that both have benefits, so I don't mind adding it now. What you are proposing is built on top of the existing code, making it a very simple change. > As writen, this patch uses zstd level=1 (whereas the ZSTD's default compress > level is 6). Why? ZSTD using this default has its reasons, no? And it would be consistent to do the same for ZSTD as for the other two methods. -The supported methods are pglz and -lz4 (if PostgreSQL was -compiled with --with-lz4). The default value is -off. Only superusers can change this setting. +The supported methods are pglz, and +(if configured when PostgreSQLwas built) +lz4 and zstd. +The default value is off. +Only superusers can change this setting. This is making the docs less verbose. I think that you should keep the mention to respectively --with-lz4 and --with-zstd after each value. Build with ZSTD compression support. + This enables use of ZSTD for + compression of WAL data. This addition is not necessary, see d7a9786. Not related to your patch, but we have more reasons to add an check in the block of BKPIMAGE_COMPRESSED() in RestoreBlockImage() of xlogreader.c to make sure that only one bit is set for the compression type. We could use a pg_popcount() == 1 for that, returning report_invalid_record() when we see some corrupted data. As a whole, 0001 looks pretty good to me after a detailed read (not tested, though). > Only 0001 should be reviewed for pg15 - the others are optional/future work. That's wiser for v15. FWIW, I have the impression that we don't need most of what's proposed in 0002~. /* compression methods supported */ -#define BKPIMAGE_COMPRESS_PGLZ 0x04 -#define BKPIMAGE_COMPRESS_LZ4 0x08 -#define BKPIMAGE_COMPRESS_ZSTD 0x10 - +#define BKPIMAGE_COMPRESS_METHOD1 0x04/* bits to encode compression method */ +#define BKPIMAGE_COMPRESS_METHOD2 0x08/* 0=none, 1=pglz, 2=lz4, 3=zstd */ As of 0002. We still have some room for this data and this makes the code harder to follow, so I'd live this part of the code as it is proposed in 0001. 0003, defaulting to zstd, and 0004 to extend compression to support a level would require a lot of benchmarking to be justified. I have argued against making the code more complicated for such things in the past, with reloptions. The footprint on the code is much smaller, here, still.. 0007, for ZLIB, does not make sense once one can choose between LZ4 and ZSTD. -- Michael signature.asc Description: PGP signature
Re: pg_tablespace_location() failure with allow_in_place_tablespaces
On Fri, Mar 04, 2022 at 06:04:00PM +0900, Kyotaro Horiguchi wrote: > And I made a quick hack on do_pg_start_backup. And I found that > pg_basebackup copies in-place tablespaces under the *current > directory*, which is not ok at all:( Yeah, I have noticed that as well while testing such configurations a couple of hours ago, but I am not sure yet how much we need to care about that as in-place tablespaces are included in the main data directory anyway, which would be fine for most test purposes we usually care about. Perhaps this has an impact on the patch posted on the thread that wants to improve the guarantees around tablespace directory structures, but I have not studied this thread much to have an opinion. And it is Friday. -- Michael signature.asc Description: PGP signature
Re: [PoC] Let libpq reject unexpected authentication requests
On Fri, Mar 04, 2022 at 08:19:26PM -0500, Tom Lane wrote: > Jacob Champion writes: >> Here is my take on option 2, then: you get to choose exactly one method >> that the client will accept. If you want to use client certificates, >> use require_auth=cert. If you want to force SCRAM, use >> require_auth=scram-sha-256. If the server asks for something different, >> libpq will fail. If the server tries to get away without asking you for >> authentication, libpq will fail. There is no negotiation. Fine by me to put all the control on the client-side, that makes the whole much simpler to reason about. > Seems reasonable, but I bet that for very little more code you could > accept a comma-separated list of allowed methods; libpq already allows > comma-separated lists for some other connection options. That seems > like it'd be a useful increment of flexibility. Same impression here, so +1 for supporting a comma-separated list of values here. This is already handled in parse_comma_separated_list(), now used for multiple hosts and hostaddrs. -- Michael signature.asc Description: PGP signature
Re: wal_compression=zstd
On Fri, Mar 04, 2022 at 08:08:03AM -0500, Robert Haas wrote: > On Fri, Mar 4, 2022 at 6:44 AM Justin Pryzby wrote: >> In my 1-off test, it gets 610/633 = 96% of the benefit at 209/273 = 77% of >> the >> cost. Hmm, it may be good to start afresh and compile numbers in a single chart. I did that here with some numbers on the user and system CPU: https://www.postgresql.org/message-id/YMmlvyVyAFlxZ+/h...@paquier.xyz For this test, regarding ZSTD, the lowest level did not have much difference with the default level, and at the highest level the user CPU spiked for little gain in compression. All of them compressed more than LZ4, with more CPU used in each case, but the default or a level value lower than the default gives me the impression that it won't matter much in terms of compression gains and CPU usage. > I agree with Michael. Your 1-off test is exactly that, and the results > will have depended on the data you used for the test. I'm not saying > we could never decide to default to a compression level other than the > library's default, but I do not think we should do it casually or as > the result of any small number of tests. There should be a strong > presumption that the authors of the library have a good idea what is > sensible in general unless we can explain compellingly why our use > case is different from typical ones. > > There's an ease-of-use concern here too. It's not going to make things > any easier for users to grok if zstd is available in different parts > of the system but has different defaults in each place. It wouldn't be > the end of the world if that happened, but neither would it be ideal. I'd like to believe that anybody who writes his/her own compression algorithm have a good idea of the default behavior they want to show, so we could remain simple, and believe in them. Now, I would not object to see some fresh numbers, and assuming that all FPIs have the same page size, we could go down to designing a couple of test cases that produce a fixed number of FPIs and measure the compressability in a single session. Repeatability and randomness of data counts, we could have for example one case with a set of 5~7 int attributes, a second with text values that include random data, up to 10~12 bytes each to count on the tuple header to be able to compress some data, and a third with more repeatable data, like one attribute with an int column populate with generate_series(). Just to give an idea. -- Michael signature.asc Description: PGP signature
Re: pl/pgsql feature request: shorthand for argument and local variable references
On Sat, Mar 05, 2022 at 04:54:18PM +0800, Julien Rouhaud wrote: > On Thu, Jan 06, 2022 at 05:05:32PM +0800, Julien Rouhaud wrote: >> Anyway, the only committer that showed some interest in the feature is >> Michael, >> and he seemed ok in principle with the "alias-implementation" approach. >> Michael, did you have a look at this version ([1]), or do you think it should >> simply be rejected? > > After a couple of months I see that there is unfortunately no agreement on > this > patch, or even its need. I got a short look at what was proposed in the patch a couple of months ago, and still found the implementation confusing with the way aliases are handled, particularly when it came to several layers of pl/pgsql. I am fine to let it go for now. -- Michael signature.asc Description: PGP signature
Re: Make unlogged table resets detectable
On Fri, Mar 04, 2022 at 10:12:27AM -0600, Justin Pryzby wrote: > Is this patch targetting pg15 ? > There's no discussion since June. > > Latest at 2021-06-08 21:29:25 by Jeff Davis This is too long, so let's discard this patch for now. -- Michael signature.asc Description: PGP signature
Re: Comment typo in CheckCmdReplicaIdentity
On Mon, Mar 07, 2022 at 09:31:33AM +1100, Peter Smith wrote: > PSA patch to fix a comment typo. > > (The 'OR' should not be uppercase - that keyword is irrelevant here). I was looking at the whole routine, and your suggestion looks like an improvement to me. Will apply if there are no objections. -- Michael signature.asc Description: PGP signature
Re: pl/pgsql feature request: shorthand for argument and local variable references
On Sat, Mar 05, 2022 at 07:31:53PM +0900, Michael Paquier wrote: > I got a short look at what was proposed in the patch a couple of > months ago, and still found the implementation confusing with the way > aliases are handled, particularly when it came to several layers of > pl/pgsql. I am fine to let it go for now. Just had an extra look at the patch, still same impression. So done this way. -- Michael signature.asc Description: PGP signature
Re: make tuplestore helper function
On Wed, Mar 02, 2022 at 03:43:17PM +0900, Michael Paquier wrote: > This is actually setting up a function in the context of a single call > where we fill the tuplestore with all its values, so instead I have > settled down to name that SetSingleFuncCall(), to make a parallel with > the existing MultiFuncCall*(). funcapi.c is the right place for > that, and I have added more documentation in the fmgr's README as well > as funcapi.h. I have tortured all those code paths for the last couple of days, and the new function name, as well as its options, still seemed fine to me, so I have applied the patch. The buildfarm is cool with it. It is worth noting that there are more code paths in contrib/ that could be simplified with this new routine. -- Michael signature.asc Description: PGP signature
Re: pl/pgsql feature request: shorthand for argument and local variable references
On Mon, Mar 07, 2022 at 10:31:40AM +0800, Julien Rouhaud wrote: > I was actually waiting a bit to make sure that Pavel could read the thread, > since it was the weekend and right now it's 3:30 AM in Czech Republic... Sorry about that. I have reset the state of the patch. -- Michael signature.asc Description: PGP signature
Re: wal_compression=zstd
On Fri, Mar 04, 2022 at 08:10:35AM -0500, Andrew Dunstan wrote: > I don't see why patch 5 shouldn't be applied forthwith. Only applying 0005 would result in a failure in the TAP test for a problem whose fix is attempted in 0006. This is an issue unrelated to this thread. FWIW, I am a bit disturbed by EnsureTopTransactionIdLogged() and its design in 0006, where we'd finish by using a XLogFlush() call within two SQL functions, but I have not really looked at the problem to see if it is a viable solution or not. -- Michael signature.asc Description: PGP signature
Re: pg_tablespace_location() failure with allow_in_place_tablespaces
On Fri, Mar 04, 2022 at 03:44:22PM +0900, Michael Paquier wrote: > The use may be limited to any automated testing and > allow_in_place_tablespaces is a developer GUC, still it seems to me > that there is an argument to allow the case rather than tweak any > tests to hardcode a path with the tablespace OID. And any other code > paths are able to handle such tablespaces, be they in recovery or in > tablespace create/drop. > > A junction point is a directory on WIN32 as far as I recall, but > pgreadlink() is here to ensure that we get the correct path on > a source found as pgwin32_is_junction(), so we can rely on that. This > stuff has led me to the attached. Thomas, I'd rather fix this for the sake of the tests. One point is that the function returns a relative path for in-place tablespaces, but it would be easy enough to append a DataDir. What do you think? -- Michael signature.asc Description: PGP signature
Re: pg_tablespace_location() failure with allow_in_place_tablespaces
On Fri, Mar 04, 2022 at 11:26:43PM +1300, Thomas Munro wrote: > The warning from readlink() while making the mapping file isn't ideal, > and perhaps we should suppress that with something like the attached. > Or does the missing map file entry break something on Windows? > @@ -8292,6 +8293,10 @@ do_pg_start_backup(const char *backupidstr, bool fast, > TimeLineID *starttli_p, > > snprintf(fullpath, sizeof(fullpath), "pg_tblspc/%s", de->d_name); > > + /* Skip in-place tablespaces (testing use only) */ > + if (get_dirent_type(fullpath, de, false, ERROR) == PGFILETYPE_DIR) > + continue; I saw the warning when testing base backups with in-place tablespaces and it did not annoy me much, but, yes, that can be confusing. Junction points are directories, no? Are you sure that this works correctly on WIN32? It seems to me that we'd better use readlink() only for entries in pg_tlbspc/ that are PGFILETYPE_LNK on non-WIN32 and pgwin32_is_junction() on WIN32. -- Michael signature.asc Description: PGP signature
Re: make tuplestore helper function
On Mon, Mar 07, 2022 at 05:09:19PM -0500, Melanie Plageman wrote: > I've attached a patch using the helper in most locations in contrib > modules that seemed useful. Thanks for the patch. I was also looking at that yesterday, and this pretty much maps to what I have finished with, except for dblink and xml2 where it was not looking that obvious to me at quick glance. In xml2, my eyes hurt a bit on the meaning of doing the "Switch out of SPI context" in xpath_table() after doing the two SPI calls, but I agree that this extra switch back to the old context should not be necessary. For dblink, I did not notice that the change for dblink_get_notify() would be this straight-forward, good catch. It would be nice to see prepTuplestoreResult() gone at the end, though I am not sure how invasive/worth it would be for dblink/. > - pg_logdir_ls() in contrib/adminpack has return type TYPEFUNC_RECORD > and expectedDesc is not already created, so the helper can't be used. > But basically, since it doesn't specify OUT argument names, it has to > do TupleDescInitEntry() itself anyway, I think. Here, actually, I thought first that a simplification should be possible, noticing that the regression tests passed with the function called until I saw what it was testing :) I am fine to not poke at the beast, and it looks like we may run into trouble if we wish to maintain compatibility down to adminpack 1.0 at runtime. This function has a very limited usage, anyway. > - contrib/tablefunc.c was also not simple to refactor. the various parts > of SetSingleFuncCall are spread throughout different functions in the > file. > > - contrib/dblink has one function which returns a tuplestore that was > simple to change (dblink_get_notify()) and I've done so. This matches my impression, so applied. -- Michael signature.asc Description: PGP signature
Re: pg_tablespace_location() failure with allow_in_place_tablespaces
On Tue, Mar 08, 2022 at 10:06:50AM +0900, Kyotaro Horiguchi wrote: > At Tue, 8 Mar 2022 10:39:06 +1300, Thomas Munro > wrote in >> Thanks, you're right. Test on a Win10 VM. Here's a new version. Looks fine to me. > FYI, on Windows11, pg_basebackup didn't work correctly without the > patch. So this looks like fixing an undiscovered bug as well. Well, that's not really a long-time bug but just a side effect of in-place tablespaces because we don't use them in many test cases yet, is it? >> pg_basebackup -D copy > WARNING: could not read symbolic link "pg_tblspc/16384": Invalid argument > pg_basebackup: error: tar member has empty name > >1 File(s) 0 bytes > 3 Dir(s) 171,920,613,376 bytes free That's a lot of free space. -- Michael signature.asc Description: PGP signature
Re: Comment typo in CheckCmdReplicaIdentity
On Mon, Mar 07, 2022 at 10:28:08AM +0800, Julien Rouhaud wrote: > +1 And done. -- Michael signature.asc Description: PGP signature
Re: wal_compression=zstd
On Sat, Mar 05, 2022 at 07:26:39PM +0900, Michael Paquier wrote:
> Repeatability and randomness of data counts, we could have for example
> one case with a set of 5~7 int attributes, a second with text values
> that include random data, up to 10~12 bytes each to count on the tuple
> header to be able to compress some data, and a third with more
> repeatable data, like one attribute with an int column populate
> with generate_series(). Just to give an idea.
And that's what I did as of the attached set of test:
- Cluster on tmpfs.
- max_wal_size, min_wal_size at 2GB and shared_buffers at 1GB, aka
large enough to include the full data set in memory.
- Rather than using Justin's full patch set, I have just patched the
code in xloginsert.c to switch the level.
- One case with table that uses one int attribute, with rather
repetitive data worth 484MB.
- One case with table using (int, text), where the text data is made
of 10~11 bytes of random data, worth ~340MB.
- Use pg_prewarm to load the data into shared buffers. With the
cluster mounted on a tmpfs that should not matter though.
- Both tables have a fillfactor at 50 to give room to the updates.
I have measured the CPU usage with a toy extension, also attached,
called pg_rusage() that is a simple wrapper to upstream's pg_rusage.c
to initialize a rusage snapshot and print its data with two SQL
functions that are called just before and after the FPIs are generated
(aka the UPDATE query that rewrites the whole table in the script
attached).
The quickly-hacked test script and the results are in test.tar.gz, for
reference. The toy extension is pg_rusage.tar.gz.
Here are the results I compiled, as of results_format.sql in the
tarball attached:
descr | rel_size | fpi_size | time_s
---+--+--+
int column no compression | 429 MB | 727 MB | 13.15
int column ztsd default level | 429 MB | 523 MB | 14.23
int column zstd level 1 | 429 MB | 524 MB | 13.94
int column zstd level 10 | 429 MB | 523 MB | 23.46
int column zstd level 19 | 429 MB | 523 MB | 103.71
int column lz4 default level | 429 MB | 575 MB | 13.37
int/text no compression | 344 MB | 558 MB | 10.08
int/text lz4 default level| 344 MB | 463 MB | 10.29
int/text zstd default level | 344 MB | 415 MB | 11.48
int/text zstd level 1 | 344 MB | 418 MB | 11.25
int/text zstd level 10| 344 MB | 415 MB | 20.59
int/text zstd level 19| 344 MB | 413 MB | 62.64
(12 rows)
I did not expect zstd to be this slow at a level of 10~ actually. The
runtime (elapsed CPU time) got severely impacted at level 19, that I
ran just for fun to see how that it would become compared to a level
of 10. There is a slight difference between the default level and a
level of 1, and the compression size does not change much, nor does
the CPU usage really change.
Attached is an updated patch, while on it, that I have tweaked before
running my own tests.
At the end, I'd still like to think that we'd better stick with the
default level for this parameter, and that's the suggestion of
upstream. So I would like to move on with that for this patch.
--
Michael
test.tar.gz
Description: application/gzip
pg_rusage.tar.gz
Description: application/gzip
From 254ddbf4223c35a7990e301e53d6ddbffcf210c0 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Fri, 18 Feb 2022 22:54:18 -0600
Subject: [PATCH v2] add wal_compression=zstd
---
src/include/access/xlog.h | 3 +-
src/include/access/xlogrecord.h | 5 +++-
src/backend/access/transam/xloginsert.c | 30 ++-
src/backend/access/transam/xlogreader.c | 20 +
src/backend/utils/misc/guc.c | 3 ++
src/backend/utils/misc/postgresql.conf.sample | 2 +-
src/bin/pg_waldump/pg_waldump.c | 2 ++
doc/src/sgml/config.sgml | 11 ---
doc/src/sgml/installation.sgml| 8 +
9 files changed, 76 insertions(+), 8 deletions(-)
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 4b45ac64db..09f6464331 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -75,7 +75,8 @@ typedef enum WalCompression
{
WAL_COMPRESSION_NONE = 0,
WAL_COMPRESSION_PGLZ,
- WAL_COMPRESSION_LZ4
+ WAL_COMPRESSION_LZ4,
+ WAL_COMPRESSION_ZSTD
} WalCompression;
/* Recovery states */
diff --git a/src/include/access/xlogrecord.h b/src/include/access/xlogrecord.h
index c1b1137aa7..052ac6817a 100644
--- a/src/include/access/xlogrecord.h
+++ b/src/include/access/xlogrecord.h
@@ -149,8 +149,11 @@ typedef struct XLogRecordBlockImageHeader
/* compression methods supported */
#define BKPIMAGE_COMPRESS_PGLZ 0x04
#define BKPIMAGE_COMPRESS_LZ4 0x08
+#define BKPIMAGE_COMPRESS_ZSTD 0x10
+
#define BKPIMAGE_COMPR
Re: Changing "Hot Standby" to "hot standby"
On Wed, Mar 09, 2022 at 07:45:32AM +, Daniel Westermann (DWE) wrote: > Thanks for having a look. Done that way. Hmm. Outside the title that had better use upper-case characters for the first letter of each word, I can see references to the pattern you are trying to eliminate in amcheck.sgml (1), config.sgml (3), protocol.sgml (3) and mvcc.sgml (1). Shouldn't you refresh these as well if the point is to make the full set of docs consistent? As of the full tree, I can see that: $ git grep "hot standby" | wc -l 259 $ git grep "Hot Standby" | wc -l 73 So there is a trend for one of the two. -- Michael signature.asc Description: PGP signature
Re: wal_compression=zstd
On Wed, Mar 09, 2022 at 07:14:11AM -0600, Justin Pryzby wrote: > Anyway there's no compelling reason to not use the default. If we were to use > a non-default default, we'd have to choose between 1 and 2 (or some negative > compression level). My thinking was that zstd-1 would give the lowest-hanging > fruits for zstd, while minimizing performance tradeoff, since WAL affects > interactivity. But choosing between 1 and 2 seems like bikeshedding. Yeah, I have looked again at the patch today, and I saw no reason to not apply it to give more options to the user as zstd or lz4 are both good in their own ways. So done, with the default level used. -- Michael signature.asc Description: PGP signature
Re: Add pg_freespacemap extension sql test
On Wed, Mar 09, 2022 at 08:13:15PM +0900, Dong Wook Lee wrote: > I agree with you, but I have no good idea how to deal with it. Well, my guess is that you basically just care about being able to detect if there is free space in the map or not, which goes down to detecting if pg_freespace() returns 0 or a number strictly higher than 0, so wouldn't it be enough to stick some > 0 in your test queries? Btw, if you want to test 32-bit builds, gcc allows that by passing down -m32. > Can the Perl TAP test be a good way? That does not seem necessary here. -- Michael signature.asc Description: PGP signature
Re: Changing "Hot Standby" to "hot standby"
On Thu, Mar 10, 2022 at 05:58:05PM -0500, Robert Treat wrote: > Not sure why the previous emails didn't go through, and still doesn't > look like they were picked up. In the interest of progress though, > attaching an updated patch with some minor wordsmithing; lmk if you'd > prefer this differently Looks the same as v5 for me, that applies the same consistency rules everywhere in the docs. So applied this one. -- Michael signature.asc Description: PGP signature
Re: wal_compression=zstd
On Fri, Mar 11, 2022 at 03:49:00PM -0600, Justin Pryzby wrote: > While rebasing, I realized this should have bumped XLOG_PAGE_MAGIC. > > Also, there's a dangling "and". Right. I'll address that a bit later today. Thanks! -- Michael signature.asc Description: PGP signature
Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
On Wed, Mar 09, 2022 at 10:50:45AM -0600, Justin Pryzby wrote: > Rebased over 9e9858389 (Michael may want to look at the tuplestore part?). Are you referring to the contents of 0003 here that changes the semantics of pg_ls_dir_files() regarding its setup call? -- Michael signature.asc Description: PGP signature
Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
On Wed, Mar 09, 2022 at 10:50:45AM -0600, Justin Pryzby wrote:
> I also changed pg_ls_dir_recurse() to handle concurrent removal of a dir,
> which
> I noticed caused an infrequent failure on CI. However I'm not including that
> here, since the 2nd half of the patch set seems isn't ready due to lstat() on
> windows.
lstat() has been a subject of many issues over the years with our
internal emulation and issues related to its concurrency, but we use
it in various areas of the in-core code, so that does not sound like
an issue to me. It depends on what you want to do with it in
genfile.c and which data you'd expect, in addition to the detection of
junction points for WIN32, I guess. v34 has no references to
pg_ls_dir_recurse(), but that's a WITH RECURSIVE, so we would not
really need it, do we?
@@ -27618,7 +27618,7 @@ SELECT
convert_from(pg_read_binary_file('file_in_utf8.txt'), 'UTF8');
Returns a record containing the file's size, last access time stamp,
last modification time stamp, last file status change time stamp (Unix
platforms only), file creation time stamp (Windows only), and a flag
-indicating if it is a directory.
+indicating if it is a directory (or a symbolic link to a directory).
This function is restricted to superusers by default, but other users
This is from 0001, and this addition in the documentation is not
completely right. As pg_stat_file() uses stat() to get back the
information of a file/directory, we'd just follow the link if
specifying one in the input argument. We could say instead, if we
were to improve the docs, that "If filename is a link, this function
returns information about the file or directory the link refers to."
I would put that as a different paragraph.
+select * from pg_ls_archive_statusdir() limit 0;
+ name | size | modification
+--+--+--
+(0 rows)
FWIW, this one is fine as of ValidateXLOGDirectoryStructure() that
would make sure archive_status exists before any connection is
attempted to the cluster.
> +select * from pg_ls_logdir() limit 0;
This test on pg_ls_logdir() would fail if running installcheck on a
cluster that has logging_collector disabled. So this cannot be
included.
+select * from pg_ls_logicalmapdir() limit 0;
+select * from pg_ls_logicalsnapdir() limit 0;
+select * from pg_ls_replslotdir('') limit 0;
+select * from pg_ls_tmpdir() limit 0;
+select * from pg_ls_waldir() limit 0;
+select * from pg_stat_file('.') limit 0;
The rest of the patch set should be stable AFAIK, there are various
steps when running a checkpoint that makes sure that any of these
exist, without caring about the value of wal_level.
+
+For each file in the specified directory, list the file and its
+metadata.
+Restricted to superusers by default, but other users can be granted
+EXECUTE to run the function.
+
What is metadata in this case? (I have read the code and know what
you mean, but folks only looking at the documentation may be puzzled
by that). It could be cleaner to use the same tupledesc for any
callers of this function, and return NULL in cases these are not
adapted.
+ /* check the optional arguments */
+ if (PG_NARGS() == 3)
+ {
+ if (!PG_ARGISNULL(1))
+ {
+ if (PG_GETARG_BOOL(1))
+ flags |= LS_DIR_MISSING_OK;
+ else
+ flags &= ~LS_DIR_MISSING_OK;
+ }
+
+ if (!PG_ARGISNULL(2))
+ {
+ if (PG_GETARG_BOOL(2))
+ flags &= ~LS_DIR_SKIP_DOT_DIRS;
+ else
+ flags |= LS_DIR_SKIP_DOT_DIRS;
+ }
+ }
The subtle different between the false and true code paths of those
arguments 1 and 2 had better be explained? The bit-wise operations
are slightly different in both cases, so it is not clear which part
does what, and what's the purpose of this logic.
- SetSingleFuncCall(fcinfo, 0);
+ /* isdir depends on metadata */
+ Assert(!(flags&LS_DIR_ISDIR) || (flags&LS_DIR_METADATA));
+ /* Unreasonable to show isdir and skip dirs */
+ Assert(!(flags&LS_DIR_ISDIR) || !(flags&LS_DIR_SKIP_DIRS));
Incorrect code format. Spaces required.
+-- This tests the missing_ok parameter, which causes pg_ls_tmpdir to
succeed even if the tmpdir doesn't exist yet
+-- The name='' condition is never true, so the function runs to
completion but returns zero rows.
+-- The query is written to ERROR if the tablespace doesn't exist,
rather than silently failing to call pg_ls_tmpdir()
+SELECT c.* FROM (SELECT oid FROM pg_tablespace b WHERE
b.spcname='regress_tblspace' UNION SELECT 0 ORDER BY 1 DESC LIMIT 1)
AS b , pg_ls_tmpdir(oid) AS c WHERE c.name='Does not exist';
So, here, we have a test that may not actually test what we want to
test.
Hmm. I am not convinced that we need a new set of SQL functions as
presented in 0003 (addition of meta-data in pg_ls_dir()), and
extensions of 0004 (do the same but for pg_ls_tmpd
Re: add checkpoint stats of snapshot and mapping files of pg_logical dir
On Sun, Mar 13, 2022 at 02:58:58PM -0700, Nathan Bossart wrote: > On Sun, Mar 13, 2022 at 01:54:10PM +0530, Bharath Rupireddy wrote: >> Another thing I added in v2 is to not emit snapshot and mapping files >> stats in case of restartpoint as logical decoding isn't supported on >> standbys, so it doesn't make sense to emit the stats there and cause >> server log to grow unnecessarily. Having said that, I added a note >> there to change it whenever logical decoding on standbys is supported. > > I think we actually do want to include this information for restartpoints > since some files might be left over from the snapshot that was used to > create the standby. Also, presumably these functions could do some work > during recovery on a primary. Yes, I would agree that consistency makes sense here, and it is not complicated to add the code to support this code path anyway. There is a risk that folks working on logical decoding on standbys overse this code path, instead. > Another problem I see is that this patch depends on the return value of the > lstat() calls that we are trying to remove in 0001 from another thread [0]. > I think the size of the removed/sync'd files is somewhat useful for > understanding disk space usage, but I suspect the time spent performing > these tasks is more closely related to the number of files. Do you think > reporting the sizes is worth the extra system call? We are not talking about files that are large either, are we? Another thing I am a bit annoyed with in this patch is the fact that the size of the ereport() call is doubled. The LOG currently generated is already bloated, and this does not arrange things. -- Michael signature.asc Description: PGP signature
Re: add checkpoint stats of snapshot and mapping files of pg_logical dir
On Mon, Mar 14, 2022 at 10:54:56AM +0530, Bharath Rupireddy wrote: > Yes, this is a concern. Also, when there were no logical replication > slots on a plain server or the server removed or cleaned up all the > snapshot/mappings files, why would anyone want to have these messages > with all 0s in the server log? The default settings don't enable that, so making things conditional roughly as you are suggesting with two different LOG messages sounds rather fine. > Here's what I'm thinking: > > Leave the existing "checkpoint/restartpoint complete" messages intact, > add the following in LogCheckpointEnd: FWIW, I also think that it would be good to check if there are cases where this information is significant enough that its inclusion makes sense. In the top message of the thread, the logs you showed refer to operations that represent 1/2ms worth of checkpoint. So, if in most cases this is going to be very quick, adding it to the logs won't matter because that's not a performance bottleneck. Perhaps that's something the patch that works on progress reporting for checkpoint is able to track? -- Michael signature.asc Description: PGP signature
Re: pg_tablespace_location() failure with allow_in_place_tablespaces
On Tue, Mar 15, 2022 at 02:33:17PM +1300, Thomas Munro wrote: > Ok, I pushed the fix for pg_basebackup. > > As for the complaint about pg_tablespace_location() failing, would it > be better to return an empty string? That's what was passed in as > LOCATION. Something like the attached. Hmm, I don't think so. The point of the function is to be able to know the location of a tablespace at SQL level so as we don't have any need to hardcode its location within any external tests (be it a pg_regress test or a TAP test) based on how in-place tablespace paths are built in the backend, so I think that we'd better report either a relative path from data_directory or an absolute path, but not an empty string. In any case, I'd suggest to add a regression test. What I have sent upthread would be portable enough. -- Michael signature.asc Description: PGP signature
Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
On Mon, Mar 14, 2022 at 01:53:54PM +0900, Michael Paquier wrote:
> +select * from pg_ls_logicalmapdir() limit 0;
> +select * from pg_ls_logicalsnapdir() limit 0;
> +select * from pg_ls_replslotdir('') limit 0;
> +select * from pg_ls_tmpdir() limit 0;
> +select * from pg_ls_waldir() limit 0;
> +select * from pg_stat_file('.') limit 0;
>
> The rest of the patch set should be stable AFAIK, there are various
> steps when running a checkpoint that makes sure that any of these
> exist, without caring about the value of wal_level.
I was contemplating at 0002 this morning, so see which parts would be
independently useful, and got reminded that we already check the
execution of all those functions in other regression tests, like
test_decoding for the replication slot ones and misc_functions.sql for
the more critical ones, so those extra queries would be just
interesting to check the shape of their SRFs, which is related to the
other patches of the set and limited based on my arguments from
yesterday.
There was one thing that stood out though: there was nothing for the
two options of pg_ls_dir(), called missing_ok and include_dot_dirs.
missing_ok is embedded in one query of pg_rewind, but this is a
counter-measure against concurrent file removals so we cannot rely on
pg_rewind to check that. And the second option was not run at all.
I have extracted both test cases after rewriting them a bit, and
applied that separately.
--
Michael
signature.asc
Description: PGP signature
Re: add checkpoint stats of snapshot and mapping files of pg_logical dir
On Mon, Mar 14, 2022 at 03:54:19PM +0530, Bharath Rupireddy wrote: > At times, the snapshot or mapping files can be large in number and one > some platforms it takes time for checkpoint to process all of them. > Having the stats about them in server logs can help us better analyze > why checkpoint took a long time and provide a better RCA. Do you have any numbers to share regarding that? Seeing information about 1k WAL segments being recycled and/or removed by a checkpoint where the operation takes dozens of seconds to complete because we can talk about hundred of gigs worth of files moved around. If we are talking about 100~200 files up to 10~20kB each for snapshot and mapping files, the information has less value, worth only a portion of one WAL segment. -- Michael signature.asc Description: PGP signature
Re: Estimating HugePages Requirements?
On Mon, Mar 14, 2022 at 10:34:17AM -0700, Nathan Bossart wrote: > On Mon, Mar 14, 2022 at 04:26:43PM +0100, Magnus Hagander wrote: >> And in fact, the command documented on >> https://www.postgresql.org/docs/devel/kernel-resources.html doesn't >> actually produce the output that the docs show, it also shows the log >> line, in the default config? If we can't fix the extra logging we >> should at least have our docs represent reality -- maybe by adding a >> "2>/dev/null" entry? But it'd be better to have it not output that log >> in the first place... > > I attached a patch to adjust the documentation for now. This apparently > crossed my mind earlier [0], but I didn't follow through with it for some > reason. Another thing that we can add is -c log_min_messages=fatal, but my method is more complicated than a simple redirection, of course :) >> (Of course what I'd really want is to be able to run it on a cluster >> that's running, but that was discussed downthread so I'm not bringing >> that one up for changes now) > > I think it is worth revisiting the extra logging and the ability to view > runtime-computed GUCs on a running server. Should this be an open item for > v15, or do you think it's alright to leave it for the v16 development > cycle? Well, this is a completely new problem as it opens the door of potential concurrent access issues with the data directory lock file while reading values from the control file. And that's not mandatory to be able to get those estimations without having to allocate a large chunk of memory, which was the primary goal discussed upthread as far as I recall. So I would leave that as an item to potentially tackle in future versions. -- Michael signature.asc Description: PGP signature
Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
On Mon, Mar 14, 2022 at 09:37:25PM -0500, Justin Pryzby wrote: > One could argue that most of the pg_ls_* functions aren't needed (including > 1922d7c6e), since the same things are possible with pg_ls_dir() and > pg_stat_file(). > |1922d7c6e Add SQL functions to monitor the directory contents of replication > slots This main argument behind this one is monitoring, as the execution to those functions can be granted at a granular level depending on the roles doing the disk space lookups. -- Michael signature.asc Description: PGP signature
Re: [PATCH] Fix various spelling errors
On Mon, Mar 14, 2022 at 06:49:07PM -0500, Justin Pryzby wrote: > On Mon, Mar 14, 2022 at 11:03:50PM +, Kekalainen, Otto wrote: >> I propose the attached patch to be applied on the 'master' branch of >> PostgreSQL >> to fix various spelling errors. >> >> Most fixes are in comments and have no effect on functionality. Some fixes >> are >> also in variable names but they should be safe to change, as the change is >> consistent in all occurrences of the variable. > > LGTM - I found a few of these myself. > Attached now, in case it's useful to handle them together. It is useful to group that together. I have gathered everything that looked like a typo or a grammar mistake, and applied the fixes. Thanks! -- Michael signature.asc Description: PGP signature
Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
On Tue, Mar 15, 2022 at 05:23:40PM +0900, Kyotaro Horiguchi wrote: > At Tue, 15 Mar 2022 12:19:47 +0530, Bharath Rupireddy > wrote in >> On Fri, Mar 4, 2022 at 10:40 AM Kyotaro Horiguchi >> wrote: >> 0001 - I don't think you need to do this as UpdateControlFile >> (update_controlfile) will anyway update it, no? >> + /* Update control file using current time */ >> + ControlFile->time = (pg_time_t) time(NULL); > > Ugh.. Yes. It is a copy-pasto from older versions. They may have the > same copy-pasto.. This thread has shifted to an entirely different discussion, presenting patches that touch code paths unrelated to what was first stated. Shouldn't you create a new thread with a proper $subject to attract a more correct audience? -- Michael signature.asc Description: PGP signature
Re: pg_tablespace_location() failure with allow_in_place_tablespaces
On Tue, Mar 15, 2022 at 03:55:56PM +1300, Thomas Munro wrote: > On Tue, Mar 15, 2022 at 2:50 PM Michael Paquier wrote: >> On Tue, Mar 15, 2022 at 02:33:17PM +1300, Thomas Munro wrote: >> > As for the complaint about pg_tablespace_location() failing, would it >> > be better to return an empty string? That's what was passed in as >> > LOCATION. Something like the attached. >> >> Hmm, I don't think so. The point of the function is to be able to >> know the location of a tablespace at SQL level so as we don't have any >> need to hardcode its location within any external tests (be it a >> pg_regress test or a TAP test) based on how in-place tablespace paths >> are built in the backend, so I think that we'd better report either a >> relative path from data_directory or an absolute path, but not an >> empty string. >> >> In any case, I'd suggest to add a regression test. What I have sent >> upthread would be portable enough. > > Fair enough. No objections here. So, which one of a relative path or an absolute path do you think would be better for the user? My preference tends toward the relative path, as we know that all those tablespaces stay in pg_tblspc/ so one can make the difference with normal tablespaces more easily. The barrier is thin, though :p -- Michael signature.asc Description: PGP signature
Re: Assert in pageinspect with NULL pages
On Thu, Feb 17, 2022 at 09:00:20PM -0600, Justin Pryzby wrote:
> BRIN can also crash if passed a non-brin index.
>
> I've been sitting on this one for awhile. Feel free to include it in your
> patchset.
So, I have begun a close lookup of this thread, and the problem you
are reporting here is worth fixing on its own, before we do something
for new pages as reported originally. There is more that caught my
attention than the brin AM not being check properly, because a couple
of code paths are fuzzy with the checks on the page sizes. My
impression of the whole thing is that we'd better generalize the use
of get_page_from_raw(), improving the code on many sides when the user
can provide a raw page in input (also I'd like to think that it is a
better practice anyway as any of those functions may finish to look
8-byte fields, but the current coding would silently break in
alignment-picky environments):
- Some code paths (hash, btree) would trigger elog(ERROR) but we
cannot use that for errors that can be triggered by the user.
- bt_page_items_bytea(), page_header() and fsm_page_contents() were
fuzzy on the page size check, so it was rather easy to generate
garbage with random data.
- page_checksum_internal() used a PageHeader, where I would have
expected a Page.
- More consistency in the error strings, meaning less contents to
translate in the long-term.
This first batch leads me to the attached, with tests to stress all
that for all the functions taking raw pages in input.
--
Michael
From 588ffddf2bd2c0d1e6168a2e7093c2488caec94b Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Tue, 15 Mar 2022 17:59:04 +0900
Subject: [PATCH] Fixes for pageinspect with page sizes
---
contrib/pageinspect/brinfuncs.c| 36 ++
contrib/pageinspect/btreefuncs.c | 28 ++--
contrib/pageinspect/expected/brin.out | 4 +++
contrib/pageinspect/expected/btree.out | 15 +++
contrib/pageinspect/expected/gin.out | 11
contrib/pageinspect/expected/gist.out | 15 +++
contrib/pageinspect/expected/hash.out | 17
contrib/pageinspect/expected/page.out | 11
contrib/pageinspect/fsmfuncs.c | 4 ++-
contrib/pageinspect/gistfuncs.c| 9 +++
contrib/pageinspect/hashfuncs.c| 6 +++--
contrib/pageinspect/rawpage.c | 29 +++--
contrib/pageinspect/sql/brin.sql | 4 +++
contrib/pageinspect/sql/btree.sql | 13 ++
contrib/pageinspect/sql/gin.sql| 9 +++
contrib/pageinspect/sql/gist.sql | 13 ++
contrib/pageinspect/sql/hash.sql | 13 ++
contrib/pageinspect/sql/page.sql | 9 +++
18 files changed, 179 insertions(+), 67 deletions(-)
diff --git a/contrib/pageinspect/brinfuncs.c b/contrib/pageinspect/brinfuncs.c
index b7c8365218..bd0ea8b18c 100644
--- a/contrib/pageinspect/brinfuncs.c
+++ b/contrib/pageinspect/brinfuncs.c
@@ -16,6 +16,7 @@
#include "access/brin_tuple.h"
#include "access/htup_details.h"
#include "catalog/index.h"
+#include "catalog/pg_am_d.h"
#include "catalog/pg_type.h"
#include "funcapi.h"
#include "lib/stringinfo.h"
@@ -31,6 +32,8 @@ PG_FUNCTION_INFO_V1(brin_page_items);
PG_FUNCTION_INFO_V1(brin_metapage_info);
PG_FUNCTION_INFO_V1(brin_revmap_data);
+#define IS_BRIN(r) ((r)->rd_rel->relam == BRIN_AM_OID)
+
typedef struct brin_column_state
{
int nstored;
@@ -45,8 +48,7 @@ Datum
brin_page_type(PG_FUNCTION_ARGS)
{
bytea *raw_page = PG_GETARG_BYTEA_P(0);
- Page page = VARDATA(raw_page);
- int raw_page_size;
+ Page page;
char *type;
if (!superuser())
@@ -54,14 +56,7 @@ brin_page_type(PG_FUNCTION_ARGS)
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to use raw page functions")));
- raw_page_size = VARSIZE(raw_page) - VARHDRSZ;
-
- if (raw_page_size != BLCKSZ)
- ereport(ERROR,
-(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("input page too small"),
- errdetail("Expected size %d, got %d",
- BLCKSZ, raw_page_size)));
+ page = get_page_from_raw(raw_page);
switch (BrinPageType(page))
{
@@ -89,19 +84,7 @@ brin_page_type(PG_FUNCTION_ARGS)
static Page
verify_brin_page(bytea *raw_page, uint16 type, const char *strtype)
{
- Page page;
- int raw_page_size;
-
- raw_page_size = VARSIZE(raw_page) - VARHDRSZ;
-
- if (raw_page_size != BLCKSZ)
- ereport(ERROR,
-(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("input page too small"),
- errdetail("Expected size %d, got %d",
- BLCKSZ, raw_page_size)));
-
- page = VARDATA(raw_page);
+ Page page = get_page_from_raw(raw_page);
/* verify the special space says this page is what we want */
if (BrinPageType(page) != type)
@@ -143,6 +126,13 @@ brin_page_items(PG_FUNCTION_ARGS)
SetSingleFuncCall(
Re: Assert in pageinspect with NULL pages
On Tue, Mar 15, 2022 at 06:56:46AM -0500, Justin Pryzby wrote: > On Tue, Mar 15, 2022 at 06:32:44PM +0900, Michael Paquier wrote: >> +-- Suppress the DETAIL message, to allow the tests to work across various >> +-- default page sizes. > > I think you mean "various non-default page sizes" or "various page sizes". Thanks. Indeed, this sounded a bit weird. I have been able to look at all that this morning and backpatched this first part. -- Michael signature.asc Description: PGP signature
Re: pg_tablespace_location() failure with allow_in_place_tablespaces
On Wed, Mar 16, 2022 at 10:34:15AM +0900, Kyotaro Horiguchi wrote:
> +1. Desn't the doc need to mention that?
Yes, I agree that it makes sense to add a note, even if
allow_in_place_tablespaces is a developer option. I have added the
following paragraph in the docs:
+A full path of the symbolic link in pg_tblspc/
+is returned. A relative path to the data directory is returned
+for tablespaces created with
+ enabled.
Another thing that was annoying in the first version of the patch is
the useless call to lstat() on Windows, not needed because it is
possible to rely just on pgwin32_is_junction() to check if readlink()
should be called or not.
This leads me to the revised version attached. What do you think?
--
Michael
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 4568749d23..89690be2ed 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -15,6 +15,7 @@
#include "postgres.h"
#include
+#include
#include
#include
#include
@@ -282,6 +283,9 @@ pg_tablespace_location(PG_FUNCTION_ARGS)
char sourcepath[MAXPGPATH];
char targetpath[MAXPGPATH];
int rllen;
+#ifndef WIN32
+ struct stat st;
+#endif
/*
* It's useful to apply this function to pg_class.reltablespace, wherein
@@ -306,6 +310,31 @@ pg_tablespace_location(PG_FUNCTION_ARGS)
*/
snprintf(sourcepath, sizeof(sourcepath), "pg_tblspc/%u", tablespaceOid);
+ /*
+ * Before reading the link, check if the source path is a link or a
+ * junction point. Note that a directory is possible for a tablespace
+ * created with allow_in_place_tablespaces enabled. If a directory is
+ * found, a relative path to the data directory is returned.
+ */
+#ifdef WIN32
+ if (!pgwin32_is_junction(sourcepath))
+ PG_RETURN_TEXT_P(cstring_to_text(sourcepath));
+#else
+ if (lstat(sourcepath, &st) < 0)
+ {
+ ereport(ERROR,
+(errcode_for_file_access(),
+ errmsg("could not stat file \"%s\": %m",
+ sourcepath)));
+ }
+
+ if (!S_ISLNK(st.st_mode))
+ PG_RETURN_TEXT_P(cstring_to_text(sourcepath));
+#endif
+
+ /*
+ * In presence of a link or a junction point, return the path pointing to.
+ */
rllen = readlink(sourcepath, targetpath, sizeof(targetpath));
if (rllen < 0)
ereport(ERROR,
diff --git a/src/test/regress/expected/tablespace.out b/src/test/regress/expected/tablespace.out
index 2dfbcfdebe..473fe8c28e 100644
--- a/src/test/regress/expected/tablespace.out
+++ b/src/test/regress/expected/tablespace.out
@@ -24,6 +24,15 @@ SELECT spcoptions FROM pg_tablespace WHERE spcname = 'regress_tblspacewith';
DROP TABLESPACE regress_tblspacewith;
-- create a tablespace we can use
CREATE TABLESPACE regress_tblspace LOCATION '';
+-- This returns a relative path as of an effect of allow_in_place_tablespaces,
+-- masking the tablespace OID used in the path name.
+SELECT regexp_replace(pg_tablespace_location(oid), '(pg_tblspc)/(\d+)', '\1/NNN')
+ FROM pg_tablespace WHERE spcname = 'regress_tblspace';
+ regexp_replace
+
+ pg_tblspc/NNN
+(1 row)
+
-- try setting and resetting some properties for the new tablespace
ALTER TABLESPACE regress_tblspace SET (random_page_cost = 1.0, seq_page_cost = 1.1);
ALTER TABLESPACE regress_tblspace SET (some_nonexistent_parameter = true); -- fail
diff --git a/src/test/regress/sql/tablespace.sql b/src/test/regress/sql/tablespace.sql
index 896f05cea3..0949a28488 100644
--- a/src/test/regress/sql/tablespace.sql
+++ b/src/test/regress/sql/tablespace.sql
@@ -22,6 +22,10 @@ DROP TABLESPACE regress_tblspacewith;
-- create a tablespace we can use
CREATE TABLESPACE regress_tblspace LOCATION '';
+-- This returns a relative path as of an effect of allow_in_place_tablespaces,
+-- masking the tablespace OID used in the path name.
+SELECT regexp_replace(pg_tablespace_location(oid), '(pg_tblspc)/(\d+)', '\1/NNN')
+ FROM pg_tablespace WHERE spcname = 'regress_tblspace';
-- try setting and resetting some properties for the new tablespace
ALTER TABLESPACE regress_tblspace SET (random_page_cost = 1.0, seq_page_cost = 1.1);
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 8a802fb225..df54c5815d 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -23924,7 +23924,14 @@ SELECT currval(pg_get_serial_sequence('sometable', 'id'));
Returns the file system path that this tablespace is located in.
-
+
+
+A full path of the symbolic link in pg_tblspc/
+is returned. A relative path to the data directory is returned
+for tablespaces created with
+ enabled.
+
+
signature.asc
Description: PGP signature
Re: Tab completion for ALTER MATERIALIZED VIEW ... SET ACCESS METHOD
Hi Nagata-san, On Wed, Mar 16, 2022 at 01:33:37PM +0900, Yugo NAGATA wrote: > SET ACCESS METHOD is supported in ALTER TABLE since the commit > b0483263dd. Since that time, this also has be allowed SET ACCESS > METHOD in ALTER MATERIALIZED VIEW. Although it is not documented, > this works. Yes, that's an oversight. I see no reason to not authorize that, and the rewrite path in tablecmds.c is the same as for plain tables. > I cannot found any reasons to prohibit SET ACCESS METHOD in ALTER > MATERIALIZED VIEW, so I think it is better to support this in psql > tab-completion and be documented. I think that we should have some regression tests about those command flavors. How about adding a couple of queries to create_am.sql for SET ACCESS METHOD and to tablespace.sql for SET TABLESPACE? -- Michael signature.asc Description: PGP signature
Re: Assert in pageinspect with NULL pages
On Mon, Feb 21, 2022 at 10:00:00AM +0300, Alexander Lakhin wrote: > Could you please confirm before committing the patchset that it fixes > the bug #16527 [1]? Or maybe I could check it? > (Original patch proposed by Daria doesn't cover that case, but if the > patch going to be improved, probably it's worth fixing that bug too.) > > [1] > https://www.postgresql.org/message-id/flat/16527-ef7606186f0610a1%40postgresql.org FWIW, I think that this one has been fixed by 076f4d9, where we make sure that the page is correctly aligned. Are you still seeing this problem? -- Michael signature.asc Description: PGP signature
Re: Out-of-tree certificate interferes ssltest
On Wed, Mar 16, 2022 at 04:36:58PM +0900, Kyotaro Horiguchi wrote: > ok 6 - ssl_client_cert_present() for connection with cert > connection error: 'psql: error: connection to server at "127.0.0.1", port > 61688 failed: could not read certificate file > "/home/horiguti/.postgresql/postgresql.crt": no start line' > while running 'psql -XAtq -d sslrootcert=ssl/root+server_ca.crt > sslmode=require dbname=trustdb hostaddr=127.0.0.1 user=ssltestuser > host=localhost -f - -v ON_ERR > > I think we don't want this behavior. > > The attached fixes that and make-world successfully finished even if I > have a cert file in my home direcotory. That's the same issue as the one fixed in dd87799, using the same method. I'll double-check on top of looking at what you are suggesting here. -- Michael signature.asc Description: PGP signature
Re: Assert in pageinspect with NULL pages
On Wed, Feb 23, 2022 at 12:09:02PM +0500, Daria Lepikhova wrote:
> And one more addition. In the previous version of the patch, I forgot to add
> tests for the gist index, but the described problem is also relevant for it.
So, I have looked at this second part of the thread, and concluded
that we should not fail for empty pages. First, we fetch pages from
the buffer pool in normal mode, where empty pages are valid. There is
also a second point in favor of doing so: code paths dedicated to hash
indexes already do that, marking such pages as simply "unused". The
proposal from Julien upthread sounds cleaner to me though in the long
run, as NULL gives the user the possibility to do a full-table scan
with simple clauses to filter out anything found as NULL.
Painting more PageIsNew() across the place requires a bit more work
than a simple ereport(ERROR) in get_page_from_raw(), of course, but
the gain is the portability of the functions.
(One can have a lot of fun playing with random inputs and breaking
most code paths, but that's not new.)
--
Michael
diff --git a/contrib/pageinspect/brinfuncs.c b/contrib/pageinspect/brinfuncs.c
index bf12901ac3..87e75f663c 100644
--- a/contrib/pageinspect/brinfuncs.c
+++ b/contrib/pageinspect/brinfuncs.c
@@ -58,6 +58,9 @@ brin_page_type(PG_FUNCTION_ARGS)
page = get_page_from_raw(raw_page);
+ if (PageIsNew(page))
+ PG_RETURN_NULL();
+
switch (BrinPageType(page))
{
case BRIN_PAGETYPE_META:
@@ -86,6 +89,9 @@ verify_brin_page(bytea *raw_page, uint16 type, const char *strtype)
{
Page page = get_page_from_raw(raw_page);
+ if (PageIsNew(page))
+ return page;
+
/* verify the special space says this page is what we want */
if (BrinPageType(page) != type)
ereport(ERROR,
@@ -138,6 +144,13 @@ brin_page_items(PG_FUNCTION_ARGS)
/* minimally verify the page we got */
page = verify_brin_page(raw_page, BRIN_PAGETYPE_REGULAR, "regular");
+ if (PageIsNew(page))
+ {
+ brin_free_desc(bdesc);
+ index_close(indexRel, AccessShareLock);
+ PG_RETURN_NULL();
+ }
+
/*
* Initialize output functions for all indexed datatypes; simplifies
* calling them later.
@@ -313,6 +326,9 @@ brin_metapage_info(PG_FUNCTION_ARGS)
page = verify_brin_page(raw_page, BRIN_PAGETYPE_META, "metapage");
+ if (PageIsNew(page))
+ PG_RETURN_NULL();
+
/* Build a tuple descriptor for our result type */
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
@@ -364,6 +380,12 @@ brin_revmap_data(PG_FUNCTION_ARGS)
/* minimally verify the page we got */
page = verify_brin_page(raw_page, BRIN_PAGETYPE_REVMAP, "revmap");
+ if (PageIsNew(page))
+ {
+ MemoryContextSwitchTo(mctx);
+ PG_RETURN_NULL();
+ }
+
state = palloc(sizeof(*state));
state->tids = ((RevmapContents *) PageGetContents(page))->rm_tids;
state->idx = 0;
diff --git a/contrib/pageinspect/btreefuncs.c b/contrib/pageinspect/btreefuncs.c
index d9628dd664..dde640fd33 100644
--- a/contrib/pageinspect/btreefuncs.c
+++ b/contrib/pageinspect/btreefuncs.c
@@ -611,6 +611,12 @@ bt_page_items_bytea(PG_FUNCTION_ARGS)
uargs->page = get_page_from_raw(raw_page);
+ if (PageIsNew(uargs->page))
+ {
+ MemoryContextSwitchTo(mctx);
+ PG_RETURN_NULL();
+ }
+
uargs->offset = FirstOffsetNumber;
opaque = (BTPageOpaque) PageGetSpecialPointer(uargs->page);
diff --git a/contrib/pageinspect/expected/brin.out b/contrib/pageinspect/expected/brin.out
index 10cd36c177..15786a0c0d 100644
--- a/contrib/pageinspect/expected/brin.out
+++ b/contrib/pageinspect/expected/brin.out
@@ -52,4 +52,29 @@ SELECT * FROM brin_page_items(get_raw_page('test1_a_idx', 2), 'test1_a_idx')
CREATE INDEX test1_a_btree ON test1 (a);
SELECT brin_page_items(get_raw_page('test1_a_btree', 0), 'test1_a_btree');
ERROR: "test1_a_btree" is not a BRIN index
+-- Test for empty pages with raw input.
+SHOW block_size \gset
+SELECT brin_page_type(decode(repeat('00', :block_size), 'hex'));
+ brin_page_type
+
+
+(1 row)
+
+SELECT brin_page_items(decode(repeat('00', :block_size), 'hex'), 'test1_a_idx');
+ brin_page_items
+-
+(0 rows)
+
+SELECT brin_metapage_info(decode(repeat('00', :block_size), 'hex'));
+ brin_metapage_info
+
+
+(1 row)
+
+SELECT brin_revmap_data(decode(repeat('00', :block_size), 'hex'));
+ brin_revmap_data
+--
+
+(1 row)
+
DROP TABLE test1;
diff --git a/contrib/pageinspect/expected/btree.out b/contrib/pageinspect/expected/btree.out
index 80b3dfe861..8815b56b15 100644
--- a/contrib/pageinspect/expected/btree.out
+++ b/contrib/pageinspect/expected/btree.out
@@ -85,4 +85,10 @@ ERROR: "test1_a_hash" is not a btree index
SELECT bt_page_items('aaa'::bytea);
ERROR: invalid page size
\set VERBOSITY default
+-- Test for empty pages with raw input.
+SHOW block_size \gset
+SELECT bt_page_items(decode(repeat('00', :block_size), 'hex'));
+-[ RECORD 1 ]-+-
+bt_page_items |
+
DROP TABLE test1;
d
Re: pg_tablespace_location() failure with allow_in_place_tablespaces
On Wed, Mar 16, 2022 at 05:15:58PM +0900, Kyotaro Horiguchi wrote: > I'm not sure that the "of the symbolic link in pg_tblspc/" is > needed. And allow_in_place_tablespaces alone doesn't create in-place > tablespace. So this might need rethink at least for the second point. Surely this can be improved. I was not satisfied with this paragraph after re-reading it this morning, so I have just removed it, rewording slightly the part for in-place tablespaces that is still necessary. > Agreed. And v2 looks cleaner. > > The test detects the lack of the feature. > It successfully builds and runs on Rocky8 and Windows11. Thanks for the review. After a second look, it seemed fine so I have applied it. (I'll try to jump on the tablespace patch for recovery soon-ish-ly if nobody beats me to it.) -- Michael signature.asc Description: PGP signature
Re: Out-of-tree certificate interferes ssltest
On Wed, Mar 16, 2022 at 11:45:39AM +0100, Daniel Gustafsson wrote:
> On 16 Mar 2022, at 08:36, Kyotaro Horiguchi wrote:
>> The attached fixes that and make-world successfully finished even if I
>> have a cert file in my home direcotory.
>
> Seems correct to me, thanks!
The ultimate test I can think about to stress the robustness of this
test suite is to generate various certs and keys using "make
sslfiles", save them into a ~/.postgresql/ (postgresql.crt,
postgresql.key, root.crl and root.crt), and then run the tests to see
how much junk data the SSL scripts would feed on. With this method, I
have caught a total of 71 failures, much more than reported upthread.
We should really put more attention to set invalid default values for
sslcert, sslkey, sslcrl, sslcrldir and sslrootcert, rather than
hardcoding a couple of them in only a few places, opening ourselves to
the same problem, again, each time a new test is added. The best way
I can think about here is to use a string that includes all the
default SSL settings, appending that at the beginning of each
$common_connstr. This takes care of most the failures, except two
cases related to expected failures for sslcrldir:
- directory CRL belonging to a different CA
- does not connect with client-side CRL directory
In both cases, enforcing sslcrl to a value of "invalid" interferes
with the failure scenario we expect from sslcrldir. It is possible to
bypass that with something like the attached, but that's a kind of
ugly hack. Another alternative would be to drop those two tests, and
I am not sure how much we care about these two negative scenarios.
Thoughts?
--
Michael
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 5c5b16fbe7..55f2a52dee 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -138,8 +138,17 @@ note "running client tests";
switch_server_cert($node, 'server-cn-only');
+# Set of default settings for SSL parameters in connection string. This
+# makes the tests protected against any defaults the environment may have
+# in ~/.postgresql/. no_crl is a flavor that does not have a default
+# value set for the connection parameter sslcrl.
+my $default_ssl_connstr = "sslkey=invalid sslcert=invalid sslrootcert=invalid sslcrl=invalid sslcrldir=invalid";
+my $default_ssl_connstr_no_crl = "sslkey=invalid sslcert=invalid sslrootcert=invalid sslcrldir=invalid";
+
$common_connstr =
- "user=ssltestuser dbname=trustdb sslcert=invalid hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test";
+ "$default_ssl_connstr user=ssltestuser dbname=trustdb hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test";
+my $common_connstr_no_crl =
+ "$default_ssl_connstr_no_crl user=ssltestuser dbname=trustdb hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test";
# The server should not accept non-SSL connections.
$node->connect_fails(
@@ -150,14 +159,14 @@ $node->connect_fails(
# Try without a root cert. In sslmode=require, this should work. In verify-ca
# or verify-full mode it should fail.
$node->connect_ok(
- "$common_connstr sslrootcert=invalid sslmode=require",
+ "$common_connstr sslmode=require",
"connect without server root cert sslmode=require");
$node->connect_fails(
- "$common_connstr sslrootcert=invalid sslmode=verify-ca",
+ "$common_connstr sslmode=verify-ca",
"connect without server root cert sslmode=verify-ca",
expected_stderr => qr/root certificate file "invalid" does not exist/);
$node->connect_fails(
- "$common_connstr sslrootcert=invalid sslmode=verify-full",
+ "$common_connstr sslmode=verify-full",
"connect without server root cert sslmode=verify-full",
expected_stderr => qr/root certificate file "invalid" does not exist/);
@@ -217,8 +226,10 @@ $node->connect_fails(
expected_stderr => qr/SSL error: certificate verify failed/);
# The same for CRL directory
+# The default connection string should not include a default value for
+# sslcrl, or this interferes with the result of this test.
$node->connect_fails(
- "$common_connstr sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca sslcrldir=ssl/client-crldir",
+ "$common_connstr_no_crl sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca sslcrldir=ssl/client-crldir",
"directory CRL belonging to a different CA",
expected_stderr => qr/SSL error: certificate verify failed/);
@@ -235,7 +246,7 @@ $node->connect_ok(
# Check that connecting with verify-full fails, when the hostname doesn't
# match the hostname in the server's certificate.
$common_connstr =
- "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";
+ "$default_ssl_connstr user=ssltestuser dbname=trustdb sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";
$node->connect_ok("$common_connstr sslmode=require host=wronghost.test",
"mismatch between host name and server certificate sslmode=require");
@@ -253,7 +264,7 @@ $node->connect_fails(
switch_server_cert($no
Re: pg_tablespace_location() failure with allow_in_place_tablespaces
On Thu, Mar 17, 2022 at 04:34:30PM +1300, Thomas Munro wrote: > I think what Horiguchi-san was pointing out above is that you need to > enable the GUC *and* say LOCATION '', which the new paragraph doesn't > capture. What do you think about this: > > A path relative to the data directory is returned for in-place > tablespaces (see ). An issue I have with this wording is that we give nowhere in the docs an explanation of about the term "in-place tablespace", even if it can be guessed from the name of the GUC. Another idea would be something like that: "A relative path to the data directory is returned for tablespaces created with an empty location string specified in the CREATE TABLESPACE query when allow_in_place_tablespaces is enabled (see link blah)." But perhaps that's just me being overly pedantic :p -- Michael signature.asc Description: PGP signature
Re: Out-of-tree certificate interferes ssltest
On Thu, Mar 17, 2022 at 02:59:26PM +0900, Michael Paquier wrote:
> In both cases, enforcing sslcrl to a value of "invalid" interferes
> with the failure scenario we expect from sslcrldir. It is possible to
> bypass that with something like the attached, but that's a kind of
> ugly hack. Another alternative would be to drop those two tests, and
> I am not sure how much we care about these two negative scenarios.
Actually, there is a trick I have recalled here: we can enforce sslcrl
to an empty value in the connection string after the default. This
still ensures that the test won't pick up any SSL data from the local
environment and avoids any interferences of OpenSSL's
X509_STORE_load_locations(). This gives a much simpler and cleaner
patch.
Thoughts?
--
Michael
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 5c5b16fbe7..45bd962f40 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -138,8 +138,13 @@ note "running client tests";
switch_server_cert($node, 'server-cn-only');
+# Set of default settings for SSL parameters in connection string. This
+# makes the tests protected against any defaults the environment may have
+# in ~/.postgresql/.
+my $default_ssl_connstr = "sslkey=invalid sslcert=invalid sslrootcert=invalid sslcrl=invalid sslcrldir=invalid";
+
$common_connstr =
- "user=ssltestuser dbname=trustdb sslcert=invalid hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test";
+ "$default_ssl_connstr user=ssltestuser dbname=trustdb hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test";
# The server should not accept non-SSL connections.
$node->connect_fails(
@@ -150,14 +155,14 @@ $node->connect_fails(
# Try without a root cert. In sslmode=require, this should work. In verify-ca
# or verify-full mode it should fail.
$node->connect_ok(
- "$common_connstr sslrootcert=invalid sslmode=require",
+ "$common_connstr sslmode=require",
"connect without server root cert sslmode=require");
$node->connect_fails(
- "$common_connstr sslrootcert=invalid sslmode=verify-ca",
+ "$common_connstr sslmode=verify-ca",
"connect without server root cert sslmode=verify-ca",
expected_stderr => qr/root certificate file "invalid" does not exist/);
$node->connect_fails(
- "$common_connstr sslrootcert=invalid sslmode=verify-full",
+ "$common_connstr sslmode=verify-full",
"connect without server root cert sslmode=verify-full",
expected_stderr => qr/root certificate file "invalid" does not exist/);
@@ -216,9 +221,10 @@ $node->connect_fails(
"CRL belonging to a different CA",
expected_stderr => qr/SSL error: certificate verify failed/);
-# The same for CRL directory
+# The same for CRL directory. sslcrl='' is added here to override the
+# invalid default, so as this does not interfere with this case.
$node->connect_fails(
- "$common_connstr sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca sslcrldir=ssl/client-crldir",
+ "$common_connstr sslcrl='' sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca sslcrldir=ssl/client-crldir",
"directory CRL belonging to a different CA",
expected_stderr => qr/SSL error: certificate verify failed/);
@@ -235,7 +241,7 @@ $node->connect_ok(
# Check that connecting with verify-full fails, when the hostname doesn't
# match the hostname in the server's certificate.
$common_connstr =
- "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";
+ "$default_ssl_connstr user=ssltestuser dbname=trustdb sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";
$node->connect_ok("$common_connstr sslmode=require host=wronghost.test",
"mismatch between host name and server certificate sslmode=require");
@@ -253,7 +259,7 @@ $node->connect_fails(
switch_server_cert($node, 'server-multiple-alt-names');
$common_connstr =
- "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR sslmode=verify-full";
+ "$default_ssl_connstr user=ssltestuser dbname=trustdb sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR sslmode=verify-full";
$node->connect_ok(
"$common_connstr host=dns1.alt-name.pg-ssltest.test",
@@ -282,7 +288,7 @@ $node->connect_fails(
switch_server_cert($node, 'server-single-alt-name');
$common_connstr =
- "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR sslmode=verify-full";
+ "$default_ssl_connstr user=ssltestuser dbname=trustdb sslrootcert=ssl/root+server_ca.crt hostaddr=$SE
