Re: [PoC] pg_upgrade: allow to upgrade publisher node
On Fri, Aug 18, 2023 at 12:47 PM Amit Kapila wrote: > > On Thu, Aug 17, 2023 at 2:10 PM Peter Smith wrote: > > > > Here are some review comments for the first 2 patches. > > > > /* > > - * Fetch all libraries containing non-built-in C functions in this DB. > > + * Fetch all libraries containing non-built-in C functions and > > + * output plugins in this DB. > > */ > > ress[dbnum] = executeQueryOrDie(conn, > > "SELECT DISTINCT probin " > > "FROM pg_catalog.pg_proc " > > "WHERE prolang = %u AND " > > "probin IS NOT NULL AND " > > - "oid >= %u;", > > + "oid >= %u " > > + "UNION " > > + "SELECT DISTINCT plugin " > > + "FROM pg_catalog.pg_replication_slots " > > + "WHERE wal_status <> 'lost' AND " > > + "database = current_database() AND " > > + "temporary IS FALSE;", > > ClanguageId, > > FirstNormalObjectId); > > totaltups += PQntuples(ress[dbnum]); > > > > ~ > > > > Maybe it is OK, but it somehow seems like the new logic has been > > jammed into the get_loadable_libraries() function for coding > > convenience. For example, all the names (function names, variable > > names, structure field names) are referring to "libraries", so the > > plugin seems a bit out of place. > > > > But the same name library (as plugin) should exist for the upgrade of > slots. I feel doing it separately could either lead to a redundant > code or a different way to achieve the same thing. Do you envision any > problem which we are not seeing? > No problem. I'd misunderstood that the "plugin" referred to here is a shared object file (aka library) name, so it does belong here after all. I think the new comments could be made more clear about this point though. -- Kind Regards, Peter Smith. Fujitsu Australia
Re: Extract numeric filed in JSONB more effectively
> because as long as it has parameters declared internal,
> no SQL can ever call it.
I was confused about the difference between anyelement and
internal, and I want to know a way to create a function which
is disallowed to be called by the user. Your above words
resolved two questions of mine!
So it can only appear in an expression
> tree because your SupportRequestSimplify put it there properly
> typed, after the SQL query was parsed but before evaluation.
>
> The thing about 'internal' is it doesn't represent any specific
> type, it doesn't necessarily represent the same type every time it
> is mentioned, and it often means something that isn't a cataloged
> type at all, such as a pointer to some kind of struct.
I should have noticed this during the study planner support function,
but highlighting this is pretty amazing.
> If there isn't, one might have to be invented. So it might be that
> if we go down the "use polymorphic resolution" road, we have to
> invent dummy Consts, and down the "internal" road we also have to
> invent something.
I think you might already feel that putting an internal function
into an expression would cause something wrong. I just have
a quick hack on this, and crash happens at the simplest case.
If something already exists to fix this, I am inclined
to use 'internal', but I didn't find the way. I'm thinking if we
should clarify "internal" should only be used internally and
should never be used in expression by design?
> (And I'm not even sure anything has to be invented. If there's an
> existing node for no-op binary casts, I think I'd first try
> putting that there and see if anything complains.)
>
> If this thread is being followed by others more familiar with
> the relevant code or who see obvious problems I'm missing,
> please chime in!
>
Thank you wise & modest gentleman, I would really hope Tom can
chime in at this time.
In general, the current decision we need to make is shall we use
'internal' or 'anyelement' to present the target OID. the internal way
would be more straight but have troubles to be in the expression tree.
the 'anyelement' way is compatible with expression, but it introduces
the makeDummyConst overhead and I'm not pretty sure it is a correct
implementation in makeDummyConst. see the XXX part.
+/*
+ * makeDummyConst
+ * create a Const node with the specified type/typmod.
+ *
+ * This is a convenience routine to create a Const which only the
+ * type is interested but make sure the value is accessible.
+ */
+Const *
+makeDummyConst(Oid consttype, int32 consttypmod, Oid constcollid)
+{
+ int16 typLen;
+ booltypByVal;
+ Const *c;
+ Datum val = 0;
+
+
+ get_typlenbyval(consttype, &typLen, &typByVal);
+
+ if (consttype == NUMERICOID)
+ val = DirectFunctionCall1(numeric_in, CStringGetDatum("0"));
+ else if (!typByVal)
+ elog(ERROR, "create dummy const for type %u is not
supported.", consttype);
+
+ /* XXX: here I assume constvalue=0 is accessible for const by value
type.*/
+ c = makeConst(consttype, consttypmod, 0, (int) typLen, val, false,
typByVal);
+
+ return c;
+}
--
Best Regards
Andy Fan
0001-convert-anyelement-to-internal.patch
Description: Binary data
v9-0001-optimize-casting-jsonb-to-a-given-type.patch
Description: Binary data
RE: Adding a LogicalRepWorker type field
On Friday, August 18, 2023 11:20 AM Amit Kapila wrote:
>
> On Mon, Aug 14, 2023 at 12:08 PM Peter Smith
> wrote:
> >
> > The main patch for adding the worker type enum has been pushed [1].
> >
> > Here is the remaining (rebased) patch for changing some previous
> > cascading if/else to switch on the LogicalRepWorkerType enum instead.
> >
>
> I see this as being useful if we plan to add more worker types. Does anyone
> else
> see this remaining patch as an improvement?
+1
I have one comment for the new error message.
+ case WORKERTYPE_UNKNOWN:
+ ereport(ERROR,
errmsg_internal("should_apply_changes_for_rel: Unknown worker type"));
I think reporting an ERROR in this case is fine. However, I would suggest
refraining from mentioning the function name in the error message, as
recommended in the error style document [1]. Also, it appears we could use
elog() here.
[1] https://www.postgresql.org/docs/devel/error-style-guide.html
Best Regards,
Hou zj
Re: Remove distprep
Re: Michael Paquier > This one comes down to Debian that patches autoconf with its own set > of options, requiring a new ./configure in the tree, right? Yes, mostly. Since autoconf had not seen a new release for so long, everyone started to patch it, and one of the things that Debian and others added was --runstatedir=DIR. The toolchain is also using it, it's part of the default set of options used by dh_auto_configure. In parallel, the standard debhelper toolchain also started to run autoreconf by default, so instead of telling dh_auto_configure to omit --runstatedir, it was really easier to patch configure.ac to remove the autoconf 2.69 check. Two of the other patches are touching configure(.ac) anyway to tweak compiler flags (reproducibility, aarch64 tweaks). Christoph
Re: WIP: new system catalog pg_wait_event
Hi, On 8/17/23 9:37 AM, Masahiro Ikeda wrote: Hi, On 2023-08-17 14:53, Drouvot, Bertrand wrote: The followings are additional comments for v7. 1) I am not sure that "pg_wait_event" is a good idea for the name if the new view. How about "pg_wait_events" instead, in plural form? There is more than one wait event listed. I'd prefer the singular form. There is a lot of places where it's already used (pg_database, pg_user, pg_namespace...to name a few) and it looks like that using the plural form are exceptions. Since I got the same feeling as Michael-san that "pg_wait_events" would be better, I check the names of all system views. I think the singular form seems to be exceptions for views though the singular form is used usually for system catalogs. https://www.postgresql.org/docs/devel/views.html https://www.postgresql.org/docs/devel/catalogs.html Okay, using the plural form in v8 attached. 2) $ctmp must be $wctmp? + rename($wctmp, "$output_path/wait_event_funcs_data.c") + || die "rename: $ctmp to $output_path/wait_event_funcs_data.c: $!"; Nice catch, thanks, fixed in v8. 3) "List all the wait events type, name and descriptions" is enough? + * List all the wait events (type, name) and their descriptions. Agree, used "List all the wait events type, name and description" in v8 (using description in singular as compared to your proposal) 4) Why don't you add the usage of the view in the monitoring.sgml? ``` Here is an example of how wait events can be viewed: SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event is NOT NULL; pid | wait_event_type | wait_event --+-+ 2540 | Lock | relation 6644 | LWLock | ProcArray (2 rows) ``` ex. postgres(3789417)=# SELECT a.pid, a.wait_event_type, a.wait_event, w.description FROM pg_stat_activity a JOIN pg_wait_event w ON (a.wait_event_type = w.type AND a.wait_event = w.name) WHERE wait_event is NOT NULL; Agree, I think that's a good idea. I'd prefer to add also a filtering on "state='active'" for this example (I think that would provide a "real" use case as the sessions are not waiting if they are not active). Done in v8. 5) Though I'm not familiar the value, do we need procost? I think it makes sense to add a "small" one as it's done. BTW, while looking at it, I changed prorows to a more "accurate" value. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.comFrom 027a83d37d52eee52e3c82bcf814c6d5949f7ccf Mon Sep 17 00:00:00 2001 From: bdrouvotAWS Date: Sat, 5 Aug 2023 12:39:42 + Subject: [PATCH v8] Add catalog pg_wait_events Adding a new system view, namely pg_wait_events, that describes the wait events. --- doc/src/sgml/monitoring.sgml | 13 ++- doc/src/sgml/system-views.sgml| 64 + src/backend/Makefile | 3 +- src/backend/catalog/system_views.sql | 3 + src/backend/utils/activity/.gitignore | 1 + src/backend/utils/activity/Makefile | 8 +- .../activity/generate-wait_event_types.pl | 54 ++- src/backend/utils/activity/meson.build| 1 + src/backend/utils/activity/wait_event.c | 44 + src/backend/utils/activity/wait_event_funcs.c | 93 +++ src/include/catalog/pg_proc.dat | 6 ++ src/include/utils/meson.build | 4 +- src/include/utils/wait_event.h| 1 + .../modules/worker_spi/t/001_worker_spi.pl| 6 ++ src/test/regress/expected/rules.out | 4 + src/test/regress/expected/sysviews.out| 16 src/test/regress/sql/sysviews.sql | 4 + src/tools/msvc/Solution.pm| 3 +- src/tools/msvc/clean.bat | 1 + 19 files changed, 318 insertions(+), 11 deletions(-) create mode 100644 src/backend/utils/activity/wait_event_funcs.c diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 70511a2388..bfa658f7a8 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -1103,7 +1103,7 @@ postgres 27093 0.0 0.0 30096 2752 ?Ss 11:34 0:00 postgres: ser &wait_event_types; - Here is an example of how wait events can be viewed: + Here is are examples of how wait events can be viewed: SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event is NOT NULL; @@ -1112,6 +1112,17 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i 2540 | Lock| relation 6644 | LWLock | ProcArray (2 rows) + + + +SELECT a.pid, a.wait_event, w.description +FROM pg_stat_activity a JOIN pg_wait_events w +ON (a.wait_event_type = w.type AND a.wait_event = w.name) +WHERE wait_event is NOT NULL and a.state = 'active'; +-[ RECORD 1 ]
Re: WIP: new system catalog pg_wait_event
Hi, On 8/18/23 12:37 AM, Michael Paquier wrote: On Thu, Aug 17, 2023 at 04:37:22PM +0900, Masahiro Ikeda wrote: On 2023-08-17 14:53, Drouvot, Bertrand wrote: BTW, is it better to start with "Waiting" like any other lines? For example, "Waiting for custom event \"worker_spi_main\" defined by an extension". Using "Waiting for custom event" is a good term here. Yeah, done in v8 just shared up-thread. I am wondering if this should be s/extension/module/, as an event could be set by a loaded library, which may not be set with CREATE EXTENSION. I think that removing "extension" sounds weird given the fact that the wait event type is "Extension" (that could lead to confusion). I did use "defined by an extension module" in v8 (also that's aligned with the wording used in wait_event.h). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
[dsm] comment typo
In the following sentence, I believe either 'the' or 'a' should be kept, not both. I here keep the 'the', but feel free to change. --- src/backend/storage/ipc/dsm_impl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c index 6399fa2ad5..19a9cfc8ac 100644 --- a/src/backend/storage/ipc/dsm_impl.c +++ b/src/backend/storage/ipc/dsm_impl.c @@ -137,7 +137,7 @@ int min_dynamic_shared_memory; * Arguments: * op: The operation to be performed. * handle: The handle of an existing object, or for DSM_OP_CREATE, the - *a new handle the caller wants created. + *new handle the caller wants created. * request_size: For DSM_OP_CREATE, the requested size. Otherwise, 0. * impl_private: Private, implementation-specific data. Will be a pointer *to NULL for the first operation on a shared memory segment within this -- -- Regards Junwang Zhao
PostgreSQL 17 alpha RPMs
Hi hackers, Just a FYI: Started building daily snapshot RPMs using the tarball at: https://download.postgresql.org/pub/snapshot/dev/postgresql-snapshot.tar.bz2 Packages are available on these platforms: * RHEL 9 (x86_64 & aarch64) * RHEL 8 (x86_64, aarch64 and ppc64le) * Fedora 38 (x86_64) * Fedora 37 (x86_64) * SLES 15 These alpha packages are built with compile options (--enable-debug -- enable-cassert) which have significant negative effect on performance, so packages are not useful for performance testing. Please use latest repo rpm for Fedora, RHEL/Rocky from: https://yum.postgresql.org/repopackages/ and run: dnf config-manager --set-enabled pgdg17-updates-testing to enable v17 daily repos. For SLES 15, run: zypper addrepo https://download.postgresql.org/pub/repos/zypp/repo/pgdg-sles-15-pg17-devel.repo and edit repo file to enable v17 repo. -HTH Regards, -- Devrim Gündüz Open Source Solution Architect, PostgreSQL Major Contributor Twitter: @DevrimGunduz , @DevrimGunduzTR
Re: WIP: new system catalog pg_wait_event
On Fri, Aug 18, 2023 at 10:57:28AM +0200, Drouvot, Bertrand wrote: > I did use "defined by an extension module" in v8 (also that's aligned with > the wording used in wait_event.h). WFM. -- Michael signature.asc Description: PGP signature
Re: WIP: new system catalog pg_wait_event
On Fri, Aug 18, 2023 at 10:56:55AM +0200, Drouvot, Bertrand wrote: > Okay, using the plural form in v8 attached. Noting in passing: - Here is an example of how wait events can be viewed: + Here is are examples of how wait events can be viewed: s/is are/are/. -- Michael signature.asc Description: PGP signature
Re: Ignore 2PC transaction GIDs in query jumbling
Michael Paquier writes:
> On Tue, Aug 15, 2023 at 08:49:37AM +0900, Michael Paquier wrote:
>> Hmm. One issue with the patch is that we finish by considering
>> DEALLOCATE ALL and DEALLOCATE $1 as the same things, compiling the
>> same query IDs. The difference is made in the Nodes by assigning NULL
>> to the name but we would now ignore it. Wouldn't it be better to add
>> an extra field to DeallocateStmt to track separately the named
>> deallocate queries and ALL in monitoring?
>
> In short, I would propose something like that, with a new boolean
> field in DeallocateStmt that's part of the jumbling.
>
> Dagfinn, Julien, what do you think about the attached?
I don't have a particularly strong opinion on whether we should
distinguish DEALLOCATE ALL from DEALLOCATE (call it +0.5), but
this seems like a reasonable way to do it unless we want to invent a
query_jumble_ignore_unless_null attribute (which seems like overkill for
this one case).
- ilmari
> Michael
>
> From ad91672367f408663824b36b6dd517513d7f0a2a Mon Sep 17 00:00:00 2001
> From: Michael Paquier
> Date: Fri, 18 Aug 2023 09:12:58 +0900
> Subject: [PATCH v2] Track DEALLOCATE statements in pg_stat_activity
>
> Treat the statement name as a constant when jumbling. A boolean field
> is added to DeallocateStmt to make a distinction between the ALL and
> named cases.
> ---
> src/include/nodes/parsenodes.h| 8 +++-
> src/backend/parser/gram.y | 8
> .../pg_stat_statements/expected/utility.out | 41 +++
> .../pg_stat_statements/pg_stat_statements.c | 8 +---
> contrib/pg_stat_statements/sql/utility.sql| 13 ++
> 5 files changed, 70 insertions(+), 8 deletions(-)
>
> diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
> index 2565348303..2b356336eb 100644
> --- a/src/include/nodes/parsenodes.h
> +++ b/src/include/nodes/parsenodes.h
> @@ -3925,8 +3925,12 @@ typedef struct ExecuteStmt
> typedef struct DeallocateStmt
> {
> NodeTag type;
> - char *name; /* The name of the plan to
> remove */
> - /* NULL means DEALLOCATE ALL */
> + /* The name of the plan to remove. NULL if DEALLOCATE ALL. */
> + char *name pg_node_attr(query_jumble_ignore);
> + /* true if DEALLOCATE ALL */
> + boolisall;
> + /* token location, or -1 if unknown */
> + int location pg_node_attr(query_jumble_location);
> } DeallocateStmt;
>
> /*
> diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
> index b3bdf947b6..df34e96f89 100644
> --- a/src/backend/parser/gram.y
> +++ b/src/backend/parser/gram.y
> @@ -11936,6 +11936,8 @@ DeallocateStmt: DEALLOCATE name
> DeallocateStmt *n =
> makeNode(DeallocateStmt);
>
> n->name = $2;
> + n->isall = false;
> + n->location = @2;
> $$ = (Node *) n;
> }
> | DEALLOCATE PREPARE name
> @@ -11943,6 +11945,8 @@ DeallocateStmt: DEALLOCATE name
> DeallocateStmt *n =
> makeNode(DeallocateStmt);
>
> n->name = $3;
> + n->isall = false;
> + n->location = @3;
> $$ = (Node *) n;
> }
> | DEALLOCATE ALL
> @@ -11950,6 +11954,8 @@ DeallocateStmt: DEALLOCATE name
> DeallocateStmt *n =
> makeNode(DeallocateStmt);
>
> n->name = NULL;
> + n->isall = true;
> + n->location = -1;
> $$ = (Node *) n;
> }
> | DEALLOCATE PREPARE ALL
> @@ -11957,6 +11963,8 @@ DeallocateStmt: DEALLOCATE name
> DeallocateStmt *n =
> makeNode(DeallocateStmt);
>
> n->name = NULL;
> + n->isall = true;
> + n->location = -1;
> $$ = (Node *) n;
> }
> ;
> diff --git a/contrib/pg_stat_statements/expected/utility.out
> b/contrib/pg_stat_statements/expected/utility.out
> index 93735d5d85..f331044f3e 100644
> --- a/contrib/pg_stat_statements/expected/utility.out
> +++ b/contrib/pg_stat_statements/expected/utility.out
> @@ -472,6
Re: logical decoding and replication of sequences, take 2
On Fri, Aug 18, 2023 at 10:37 AM Amit Kapila wrote:
>
> On Thu, Aug 17, 2023 at 7:13 PM Ashutosh Bapat
> wrote:
> >
> > On Wed, Aug 16, 2023 at 7:56 PM Tomas Vondra
> > wrote:
> > >
> > > >
> > > > But whether or not that's the case, downstream should not request (and
> > > > hence receive) any changes that have been already applied (and
> > > > committed) downstream as a principle. I think a way to achieve this is
> > > > to update the replorigin_session_origin_lsn so that a sequence change
> > > > applied once is not requested (and hence sent) again.
> > > >
> > >
> > > I guess we could update the origin, per attached 0004. We don't have
> > > timestamp to set replorigin_session_origin_timestamp, but it seems we
> > > don't need that.
> > >
> > > The attached patch merges the earlier improvements, except for the part
> > > that experimented with adding a "fake" transaction (which turned out to
> > > have a number of difficult issues).
> >
> > 0004 looks good to me.
>
>
> + {
> CommitTransactionCommand();
> +
> + /*
> + * Update origin state so we don't try applying this sequence
> + * change in case of crash.
> + *
> + * XXX We don't have replorigin_session_origin_timestamp, but we
> + * can just leave that set to 0.
> + */
> + replorigin_session_origin_lsn = seq.lsn;
>
> IIUC, your proposal is to update the replorigin_session_origin_lsn, so
> that after restart, it doesn't use some prior origin LSN to start with
> which can in turn lead the sequence to go backward. If so, it should
> be updated before calling CommitTransactionCommand() as we are doing
> in apply_handle_commit_internal(). If that is not the intention then
> it is not clear to me how updating replorigin_session_origin_lsn after
> commit is helpful.
>
typedef struct ReplicationState
{
...
/*
* Location of the latest commit from the remote side.
*/
XLogRecPtrremote_lsn;
This is the variable that will be updated with the value of
replorigin_session_origin_lsn. This means we will now track some
arbitrary LSN location of the remote side in this variable. The above
comment makes me wonder if there is anything we are missing or if it
is just a matter of updating this comment because before the patch we
always adhere to what is written in the comment.
--
With Regards,
Amit Kapila.
Re: Allow parallel plan for referential integrity checks?
On Thu, Aug 17, 2023 at 3:51 PM Frédéric Yhuel wrote: > On 8/17/23 14:00, Frédéric Yhuel wrote: > > On 8/17/23 09:32, Frédéric Yhuel wrote: > >> On 8/10/23 17:06, Juan José Santamaría Flecha wrote: > >>> Recently I restored a database from a directory format backup and > >>> having this feature would have been quite useful > >> > >> Thanks for resuming work on this patch. I forgot to mention this in my > >> original email, but the motivation was also to speed up the restore > >> process. Parallelizing the FK checks could make a huge difference in > >> certain cases. We should probably provide such a test case (with perf > >> numbers), and maybe this is it what Robert asked for. > > > > I have attached two scripts which demonstrate the following problems: > Thanks for the scripts, but I think Robert's concerns come from the safety, and not the performance, of the parallel operation. Proving its vulnerability could be easy with a counter example, but assuring its safety is trickier. What test would suffice to do that? Regards, Juan José Santamaría Flecha
Re: CREATE FUNCTION ... SEARCH { DEFAULT | SYSTEM | SESSION }
On 16.08.23 19:44, Jeff Davis wrote: On Wed, 2023-08-16 at 08:51 +0200, Peter Eisentraut wrote: On 12.08.23 04:35, Jeff Davis wrote: The attached patch implements a new SEARCH clause for CREATE FUNCTION. The SEARCH clause controls the search_path used when executing functions that were created without a SET clause. I don't understand this. This adds a new option for cases where the existing option wasn't specified. Why not specify the existing option then? Is it not good enough? Can we improve it? SET search_path = '...' not good enough in my opinion. 1. Not specifying a SET clause falls back to the session's search_path, which is a bad default because it leads to all kinds of inconsistent behavior and security concerns. Not specifying SEARCH would have the same issue? 2. There's no way to explicitly request that you'd actually like to use the session's search_path, so it makes it very hard to ever change the default. That sounds like something that should be fixed independently. I could see this being useful for other GUC settings, like I want to run a function explicitly with the session's work_mem. 3. It's user-unfriendly. A safe search_path that would be suitable for most functions is "SET search_path = pg_catalog, pg_temp", which is arcane, and requires some explanation. True, but is that specific to functions? Maybe I want a safe search_path just in general, for a session or something. 4. search_path for the session is conceptually different than for a function. A session should be context-sensitive and the same query should (quite reasonably) behave differently for different sessions and users to sort out things like object name conflicts, etc. A function should (ordinarily) be context-insensitive, especially when used in something like an index expression or constraint. Having different syntax helps separate those concepts. I'm not sure I follow that. When you say a function should be context-insensitive, you could also say, a function should be context-sensitive, but have a separate context. Which is kind of how it works now. Maybe not well enough. 5. There's no way to prevent pg_temp from being included in the search_path. This is separately fixable, but having the proposed SEARCH syntax is likely to make for a better user experience in the common cases. seems related to #3 I'm open to suggestion about other ways to improve it, but SEARCH is what I came up with. Some extensions of the current mechanism, like search_path = safe, search_path = session, search_path = inherit, etc. might work.
RE: [PoC] pg_upgrade: allow to upgrade publisher node
Dear Amit, > I was thinking whether we can go a step ahead and remove this variable > altogether. In old cluster handling, we can get and check together at > the same place and for the new cluster, if we have a function that > returns slot_count by traversing old clusterinfo that should be > sufficient. If you have other better ideas to eliminate this variable > that is also fine. I think this will make the patch bit clean w.r.t > this new variable. Seems better, removed the variable. Also, the timing of checks were changed to the end of get_logical_slot_infos(). The check whether we are in live_check are moved to the function, so the argument was removed again. The whole of changes can be checked in upcoming e-mail. Best Regards, Hayato Kuroda FUJITSU LIMITED
RE: [PoC] pg_upgrade: allow to upgrade publisher node
Dear Hou, Thank you for reviewing! > +static void > +create_logical_replication_slots(void) > ... > + query = createPQExpBuffer(); > + escaped = createPQExpBuffer(); > + conn = connectToServer(&new_cluster, old_db->db_name); > > Since the connection here is not used anymore, so I think we can remove it. Per discussion [1], pg_upgrade must use connection again. So I kept it. > 2. > > +static void > +create_logical_replication_slots(void) > ... > + /* update new_cluster info again */ > + get_logical_slot_infos(&new_cluster); > +} > > Do we need to get new slots again after restoring ? I checked again and thought that it was not needed, removed. Similar function, create_new_objects(), was updated the information at the end. This was needed because the information was used to compare objects between old and new cluster, in transfer_all_new_tablespaces(). In terms of logical replication slots, however, such comparison was not done. No functions use updated information. > 3. > > + snprintf(query, sizeof(query), > + "SELECT slot_name, plugin, two_phase " > + "FROM pg_catalog.pg_replication_slots " > + "WHERE database = current_database() AND > temporary = false " > + "AND wal_status <> 'lost';"); > + > + res = executeQueryOrDie(conn, "%s", query); > + > > Instead of building the query in a new variable, can we directly put the SQL > in > executeQueryOrDie() > e.g. > executeQueryOrDie(conn, "SELECT slot_name, plugin, two_phase ..."); Right, fixed. > 4. > +int num_slots_on_old_cluster; > > Instead of a new global variable, would it be better to record this in the > cluster > info ? Per suggestion [2], the variable was removed. > 5. > > charsql_file_name[MAXPGPATH], > log_file_name[MAXPGPATH]; > + > DbInfo *old_db = &old_cluster.dbarr.dbs[dbnum]; > > There is an extra change here. Removed. > 6. > + for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++) > .. > + /* reap all children */ > + while (reap_child(true) == true) > + ; > + } > > Maybe we can move the "while (reap_child(true) == true)" out of the for() > loop ? Per discussion [1], I stopped to do in parallel. So this part was not needed anymore. The patch would be available in upcoming posts. [1]: https://www.postgresql.org/message-id/TYCPR01MB58701DAEE5E61B07AC84ADBBF51AA%40TYCPR01MB5870.jpnprd01.prod.outlook.com [2]: https://www.postgresql.org/message-id/TYAPR01MB5866691219B9CB280B709600F51BA%40TYAPR01MB5866.jpnprd01.prod.outlook.com Best Regards, Hayato Kuroda FUJITSU LIMITED
RE: [PoC] pg_upgrade: allow to upgrade publisher node
Dear Amit,
>
> Few minor comments
> 1. Why the patch updates the slots info at the end of
> create_logical_replication_slots()? Can you please update the comments
> for the same?
I checked and agreed that it was not needed. More detail, please see [1].
> 2.
> @@ -36,6 +36,7 @@ generate_old_dump(void)
> {
> char sql_file_name[MAXPGPATH],
> log_file_name[MAXPGPATH];
> +
> DbInfo*old_db = &old_cluster.dbarr.dbs[dbnum];
>
> Spurious line change.
>
Removed.
Next patch set would be available in upcoming posts.
[1]:
https://www.postgresql.org/message-id/TYAPR01MB5866F384AC62E12E9638BEC1F51BA%40TYAPR01MB5866.jpnprd01.prod.outlook.com
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
RE: [PoC] pg_upgrade: allow to upgrade publisher node
Dear Peter, Thanks for reviewing! > For patch v21-0001... > > == > 1. SaveSlotToPath > > - /* and don't do anything if there's nothing to write */ > - if (!was_dirty) > + /* > + * and don't do anything if there's nothing to write, unless it's this is > + * called for a logical slot during a shutdown checkpoint, as we want to > + * persist the confirmed_flush_lsn in that case, even if that's the only > + * modification. > + */ > + if (!was_dirty && (SlotIsPhysical(slot) || !is_shutdown)) > return; > > The condition seems to be coded in a slightly awkward way when > compared to how the comment was worded. > > How about: > if (!was_dirty && !(SlotIsLogical(slot) && is_shutdown)) Changed. > For patch v21-0002... > > == > Commit Message > > 1. > > For pg_upgrade, it query the logical replication slots information from the > old > cluter and restores the slots using the pg_create_logical_replication_slots() > statements. Note that we need to separate the timing of restoring replication > slots and other objects. Replication slots, in particular, should not be > restored before executing the pg_resetwal command because it will remove WALs > that are required by the slots. > > ~ > > Revisit this paragraph. There are lots of typos etc. Maybe I sent the patch before finalizing the commit message. Sorry for that. I reworded the part. Grammarly says OK the new part. > 1a. > "For pg_upgrade". I think this wording is a hangover from back when > the patch was split into two parts for pg_dump and pg_upgrade, but now > it seems strange. Yeah, so removed the word. > 1b. > /cluter/cluster/ Changed. > 1c > /because it/because pg_resetwal/ Changed. > src/sgml/ref/pgupgrade.sgml > > 2. > > + > +Prepare for publisher upgrades > + > + > + pg_upgrade try to dump and restore logical > + replication slots. This helps avoid the need for manually defining the > + same replication slot on the new publisher. > + > + > > 2a. > /try/attempts to/ ?? Changed. > 2b. > Is "dump" the right word here? I didn't see dumping happening in the > patch anymore. I replaced "dump and restore" to " migrate". How do you think? > 3. > > + > + Before you start upgrading the publisher node, ensure that the > + subscription is temporarily disabled. After the upgrade is complete, > + execute the > + ALTER > SUBSCRIPTION ... DISABLE > + command to update the connection string, and then re-enable the > + subscription. > + > > 3a. > That link made no sense in this context. > > Don't you mean to say: > ALTER SUBSCRIPTION ... CONNECTION ... > 3b. > Hmm. I wonder now did you *also* mean to describe how to disable? For example: > > Before you start upgrading the publisher node, ensure that the > subscription is temporarily disabled, by executing > ALTER SUBSCRIPTION ... > DISABLE. I wondered which statement should be referred, and finally did incompletely. Both of ALTER SUBSCRIPTION statements was cited, and a link was added to DISABLE clause. Is it OK? > 4. > > + > + > + Upgrading slots has some settings. At first, all the slots must not be > in > + lost, and they must have consumed all the WALs on old > + node. Furthermore, new node must have larger > + linkend="guc-max-replication-slots">max_replication_slots me> > + than existing slots on old node, and > + wal_level > must be > + logical. pg_upgrade will > + run error if something wrong. > + > + > + > > 4a. > "At first, all the slots must not be in lost" > > Apart from being strangely worded, I was not familiar with what it > meant to say "must not be in lost". Will this be meaningful to the > user? > > IMO this should have more description, e.g. including mentioning the > "wal_status" attribute with the appropriate link to > https://www.postgresql.org/docs/current/view-pg-replication-slots.html Added the reference. > 4b. > BEFORE > Upgrading slots has some settings. ... > pg_upgrade will run error if something > wrong. > > SUGGESTION > There are some prerequisites for pg_upgrade > to be able to upgrade the replication slots. If these are not met an > error will be reported. Changed. > 4c. > Wondered if this list of prerequisites might be better presented as an > SGML list. Changed to style. > src/bin/pg_upgrade/check.c > > 5. > extern char *output_files[]; > +extern int num_slots_on_old_cluster; > > ~ > > IMO something feels not quite right about having this counter floating > around as a global variable. > > Shouldn't this instead be a field member of the old_cluster. That > seems to be the normal way to hold the cluster-wise info. Per comment from Amit, the variable was removed. > 6. check_new_cluster_is_empty > > RelInfoArr *rel_arr = &new_cluster.dbarr.dbs[dbnum].rel_arr; > + DbInfo *pDbInfo = &new_cluster.dbarr.dbs[dbnum]; > + LogicalSlotInfoArr *slot_arr = &pDbInfo->slot_arr; > > IIRC I previously suggested adding this 'pDbInfo' va
RE: [PoC] pg_upgrade: allow to upgrade publisher node
Dear Peter,
PSA new version patch set.
> Here are some review comments for the patch v21-0003
>
> ==
> Commit message
>
> 1.
> pg_upgrade fails if the old node has slots which status is 'lost' or they do
> not
> consume all WAL records. These are needed for prevent the data loss.
>
> ~
>
> Maybe some minor brush-up like:
>
> SUGGESTION
> In order to prevent data loss, pg_upgrade will fail if the old node
> has slots with the status 'lost', or with unconsumed WAL records.
Improved.
> src/bin/pg_upgrade/check.c
>
> 2. check_for_confirmed_flush_lsn
>
> + /* Check that all logical slots are not in 'lost' state. */
> + res = executeQueryOrDie(conn,
> + "SELECT slot_name FROM pg_catalog.pg_replication_slots "
> + "WHERE temporary = false AND wal_status = 'lost';");
> +
> + ntups = PQntuples(res);
> + i_slotname = PQfnumber(res, "slot_name");
> +
> + for (i = 0; i < ntups; i++)
> + {
> + is_error = true;
> +
> + pg_log(PG_WARNING,
> +"\nWARNING: logical replication slot \"%s\" is obsolete.",
> +PQgetvalue(res, i, i_slotname));
> + }
> +
> + PQclear(res);
> +
> + if (is_error)
> + pg_fatal("logical replication slots not to be in 'lost' state.");
> +
>
> 2a. (GENERAL)
> The above code for checking lost state seems out of place in this
> function which is meant for checking confirmed flush lsn.
>
> Maybe you jammed both kinds of logic into one function to save on the
> extra PGconn or something but IMO two separate functions would be
> better. e.g.
> - check_for_lost_slots
> - check_for_confirmed_flush_lsn
Separated into check_for_lost_slots and check_for_confirmed_flush_lsn.
> 2b.
> + /* Check that all logical slots are not in 'lost' state. */
>
> SUGGESTION
> /* Check there are no logical replication slots with a 'lost' state. */
Changed.
> 2c.
> + res = executeQueryOrDie(conn,
> + "SELECT slot_name FROM pg_catalog.pg_replication_slots "
> + "WHERE temporary = false AND wal_status = 'lost';");
>
> This SQL fragment is very much like others in previous patches. Be
> sure to make all the cases and clauses consistent with all those
> similar SQL fragments.
Unified the order. Note that they could not be the completely the same.
> 2d.
> + is_error = true;
>
> That doesn't need to be in the loop. Better to just say:
> is_error = (ntups > 0);
Removed the variable.
> 2e.
> There is a mix of terms in the WARNING and in the pg_fatal -- e.g.
> "obsolete" versus "lost". Is it OK?
Unified to 'lost'.
> 2f.
> + pg_fatal("logical replication slots not to be in 'lost' state.");
>
> English? And maybe it should be much more verbose...
>
> "Upgrade of this installation is not allowed because one or more
> logical replication slots with a state of 'lost' were detected."
I checked other pg_fatal() and the statement like "Upgrade of this installation
is not allowed"
could not be found. So I used later part.
> 3. check_for_confirmed_flush_lsn
>
> + /*
> + * Check that all logical replication slots have reached the latest
> + * checkpoint position (SHUTDOWN_CHECKPOINT record). This checks cannot
> be
> + * done in case of live_check because the server has not been written the
> + * SHUTDOWN_CHECKPOINT record yet.
> + */
> + if (!live_check)
> + {
> + res = executeQueryOrDie(conn,
> + "SELECT slot_name FROM pg_catalog.pg_replication_slots "
> + "WHERE confirmed_flush_lsn != '%X/%X' AND temporary = false;",
> + old_cluster.controldata.chkpnt_latest_upper,
> + old_cluster.controldata.chkpnt_latest_lower);
> +
> + ntups = PQntuples(res);
> + i_slotname = PQfnumber(res, "slot_name");
> +
> + for (i = 0; i < ntups; i++)
> + {
> + is_error = true;
> +
> + pg_log(PG_WARNING,
> +"\nWARNING: logical replication slot \"%s\" has not consumed WALs yet",
> +PQgetvalue(res, i, i_slotname));
> + }
> +
> + PQclear(res);
> + PQfinish(conn);
> +
> + if (is_error)
> + pg_fatal("All logical replication slots consumed all the WALs.");
>
> ~
>
> 3a.
> /This checks/This check/
The comment was no longer needed, because the caller checks live_check variable.
More detail, please see my another post [1].
> 3b.
> I don't think the separation of
> chkpnt_latest_upper/chkpnt_latest_lower is needed like this. AFAIK
> there is an LSN_FORMAT_ARGS(lsn) macro designed for handling exactly
> this kind of parameter substitution.
Fixed to use the macro.
Previously I considered that the header "access/xlogdefs.h" could not be
included
from pg_upgrade, and it was the reason why I did not use. But it seemed my
misunderstanding - I could include the file.
> 3c.
> + is_error = true;
>
> That doesn't need to be in the loop. Better to just say:
> is_error = (ntups > 0);
Removed.
> 3d.
> + pg_fatal("All logical replication slots consumed all the WALs.");
>
> The message seems backward. shouldn't it say something like:
> "Upgrade of this installation is not allowed because one or more
> logical replication slots still have unconsumed WAL records."
I used only later part, see above reply.
> src/bin/pg_upgrade/controldat
Re: pg_upgrade fails with in-place tablespace
On Wed, Aug 09, 2023 at 09:20 AM +0800, Michael Paquier wrote: > This does not really explain the reason why in-place tablespaces need > to be skipped (in short they don't need a separate creation or check > like the others in create_script_for_old_cluster_deletion because they > are part of the data folder). Anyway, the more I think about it, the > less excited I get about the need to support pg_upgrade with in-place > tablespaces, especially regarding the fact that the patch blindly > enforces allows_in_place_tablespaces, assuming that it is OK to do so. > So what about the case where one would want to be warned if these are > still laying around when doing upgrades? And what's the actual use > case for supporting that? There is something else that we could do > here: add a pre-run check to make pg_upgrade fail gracefully if we > find in-place tablespaces in the old cluster. I have implemented the changes you suggested in our previous discussion. I have added the necessary code to ensure that pg_upgrade fails gracefully with in-place tablespaces and reports a hint to let the check pass. Thank you for your guidance and support. Please review my latest patch. -- Best regards, Rui Zhao 0001-Fix-pg_upgrade-with-in-place-tablespaces.patch Description: Binary data
Re: should frontend tools use syncfs() ?
On Thu, Aug 17, 2023 at 12:50:31PM +0900, Michael Paquier wrote: > On Wed, Aug 16, 2023 at 08:17:05AM -0700, Nathan Bossart wrote: >> The patch does have the following note: >> >> +On Linux, syncfs may be used instead to ask the >> +operating system to synchronize the whole file systems that contain >> the >> +data directory, the WAL files, and each tablespace. >> >> Do you think that is sufficient, or do you think we should really clearly >> explain that you could end up calling syncfs() on the same file system a >> few times if your tablespaces are on the same disk? I personally feel >> like that'd be a bit too verbose for the already lengthy descriptions of >> this setting. > > It does not hurt to mention that the code syncfs()-es each tablespace > path (not in-place tablespaces), ignoring locations that share the > same mounting point, IMO. For that, we'd better rely on > get_dirent_type() like the normal sync path. But it doesn't ignore tablespace locations that share the same mount point. It simply calls syncfs() for each tablespace path, just like recovery_init_sync_method. >> If syncfs() is not available, SYNC_METHOD_SYNCFS won't even be defined, and >> parse_sync_method() should fail if "syncfs" is specified. Furthermore, the >> relevant part of fsync_pgdata() won't be compiled in whenever HAVE_SYNCFS >> is not defined. > > That feels structurally inconsistent with what we do with other > option sets that have library dependencies. For example, look at > compression.h and what happens for pg_compress_algorithm. So, it > seems to me that it would be more friendly to list SYNC_METHOD_SYNCFS > all the time in SyncMethod even if HAVE_SYNCFS is not around, and at > least generate a warning rather than having a platform-dependent set > of options? Done. > SyncMethod may be a bit too generic as name for the option structure. > How about a PGSyncMethod or pg_sync_method? In v4, I renamed this to DataDirSyncMethod and merged it with RecoveryInitSyncMethod. I'm not wedded to the name, but that seemed generic enough for both use-cases. As an aside, we need to be careful to distinguish these options from those for wal_sync_method. >> Yeah, this crossed my mind. Do you know of any existing examples of >> options with links to a common section? One problem with this approach is >> that there are small differences in the wording for some of the frontend >> utilities, so it might be difficult to cleanly unite these sections. > > The closest thing I can think of is Color Support in section > Appendixes, that describes something shared across a lot of binaries > (that would be 6 tools with this patch). If I added a "syncfs() Caveats" appendix for the common parts of the docs, it would only say something like the following: Using syncfs may be a lot faster than using fsync, because it doesn't need to open each file one by one. On the other hand, it may be slower if a file system is shared by other applications that modify a lot of files, since those files will also be written to disk. Furthermore, on versions of Linux before 5.8, I/O errors encountered while writing data to disk may not be reported to the calling program, and relevant error messages may appear only in kernel logs. Does that seem reasonable? It would reduce the duplication a little bit, but I'm not sure it's really much of an improvement in this case. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com >From 5edd4b4570617a897043086cf203af57a5596e11 Mon Sep 17 00:00:00 2001 From: Nathan Bossart Date: Fri, 28 Jul 2023 15:56:24 -0700 Subject: [PATCH v4 1/1] allow syncfs in frontend utilities --- doc/src/sgml/ref/initdb.sgml | 27 doc/src/sgml/ref/pg_basebackup.sgml | 30 + doc/src/sgml/ref/pg_checksums.sgml| 27 doc/src/sgml/ref/pg_dump.sgml | 27 doc/src/sgml/ref/pg_rewind.sgml | 27 doc/src/sgml/ref/pgupgrade.sgml | 29 + src/backend/storage/file/fd.c | 4 +- src/backend/utils/misc/guc_tables.c | 7 ++- src/bin/initdb/initdb.c | 12 +++- src/bin/pg_basebackup/pg_basebackup.c | 12 +++- src/bin/pg_checksums/pg_checksums.c | 9 ++- src/bin/pg_dump/pg_backup.h | 4 +- src/bin/pg_dump/pg_backup_archiver.c | 14 +++-- src/bin/pg_dump/pg_backup_archiver.h | 1 + src/bin/pg_dump/pg_backup_directory.c | 2 +- src/bin/pg_dump/pg_dump.c | 10 ++- src/bin/pg_rewind/file_ops.c | 2 +- src/bin/pg_rewind/pg_rewind.c | 9 +++ src/bin/pg_rewind/pg_rewind.h | 2 + src/bin/pg_upgrade/option.c | 13 src/bin/pg_upgrade/pg_upgrade.c | 6 +- src/bin/pg_upgrade/pg_upgrade.h | 1 + src/common/file_utils.c | 91 ++- src/fe_utils/option_utils.c | 24 +++ src/include/common/file_utils.h | 15 - src/in
Re: SLRUs in the main buffer pool - Page Header definitions
On Fri, Aug 18, 2023 at 08:12:41AM +, Bagga, Rishu wrote: > * Frost, Stephen (sfrowt(at)snowman(dot)net) wrote: >> Haven't really looked over the patches yet but I wanted to push back >> on this a bit- you're suggesting that we'd continue to maintain and >> update slru.c for the benefit of extensions which use it while none of >> the core code uses it? For how long? For my 2c, at least, I'd rather >> we tell extension authors that they need to update their code instead. >> There's reasons why we're moving the SLRUs into the main buffer pool >> and having page headers for them and using the existing page code to >> read/write them and extension authors should be eager to gain those >> advantages too. Not sure how much concern to place on extensions that >> aren't willing to adjust to changes like these. > > Thanks for your response. I proposed this version of the patch with the > idea to make the changes gradual, and to minimize disruption of existing > functionality, with the idea of eventually deprecating the SLRUs. If the > community is okay with completely removing the extensible SLRU > mechanism, we don't have any objection to it either. I think I agree with Stephen. We routinely make changes that require updates to extensions, and I doubt anyone is terribly wild about maintaining two SLRU systems for several years. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: pgbench - adding pl/pgsql versions of tests
I will address the comments here over this coming weekend. I think that in addition to current "tpc-b like" test we could also have more modern "tpc-c like" and "tpc-h like" tests And why not any other "* -like" from the rest of TPC-*, YCSP, sysbench, ... :) though maybe not as part of pg_bench but as extensions ? --- Hannu On Wed, Aug 16, 2023 at 10:06 AM Fabien COELHO wrote: > > > Hello Nathan, > > >> I'm unclear about what variety of scripts that could be provided given the > >> tables made available with pgbench. ISTM that other scenari would involve > >> both an initialization and associated scripts, and any proposal would be > >> bared because it would open the door to anything. > > > > Why's that? > > Just a wild guess based on 19 years of occasional contributions to pg and > pgbench in particular:-) > > > I'm not aware of any project policy that prohibits such enhancements to > > pgbench. > > Attempts in extending pgbench often fall under "you can do it outside (eg > with a custom script) so there is no need to put that in pgbench as it > would add to the maintenance burden with a weak benefit proven by the fact > that it is not there already". > > > It might take some effort to gather consensus on a proposal like this, > > but IMHO that doesn't mean we shouldn't try. > > Done it in the past. Probably will do it again in the future:-) > > > If the prevailing wisdom is that we shouldn't add more built-in scripts > > because there is an existing way to provide custom ones, then it's not > > clear that we should proceed with $SUBJECT, anyway. > > I'm afraid there is that argument. I do not think that this policy is good > wrt $SUBJECT, ISTM that having an easy way to test something with a > PL/pgSQL function would help promote the language by advertising/showing > the potential performance benefit (or not, depending). Just one function > would be enough for that. > > -- > Fabien.
Re: Extract numeric filed in JSONB more effectively
On 2023-08-18 03:41, Andy Fan wrote: I just have a quick hack on this, and crash happens at the simplest case. If I build from this patch, this test: SELECT (test_json -> 0)::int4, test_json -> 0 FROM test_jsonb WHERE json_type = 'scalarint'; fails like this: Program received signal SIGSEGV, Segmentation fault. convert_saop_to_hashed_saop_walker (node=0x17, context=0x0) at /var/tmp/nohome/pgbuildh/../postgresql/src/backend/optimizer/util/clauses.c:2215 2215if (IsA(node, ScalarArrayOpExpr)) (gdb) p node $1 = (Node *) 0x17 So the optimizer is looking at some node to see if it is a ScalarArrayOpExpr, but the node has some rather weird address. Or maybe it's not that weird. 0x17 is 23, and so is: select 'int4'::regtype::oid; oid - 23 See what happened? + int64 target_typ = fexpr->funcresulttype; ... + fexpr->args = list_insert_nth(fexpr->args, 0, (void *) target_typ); This is inserting the desired result type oid directly as the first thing in the list of fexpr's args. But at the time your support function is called, nothing is being evaluated yet. You are just manipulating a tree of expressions to be evaluated later, and you want fexpr's first arg to be an expression that will produce this type oid later, when it is evaluated. A constant would do nicely: + Const *target = makeConst( INTERNALOID, -1, InvalidOid, SIZEOF_DATUM, ObjectIdGetDatum(fexpr->funcresulttype), false, true); + fexpr->args = list_insert_nth(fexpr->args, 0, target); With that change, it doesn't segfault, but it does do this: ERROR: cast jsonb to type 0 is not allowed and that's because of this: + Oid targetOid = DatumGetObjectId(0); The DatumGetFoo(x) macros are for when you already have the Datum (it's x) and you know it's a Foo. So this is just setting targetOid to zero. When you want to get something from function argument 0 and you know that's a Foo, you use a PG_GETARG_FOO(argno) macro (which amounts to PG_GETARG_DATUM(argno) followed by DatumGetFoo. So, with + Oid targetOid = PG_GETARG_OID(0); SELECT (test_json -> 0)::int4, test_json -> 0 FROM test_jsonb WHERE json_type = 'scalarint'; int4 | ?column? --+-- 2 | 2 However, EXPLAIN is sad: ERROR: cannot display a value of type internal and that may be where this idea runs aground. Now, I was expecting something to complain about the result of jsonb_array_element_type, and that didn't happen. We rewrote a function that was supposed to be a cast to int4, and replaced it with a function returning internal, and evaluation happily just took that as the int4 that the next node expected. If something had complained about that, it might have been necessary to insert some new node above the internal-returning function to say the result was really int4. Notice there is a makeRelabelType() for that. (I had figured there probably was, but didn't know its exact name.) So it doesn't seem strictly necessary to do that, but it might make the EXPLAIN result look better (if EXPLAIN were made to work, of course). Now, my guess is EXPLAIN is complaining when it sees the Const of type internal, and doesn't know how to show that value. Perhaps makeRelabelType is the answer there, too: what if the Const has Oid type, so EXPLAIN can show it, and what's inserted as the function argument is a relabel node saying it's internal? Haven't tried that yet. Regards, -Chap
Re: Extract numeric filed in JSONB more effectively
On 2023-08-18 14:50, Chapman Flack wrote:
Now, my guess is EXPLAIN is complaining when it sees the Const
of type internal, and doesn't know how to show that value.
Perhaps makeRelabelType is the answer there, too: what if the
Const has Oid type, so EXPLAIN can show it, and what's inserted
as the function argument is a relabel node saying it's internal?
Simply changing the Const to be of type Oid makes EXPLAIN happy,
and nothing ever says "hey, why are you passing this oid for an
arg that wants internal?". This is without adding any relabel
nodes anywhere.
Seq Scan on pg_temp.test_jsonb
Output: pg_catalog.jsonb_array_element_type('23'::oid, test_json, 0),
(test_json -> 0)
Filter: (test_jsonb.json_type = 'scalarint'::text)
Nothing in that EXPLAIN output to make you think anything weird
was going on, unless you went and looked up jsonb_array_element_type
and saw that its arg0 isn't oid and its return type isn't int4.
But I don't know that adding relabel nodes wouldn't still be
the civilized thing to do.
Regards,
-Chap
Re: CREATE FUNCTION ... SEARCH { DEFAULT | SYSTEM | SESSION }
On Fri, 2023-08-18 at 14:25 +0200, Peter Eisentraut wrote: > > Not specifying SEARCH would have the same issue? Not specifying SEARCH is equivalent to SEARCH DEFAULT, and that gives us some control over what happens. In the proposed patch, a GUC determines whether it behaves like SEARCH SESSION (the default for compatibility reasons) or SEARCH SYSTEM (safer). > > 2. There's no way to explicitly request that you'd actually like to > > use > > the session's search_path, so it makes it very hard to ever change > > the > > default. > > That sounds like something that should be fixed independently. I > could > see this being useful for other GUC settings, like I want to run a > function explicitly with the session's work_mem. I'm confused about how this would work. It doesn't make sense to set a GUC to be the session value in postgresql.conf, because there's no session yet. And it doesn't really make sense in a top-level session, because it would just be a no-op (right?). It maybe makes sense in a function, but I'm still not totally clear on what that would mean. > > True, but is that specific to functions? Maybe I want a safe > search_path just in general, for a session or something. I agree this is a somewhat orthogonal problem and we should have a way to keep pg_temp out of the search_path entirely. We just need to agree on a string representation of a search path that omits pg_temp. One idea would be to have special identifiers "!pg_temp" and "!pg_catalog" that would cause those to be excluded entirely. > > I'm not sure I follow that. When you say a function should be > context-insensitive, you could also say, a function should be > context-sensitive, but have a separate context. Which is kind of how > it > works now. Maybe not well enough. For functions called from index expressions or constraints, you want the function's result to only depend on its arguments; otherwise you can easily violate a constraint or cause an index to return wrong results. You're right that there is some other context, like the database default collation, but (a) that's mostly nailed down; and (b) if it changes unexpectedly that also causes problems. > > I'm open to suggestion about other ways to improve it, but SEARCH > > is > > what I came up with. > > Some extensions of the current mechanism, like search_path = safe, > search_path = session, search_path = inherit, etc. might work. I had considered some new special names like this in search path, but I didn't come up with a specific proposal that I liked. Do you have some more details about how this would help get us to a safe default? Regards, Jeff Davis
Re: Extract numeric filed in JSONB more effectively
On 2023-08-18 15:08, Chapman Flack wrote:
But I don't know that adding relabel nodes wouldn't still be
the civilized thing to do.
Interestingly, when I relabel both places, like this:
Oid targetOid = fexpr->funcresulttype;
Const *target = makeConst(
OIDOID, -1, InvalidOid, sizeof(Oid),
ObjectIdGetDatum(targetOid), false, true);
RelabelType *rTarget = makeRelabelType((Expr *)target,
INTERNALOID, -1, InvalidOid, COERCE_IMPLICIT_CAST);
fexpr->funcid = new_func_id;
fexpr->args = opexpr->args;
fexpr->args = list_insert_nth(fexpr->args, 0, rTarget);
expr = (Expr *)makeRelabelType((Expr *)fexpr,
targetOid, -1, InvalidOid, COERCE_IMPLICIT_CAST);
}
PG_RETURN_POINTER(expr);
EXPLAIN looks like this:
Seq Scan on pg_temp.test_jsonb
Output: jsonb_array_element_type(('23'::oid)::internal, test_json,
0), (test_json -> 0)
Filter: (test_jsonb.json_type = 'scalarint'::text)
With COERCE_IMPLICIT_CAST both places, the relabeling of the
function result is invisible, but the relabeling of the argument
is visible.
With the second one changed to COERCE_EXPLICIT_CAST:
Seq Scan on pg_temp.test_jsonb
Output: (jsonb_array_element_type(('23'::oid)::internal, test_json,
0))::integer, (test_json -> 0)
Filter: (test_jsonb.json_type = 'scalarint'::text)
then both relabelings are visible.
I'm not sure whether one way is better than the other, or whether
it is even important to add the relabel nodes at all, as nothing
raises an error without them. As a matter of taste, it seems like
a good idea though.
Regards,
-Chap
[17] Special search_path names "!pg_temp" and "!pg_catalog"
The attached patch adds some special names to prevent pg_temp and/or pg_catalog from being included implicitly. This is a useful safety feature for functions that don't have any need to search pg_temp. The current (v16) recommendation is to include pg_temp last, which does add to the safety, but it's confusing to *include* a namespace when your intention is actually to *exclude* it, and it's also not completely excluding pg_temp. Although the syntax in the attached patch is not much friendlier, at least it's clear that the intent is to exclude pg_temp. Furthermore, it will be friendlier if we adopt the SEARCH SYSTEM syntax proposed in another thread[1]. Additionally, this patch adds a WARNING when creating a schema that uses one of these special names. Previously, there was no warning when creating a schema with the name "$user", which could cause confusion. [1] https://www.postgresql.org/message-id/flat/2710f56add351a1ed553efb677408e51b060e67c.ca...@j-davis.com -- Jeff Davis PostgreSQL Contributor Team - AWS From 79cb94b857f51dec5cbc24b3d46d2e58de92b5c0 Mon Sep 17 00:00:00 2001 From: Jeff Davis Date: Fri, 18 Aug 2023 14:21:16 -0700 Subject: [PATCH] Special names "!pg_temp" and "!pg_catalog" for search_path. These names act as markers preventing the implicit namespaces from being added to the search path. Functions often don't need to look in pg_temp, and in those cases it's safer to exclude pg_temp entirely. --- doc/src/sgml/config.sgml| 25 ++ doc/src/sgml/ref/create_function.sgml | 8 +++--- src/backend/catalog/namespace.c | 23 src/backend/commands/schemacmds.c | 13 + src/test/regress/expected/namespace.out | 35 + src/test/regress/sql/namespace.sql | 18 + 6 files changed, 101 insertions(+), 21 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 11251fa05e..451f9c7e94 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -8631,10 +8631,11 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv; The system catalog schema, pg_catalog, is always -searched, whether it is mentioned in the path or not. If it is -mentioned in the path then it will be searched in the specified -order. If pg_catalog is not in the path then it will -be searched before searching any of the path items. +searched, unless the special schema name +!pg_catalog is specified in the path. If it is +mentioned in the path then it will be searched in the specified order. +If pg_catalog is not in the path then it will be +searched before searching any of the path items. Likewise, the current session's temporary-table schema, -pg_temp_nnn, is always searched if it -exists. It can be explicitly listed in the path by using the -alias pg_temppg_temp. If it is not listed in the path then -it is searched first (even before pg_catalog). However, -the temporary schema is only searched for relation (table, view, -sequence, etc.) and data type names. It is never searched for -function or operator names. +pg_temp_nnn, is always +searched if it exists, unless the special schema name +!pg_temp is specified. It can be explicitly +listed in the path by using the alias +pg_temppg_temp. +If it is not listed in the path then it is searched first (even before +pg_catalog). However, the temporary schema is only +searched for relation (table, view, sequence, etc.) and data type +names. It is never searched for function or operator names. diff --git a/doc/src/sgml/ref/create_function.sgml b/doc/src/sgml/ref/create_function.sgml index 863d99d1fc..93aebdeb26 100644 --- a/doc/src/sgml/ref/create_function.sgml +++ b/doc/src/sgml/ref/create_function.sgml @@ -794,9 +794,8 @@ SELECT * FROM dup(42); Particularly important in this regard is the temporary-table schema, which is searched first by default, and is normally writable by anyone. A secure arrangement can be obtained -by forcing the temporary schema to be searched last. To do this, -write pg_temppg_tempsecuring functions as the last entry in search_path. -This function illustrates safe usage: +by specifying !pg_temp. This function illustrates +safe usage: CREATE FUNCTION check_password(uname TEXT, pass TEXT) @@ -811,8 +810,7 @@ BEGIN END; $$ LANGUAGE plpgsql SECURITY DEFINER --- Set a secure search_path: trusted schema(s), then 'pg_temp'. -SET search_path = admin, pg_temp; +SET search_path = admin, "!pg_temp"; This function's intention is to access a table admin.pwds. diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c index 0c679fbf94..6d8a9cad02 1
Re: Use of additional index columns in rows filtering
On Thu, Aug 17, 2023 at 4:29 PM Jeff Davis wrote: > On Wed, 2023-08-09 at 17:14 -0700, Peter Geoghegan wrote: > > + Index quals are better than equivalent index filters because bitmap > > index scans can only use index quals > > It seems there's consensus that: > > * Index Filters (Tomas's patch and the topic of this thread) are more > general, because they can work on any expression, e.g. 1/x, which can > throw an error if x=0. Agreed, but with one small caveat: they're not more general when it comes to bitmap index scans, which seem like they'll only ever be able to support index quals. But AFAICT that's the one and only exception. > * Index quals are more optimizable, because such operators are not > supposed to throw errors or have side effects, so we can evaluate them > before determining visibility. Right. Though there is a second reason: the index AM can use them to navigate the index more efficiently with true index quals. At least in the case of SAOPs, and perhaps in other cases in the future. > I wouldn't describe one as "better" than the other, but I assume you > meant "more optimizable". The use of the term "better" is actually descriptive of Tomas' patch. It is structured in a way that conditions the use of index filters on the absence of equivalent index quals for eligible/indexed clauses. So it really does make sense to think of it as a "qual hierarchy" (to use Tomas' term). It's also true that it will always be fundamentally impossible to use index quals for a significant proportion of all clause types, so "better when feasible at all" is slightly more accurate. > Is there any part of the design here that's preventing this patch from > moving forward? If not, what are the TODOs for the current patch, or is > it just waiting on more review? Practically speaking, I have no objections to moving ahead with this. I believe that the general structure of the patch makes sense, and I don't expect Tomas to do anything that wasn't already expected before I chimed in. -- Peter Geoghegan
Re: Use of additional index columns in rows filtering
On Tue, Aug 8, 2023 at 11:36 AM Peter Geoghegan wrote: > > The only thing the patch does is it looks at clauses we decided not to > > treat as index quals, and do maybe still evaluate them on index. And I > > don't think I want to move these goalposts much further. > > Avoiding the need for visibility checks completely (in at least a > subset of cases) was originally your idea. I'm not going to insist on > it, or anything like that. It just seems like something that'll add a > great deal of value over what you have already. Another idea in this area occurred to me today: it would be cool if non-key columns from INCLUDE indexes could completely avoid visibility checks (not just avoid heap accesses using the visibility map) in roughly the same way that we'd expect with an equivalent key column already, today (if it was an index filter index qual). Offhand I think that it would make sense to do that outside of index AMs, by extending the mechanism from Tomas' patch to this special class of expression. We'd invent some other class of index filter that "outranks" conventional index filters when the optimizer can safely determine that they're "index filters with the visibility characteristics of true index quals". I am mostly thinking of simple, common cases here (e.g., an equality operator + constant). This might require the involvement of the relevant btree opclass, since that's where the no-visibility-check-safety property actually comes from. However, it wouldn't need to be limited to INCLUDE B-Tree indexes. You could for example do this with a GiST INCLUDE index that had no opclass information about the datatype/operator. That'd be a natural advantage of an approach based on index filters. -- Peter Geoghegan
Re: PG 16 draft release notes ready
I posted to pgsql-docs first, but was kindly redirected here by Jonathan: The release notes for Postgres 16 says here: https://www.postgresql.org/docs/16/release-16.html#RELEASE-16-PERFORMANCE Same as here: https://momjian.us/pgsql_docs/release-16.html#RELEASE-16-PERFORMANCE Allow window functions to use ROWS mode internally when RANGE mode is specified but unnecessary (David Rowley) But the improvement (fix to some degree) also applies to the much more common case where no mode has been specified, RANGE unfortunately being the default. That includes the most common use case "row_number() OVER (ORDER BY col)", where RANGE mode should not be applied to begin with, according to SQL specs. This is what made me investigate, test and eventually propose a fix in the first place. See: https://www.postgresql.org/message-id/flat/CAGHENJ7LBBszxS%2BSkWWFVnBmOT2oVsBhDMB1DFrgerCeYa_DyA%40mail.gmail.com https://www.postgresql.org/message-id/flat/CAApHDvohAKEtTXxq7Pc-ic2dKT8oZfbRKeEJP64M0B6%2BS88z%2BA%40mail.gmail.com Also, I was hoping to be mentioned in the release note for working this out: Allow window functions to use the faster ROWS mode internally when RANGE mode is specified or would be default, but unnecessary (David Rowley, Erwin Brandstetter) Thanks, Erwin On Sat, 19 Aug 2023 at 04:02, Bruce Momjian wrote: > I have completed the first draft of the PG 16 release notes. You can > see the output here: > > https://momjian.us/pgsql_docs/release-16.html > > I will adjust it to the feedback I receive; that URL will quickly show > all updates. > > I learned a few things creating it this time: > > * I can get confused over C function names and SQL function names in >commit messages. > > * The sections and ordering of the entries can greatly clarify the >items. > > * The feature count is slightly higher than recent releases: > > release-10: 189 > release-11: 170 > release-12: 180 > release-13: 178 > release-14: 220 > release-15: 184 > --> release-16: 200 > > -- > Bruce Momjian https://momjian.us > EDB https://enterprisedb.com > > Only you can decide what is important to you. > > > > >
Re: [PATCH] Add function to_oct
On Thu, Aug 17, 2023 at 10:26 PM Nathan Bossart
wrote:
>
> On Thu, Aug 17, 2023 at 12:35:54PM +0700, John Naylor wrote:
> > That makes it a lexically-scoped global variable, which we don't need
> > either. Can we have the internal function allocate on the stack, then
> > call cstring_to_text() on that, returning the text result? That does its
> > own palloc.
> >
> > Or maybe better, save the starting pointer, compute the length at the
end,
> > and call cstring_to_text_with_len()? (It seems we wouldn't need
> > the nul-terminator then, either.)
>
> Works for me. I did it that way in v7.
This looks nicer, but still doesn't save the starting pointer, and so needs
to lug around that big honking macro. This is what I mean:
static inline text *
convert_to_base(uint64 value, int base)
{
const char *digits = "0123456789abcdef";
/* We size the buffer for to_binary's longest possible return value. */
charbuf[sizeof(uint64) * BITS_PER_BYTE];
char * const end = buf + sizeof(buf);
char *ptr = end;
Assert(base > 1);
Assert(base <= 16);
do
{
*--ptr = digits[value % base];
value /= base;
} while (ptr > buf && value);
return cstring_to_text_with_len(ptr, end - ptr);
}
--
John Naylor
EDB: http://www.enterprisedb.com
Re: pg_upgrade fails with in-place tablespace
On Fri, Aug 18, 2023 at 10:47:04PM +0800, Rui Zhao wrote: > I have implemented the changes you suggested in our previous > discussion. I have added the necessary code to ensure that > pg_upgrade fails gracefully with in-place tablespaces and reports a > hint to let the check pass. + /* +* No need to check in-place tablespaces when allow_in_place_tablespaces is +* on because they are part of the data folder. +*/ + if (allow_in_place_tablespaces) + snprintf(query, sizeof(query), +"%s AND pg_catalog.pg_tablespace_location(oid) != 'pg_tblspc/' || oid", query); I am not sure to follow the meaning of this logic. There could be in-place tablespaces that got created with the GUC enabled during a session, while the GUC has disabled the GUC. Shouldn't this stuff be more restrictive and not require a lookup at the GUC, only looking at the paths returned by pg_tablespace_location()? -- Michael signature.asc Description: PGP signature
Re: Ignore 2PC transaction GIDs in query jumbling
On Fri, Aug 18, 2023 at 11:31:03AM +0100, Dagfinn Ilmari Mannsåker wrote: > I don't have a particularly strong opinion on whether we should > distinguish DEALLOCATE ALL from DEALLOCATE (call it +0.5), but The difference looks important to me, especially for monitoring. And pgbouncer may also use both of them, actually? (Somebody, please correct me here if necessary.) > this seems like a reasonable way to do it unless we want to invent a > query_jumble_ignore_unless_null attribute (which seems like overkill for > this one case). I don't really want to have a NULL-based property for that :) -- Michael signature.asc Description: PGP signature
Re: [17] Special search_path names "!pg_temp" and "!pg_catalog"
Hi
pá 18. 8. 2023 v 23:44 odesílatel Jeff Davis napsal:
> The attached patch adds some special names to prevent pg_temp and/or
> pg_catalog from being included implicitly.
>
> This is a useful safety feature for functions that don't have any need
> to search pg_temp.
>
> The current (v16) recommendation is to include pg_temp last, which does
> add to the safety, but it's confusing to *include* a namespace when
> your intention is actually to *exclude* it, and it's also not
> completely excluding pg_temp.
>
> Although the syntax in the attached patch is not much friendlier, at
> least it's clear that the intent is to exclude pg_temp. Furthermore, it
> will be friendlier if we adopt the SEARCH SYSTEM syntax proposed in
> another thread[1].
>
> Additionally, this patch adds a WARNING when creating a schema that
> uses one of these special names. Previously, there was no warning when
> creating a schema with the name "$user", which could cause confusion.
>
> [1]
>
> https://www.postgresql.org/message-id/flat/2710f56add351a1ed553efb677408e51b060e67c.ca...@j-davis.com
cannot be better special syntax
CREATE OR REPLACE FUNCTION xxx()
RETURNS yyy AS $$ ... $$$
SET SEARCH_PATH DISABLE
with possible next modification
SET SEARCH_PATH CATALOG .. only for pg_catalog
SET SEARCH_PATH MINIMAL .. pg_catalog, pg_temp
I question if we should block search path settings when this setting is
used. Although I set search_path, the search_path can be overwritten in
function of inside some nesting calls
(2023-08-19 07:15:21) postgres=# create or replace function fx()
returns text as $$
begin
perform set_config('search_path', 'public', false);
return current_setting('search_path');
end;
$$ language plpgsql set search_path = 'pg_catalog';
CREATE FUNCTION
(2023-08-19 07:15:27) postgres=# select fx();
┌┐
│ fx │
╞╡
│ public │
└┘
(1 row)
>
>
>
> --
> Jeff Davis
> PostgreSQL Contributor Team - AWS
>
>
>
persist logical slots to disk during shutdown checkpoint
It's entirely possible for a logical slot to have a confirmed_flush LSN higher than the last value saved on disk while not being marked as dirty. It's currently not a problem to lose that value during a clean shutdown / restart cycle but to support the upgrade of logical slots [1] (see latest patch at [2]), we seem to rely on that value being properly persisted to disk. During the upgrade, we need to verify that all the data prior to shudown_checkpoint for the logical slots has been consumed, otherwise, the downstream may miss some data. Now, to ensure the same, we are planning to compare the confirm_flush LSN location with the latest shudown_checkpoint location which means that the confirm_flush LSN should be updated after restart. I think this is inefficient even without an upgrade because, after the restart, this may lead to decoding some data again. Say, we process some transactions for which we didn't send anything downstream (the changes got filtered) but the confirm_flush LSN is updated due to keepalives. As we don't flush the latest value of confirm_flush LSN, it may lead to processing the same changes again. The idea discussed in the thread [1] is to always persist logical slots to disk during the shutdown checkpoint. I have extracted the patch to achieve the same from that thread and attached it here. This could lead to some overhead during shutdown (checkpoint) if there are many slots but it is probably a one-time work. I couldn't think of better ideas but another possibility is to mark the slot as dirty when we update the confirm_flush LSN (see LogicalConfirmReceivedLocation()). However, that would be a bigger overhead in the running server as it could be a frequent operation and could lead to more writes. Thoughts? [1] - https://www.postgresql.org/message-id/TYAPR01MB58664C81887B3AF2EB6B16E3F5939%40TYAPR01MB5866.jpnprd01.prod.outlook.com [2] - https://www.postgresql.org/message-id/TYAPR01MB5866562EF047F2C9DDD1F9DEF51BA%40TYAPR01MB5866.jpnprd01.prod.outlook.com -- With Regards, Amit Kapila. v1-0001-Always-persist-to-disk-logical-slots-during-a-sh.patch Description: Binary data
