public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 7/7] Add a comment to ATExecSetTableSpace.
16+ messages / 8 participants
[nested] [flat]
* [PATCH 7/7] Add a comment to ATExecSetTableSpace.
@ 2019-03-25 11:39 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Kyotaro Horiguchi @ 2019-03-25 11:39 UTC (permalink / raw)
We use heap_register_sync() stuff to control WAL-logging and file sync
on bulk insertion, but we cannot use it because the function lacks the
ability to handle forks explicitly. Add a comment to explain that.
---
src/backend/commands/tablecmds.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 242311b0d7..c7c7bcb308 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -11594,7 +11594,13 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode)
RelationInvalidateWALRequirements(rel);
RelationCreateStorage(newrnode, rel->rd_rel->relpersistence);
- /* copy main fork */
+ /*
+ * copy main fork
+ *
+ * You might think that we could use heap_register_sync() to control file
+ * sync and WAL-logging, but we cannot because the sutff lacks the ability
+ * to handle each fork explicitly.
+ */
copy_relation_data(rel->rd_smgr, dstrel, MAIN_FORKNUM,
rel->rd_rel->relpersistence);
--
2.16.3
----Next_Part(Mon_Mar_25_21_32_04_2019_838)----
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: ON ERROR in json_query and the like
@ 2024-06-21 04:39 Pavel Stehule <[email protected]>
2024-06-21 04:46 ` Re: ON ERROR in json_query and the like David G. Johnston <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Pavel Stehule @ 2024-06-21 04:39 UTC (permalink / raw)
To: Amit Langote <[email protected]>; +Cc: David G. Johnston <[email protected]>; Markus Winand <[email protected]>; pgsql-hackers
pá 21. 6. 2024 v 6:01 odesílatel Amit Langote <[email protected]>
napsal:
> On Fri, Jun 21, 2024 at 10:01 AM David G. Johnston
> <[email protected]> wrote:
> > On Thu, Jun 20, 2024 at 5:22 PM Amit Langote <[email protected]>
> wrote:
> >>
> >>
> >> Soft error handling *was* used for catching cast errors in the very
> >> early versions of this patch (long before I got involved and the
> >> infrastructure you mention got added). It was taken out after Pavel
> >> said [1] that he didn't like producing NULL instead of throwing an
> >> error. Not sure if Pavel's around but it would be good to know why he
> >> didn't like it at the time.
> >>
> >
> > I'm personally in the "make it error" camp but "make it conform to the
> standard" is a stronger membership (in general).
> >
> > I see this note in your linked thread:
> >
> > > By the standard, it is implementation-defined whether JSON parsing
> errors
> > > should be caught by ON ERROR clause.
> >
> > Absent someone contradicting that claim I retract my position here and
> am fine with failing if these "functions" are supplied with something that
> cannot be cast to json. I'd document them like functions that accept json
> with the implications that any casting to json happens before the function
> is called and thus its arguments do not apply to that step.
>
> Thanks for that clarification.
>
> So, there are the following options:
>
> 1. Disallow anything but jsonb for context_item (the patch I posted
> yesterday)
>
> 2. Continue allowing context_item to be non-json character or utf-8
> encoded bytea strings, but document that any parsing errors do not
> respect the ON ERROR clause.
>
> 3. Go ahead and fix implicit casts to jsonb so that any parsing errors
> respect ON ERROR (no patch written yet).
>
> David's vote seems to be 2, which is my inclination too. Markus' vote
> seems to be either 1 or 3. Anyone else?
>
@3 can be possibly messy (although be near Oracle or standard). I don't
think it is safe - one example '{a:10}' is valid for Oracle, but not for
Postgres, and using @3 impacts different results (better to raise an
exception).
The effect of @1 and @2 is similar - @1 is better so the user needs to
explicitly cast, so maybe it is cleaner, so the cast should not be handled,
@2 is more user friendly, because it accepts unknown string literal. From a
developer perspective I prefer @1, from a user perspective I prefer @2.
Maybe @2 is a good compromise.
Regards
Pavel
>
> --
> Thanks, Amit Langote
>
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: ON ERROR in json_query and the like
2024-06-21 04:39 Re: ON ERROR in json_query and the like Pavel Stehule <[email protected]>
@ 2024-06-21 04:46 ` David G. Johnston <[email protected]>
2024-06-21 05:28 ` Re: ON ERROR in json_query and the like Markus Winand <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: David G. Johnston @ 2024-06-21 04:46 UTC (permalink / raw)
To: Pavel Stehule <[email protected]>; +Cc: Amit Langote <[email protected]>; Markus Winand <[email protected]>; pgsql-hackers
On Thursday, June 20, 2024, Pavel Stehule <[email protected]> wrote:
>
>
> pá 21. 6. 2024 v 6:01 odesílatel Amit Langote <[email protected]>
> napsal:
>
>> On Fri, Jun 21, 2024 at 10:01 AM David G. Johnston
>> <[email protected]> wrote:
>>
>> > > By the standard, it is implementation-defined whether JSON parsing
>> errors
>> > > should be caught by ON ERROR clause.
>> >
>> > Absent someone contradicting that claim I retract my position here and
>> am fine with failing if these "functions" are supplied with something that
>> cannot be cast to json. I'd document them like functions that accept json
>> with the implications that any casting to json happens before the function
>> is called and thus its arguments do not apply to that step.
>>
>> Thanks for that clarification.
>>
>> So, there are the following options:
>>
>> 1. Disallow anything but jsonb for context_item (the patch I posted
>> yesterday)
>>
>> 2. Continue allowing context_item to be non-json character or utf-8
>> encoded bytea strings, but document that any parsing errors do not
>> respect the ON ERROR clause.
>>
>> 3. Go ahead and fix implicit casts to jsonb so that any parsing errors
>> respect ON ERROR (no patch written yet).
>>
>> David's vote seems to be 2, which is my inclination too. Markus' vote
>> seems to be either 1 or 3. Anyone else?
>>
>
> @3 can be possibly messy (although be near Oracle or standard). I don't
> think it is safe - one example '{a:10}' is valid for Oracle, but not for
> Postgres, and using @3 impacts different results (better to raise an
> exception).
>
> The effect of @1 and @2 is similar - @1 is better so the user needs to
> explicitly cast, so maybe it is cleaner, so the cast should not be handled,
> @2 is more user friendly, because it accepts unknown string literal. From a
> developer perspective I prefer @1, from a user perspective I prefer @2.
> Maybe @2 is a good compromise.
>
2 also has the benefit of being standard conforming while 1 does not.
3 is also conforming and I wouldn’t object to it had we already done it
that way.
But since 2 is conforming too and implemented, and we are in beta, I'm
thinking we need to go with this option.
David J.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: ON ERROR in json_query and the like
2024-06-21 04:39 Re: ON ERROR in json_query and the like Pavel Stehule <[email protected]>
2024-06-21 04:46 ` Re: ON ERROR in json_query and the like David G. Johnston <[email protected]>
@ 2024-06-21 05:28 ` Markus Winand <[email protected]>
2024-06-21 05:38 ` Re: ON ERROR in json_query and the like David G. Johnston <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Markus Winand @ 2024-06-21 05:28 UTC (permalink / raw)
To: David G. Johnston <[email protected]>; +Cc: Pavel Stehule <[email protected]>; Amit Langote <[email protected]>; pgsql-hackers
> On 21.06.2024, at 06:46, David G. Johnston <[email protected]> wrote:
>>
>> On Thursday, June 20, 2024, Pavel Stehule <[email protected]> wrote:
>>
>>
>> pá 21. 6. 2024 v 6:01 odesílatel Amit Langote <[email protected]> napsal:
>> On Fri, Jun 21, 2024 at 10:01 AM David G. Johnston
>> <[email protected]> wrote:
>>
>> > > By the standard, it is implementation-defined whether JSON parsing errors
>> > > should be caught by ON ERROR clause.
>> >
>> > Absent someone contradicting that claim I retract my position here and am fine with failing if these "functions" are supplied with something that cannot be cast to json. I'd document them like functions that accept json with the implications that any casting to json happens before the function is called and thus its arguments do not apply to that step.
>>
>> Thanks for that clarification.
>>
>> So, there are the following options:
>>
>> 1. Disallow anything but jsonb for context_item (the patch I posted yesterday)
>>
>> 2. Continue allowing context_item to be non-json character or utf-8
>> encoded bytea strings, but document that any parsing errors do not
>> respect the ON ERROR clause.
>>
>> 3. Go ahead and fix implicit casts to jsonb so that any parsing errors
>> respect ON ERROR (no patch written yet).
>>
>> David's vote seems to be 2, which is my inclination too. Markus' vote
>> seems to be either 1 or 3. Anyone else?
With a very strong preference of 3.
>>
>> @3 can be possibly messy (although be near Oracle or standard). I don't think it is safe - one example '{a:10}' is valid for Oracle, but not for Postgres, and using @3 impacts different results (better to raise an exception).
The question of what is valid JSON is a different question, I guess. My original report is about something that is invalid everywhere. Having that in line would be a start. Also I believe Oracle’s habit to accept unquoted object keys is not covered by the standard (unless defined as a JSON format and also explicitly using the corresponding FORMAT clause).
>> The effect of @1 and @2 is similar - @1 is better so the user needs to explicitly cast, so maybe it is cleaner, so the cast should not be handled, @2 is more user friendly, because it accepts unknown string literal. From a developer perspective I prefer @1, from a user perspective I prefer @2. Maybe @2 is a good compromise.
>
> 2 also has the benefit of being standard conforming while 1 does not.
Why do you think so? Do you have any references or is this just based on previous statements in this discussion?
-markus
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: ON ERROR in json_query and the like
2024-06-21 04:39 Re: ON ERROR in json_query and the like Pavel Stehule <[email protected]>
2024-06-21 04:46 ` Re: ON ERROR in json_query and the like David G. Johnston <[email protected]>
2024-06-21 05:28 ` Re: ON ERROR in json_query and the like Markus Winand <[email protected]>
@ 2024-06-21 05:38 ` David G. Johnston <[email protected]>
2024-06-21 10:59 ` Re: ON ERROR in json_query and the like Markus Winand <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: David G. Johnston @ 2024-06-21 05:38 UTC (permalink / raw)
To: Markus Winand <[email protected]>; +Cc: Pavel Stehule <[email protected]>; Amit Langote <[email protected]>; pgsql-hackers
On Thursday, June 20, 2024, Markus Winand <[email protected]> wrote:
>
>
> > On 21.06.2024, at 06:46, David G. Johnston <[email protected]>
> wrote:
> >>
>
> >
> > 2 also has the benefit of being standard conforming while 1 does not.
>
> Why do you think so? Do you have any references or is this just based on
> previous statements in this discussion?
>
>
Hearsay.
https://www.postgresql.org/message-id/CAFj8pRCnzO2cnHi5ebXciV%3DtuGVvAQOW9uPU%2BDQV1GkL31R%3D-g%40ma...
> 4) If ALREADY PARSED is False, then it is implementation-defined whether
the
> following rules are applied:
> a) The General Rules of Subclause 9.36, "Parsing JSON text", are applied
with
> JT as JSON TEXT, an implementation-defined <JSON key uniqueness
constraint>
> as UNIQUENESS CONSTRAINT, and FO as FORMAT OPTION; let ST be the STATUS
and
> let CISJI be the SQL/JSON ITEM returned from the application of those
> General Rules.
> b) If ST is not successful completion, then ST is returned as the STATUS
of
> this application of these General Rules, and no further General Rules of
> this Subclause are applied.
But maybe I’m mis-interpreting that snippet and Nikita’s related commentary
regarding have chosen between options for this implementation-defined
feature.
David j.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: ON ERROR in json_query and the like
2024-06-21 04:39 Re: ON ERROR in json_query and the like Pavel Stehule <[email protected]>
2024-06-21 04:46 ` Re: ON ERROR in json_query and the like David G. Johnston <[email protected]>
2024-06-21 05:28 ` Re: ON ERROR in json_query and the like Markus Winand <[email protected]>
2024-06-21 05:38 ` Re: ON ERROR in json_query and the like David G. Johnston <[email protected]>
@ 2024-06-21 10:59 ` Markus Winand <[email protected]>
2024-06-22 08:43 ` Re: ON ERROR in json_query and the like Amit Langote <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Markus Winand @ 2024-06-21 10:59 UTC (permalink / raw)
To: David G. Johnston <[email protected]>; +Cc: Pavel Stehule <[email protected]>; Amit Langote <[email protected]>; pgsql-hackers
> On 21.06.2024, at 07:38, David G. Johnston <[email protected]> wrote:
>
> On Thursday, June 20, 2024, Markus Winand <[email protected]> wrote:
>
>
> > On 21.06.2024, at 06:46, David G. Johnston <[email protected]> wrote:
> >>
>
> >
> > 2 also has the benefit of being standard conforming while 1 does not.
>
> Why do you think so? Do you have any references or is this just based on previous statements in this discussion?
>
>
> Hearsay.
>
> https://www.postgresql.org/message-id/CAFj8pRCnzO2cnHi5ebXciV%3DtuGVvAQOW9uPU%2BDQV1GkL31R%3D-g%40ma...
>
> > 4) If ALREADY PARSED is False, then it is implementation-defined whether the
> > following rules are applied:
> > a) The General Rules of Subclause 9.36, "Parsing JSON text", are applied with
> > JT as JSON TEXT, an implementation-defined <JSON key uniqueness constraint>
> > as UNIQUENESS CONSTRAINT, and FO as FORMAT OPTION; let ST be the STATUS and
> > let CISJI be the SQL/JSON ITEM returned from the application of those
> > General Rules.
> > b) If ST is not successful completion, then ST is returned as the STATUS of
> > this application of these General Rules, and no further General Rules of
> > this Subclause are applied.
>
> But maybe I’m mis-interpreting that snippet and Nikita’s related commentary regarding have chosen between options for this implementation-defined feature.
Ah, here we go. Nowadays this is called IA050, “Whether a JSON context item that is not of the JSON data type is parsed.” (Likewise IA054 “Whether a JSON parameter is parsed.”)
So updating the three options:
> 1. Disallow anything but jsonb for context_item (the patch I posted yesterday)
* Non-conforming
* patch available
> 2. Continue allowing context_item to be non-json character or utf-8
> encoded bytea strings, but document that any parsing errors do not
> respect the ON ERROR clause.
* Conforming by choosing IA050 to implement GR4: raise errors independent of the ON ERROR clause.
* currently committed.
> 3. Go ahead and fix implicit casts to jsonb so that any parsing errors
> respect ON ERROR (no patch written yet).
* Conforming by choosing IA050 not to implement GR4: Parsing happens later, considering the ON ERROR clause.
* no patch available, not trivial
I guess I’m the only one in favour of 3 ;) My remaining arguments are that Oracle and Db2 (LUW) do it that way and also that it is IMHO what users would expect. However, as 2 is also conforming (how could I miss that?), proper documentation is a very tempting option.
-markus
ps: Does anyone know a dialect that implements GR4?
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: ON ERROR in json_query and the like
2024-06-21 04:39 Re: ON ERROR in json_query and the like Pavel Stehule <[email protected]>
2024-06-21 04:46 ` Re: ON ERROR in json_query and the like David G. Johnston <[email protected]>
2024-06-21 05:28 ` Re: ON ERROR in json_query and the like Markus Winand <[email protected]>
2024-06-21 05:38 ` Re: ON ERROR in json_query and the like David G. Johnston <[email protected]>
2024-06-21 10:59 ` Re: ON ERROR in json_query and the like Markus Winand <[email protected]>
@ 2024-06-22 08:43 ` Amit Langote <[email protected]>
2024-06-26 12:10 ` Re: ON ERROR in json_query and the like Amit Langote <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Amit Langote @ 2024-06-22 08:43 UTC (permalink / raw)
To: Markus Winand <[email protected]>; +Cc: David G. Johnston <[email protected]>; Pavel Stehule <[email protected]>; pgsql-hackers
Hi,
Thanks all for chiming in.
On Fri, Jun 21, 2024 at 8:00 PM Markus Winand <[email protected]> wrote:
> So updating the three options:
> > 1. Disallow anything but jsonb for context_item (the patch I posted yesterday)
>
> * Non-conforming
> * patch available
>
> > 2. Continue allowing context_item to be non-json character or utf-8
> > encoded bytea strings, but document that any parsing errors do not
> > respect the ON ERROR clause.
>
> * Conforming by choosing IA050 to implement GR4: raise errors independent of the ON ERROR clause.
> * currently committed.
>
> > 3. Go ahead and fix implicit casts to jsonb so that any parsing errors
> > respect ON ERROR (no patch written yet).
>
> * Conforming by choosing IA050 not to implement GR4: Parsing happens later, considering the ON ERROR clause.
> * no patch available, not trivial
>
> I guess I’m the only one in favour of 3 ;) My remaining arguments are that Oracle and Db2 (LUW) do it that way and also that it is IMHO what users would expect. However, as 2 is also conforming (how could I miss that?), proper documentation is a very tempting option.
So, we should go with 2 for v17, because while 3 may be very
appealing, there's a risk that it might not get done in the time
remaining for v17.
I'll post the documentation patch on Monday.
--
Thanks, Amit Langote
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: ON ERROR in json_query and the like
2024-06-21 04:39 Re: ON ERROR in json_query and the like Pavel Stehule <[email protected]>
2024-06-21 04:46 ` Re: ON ERROR in json_query and the like David G. Johnston <[email protected]>
2024-06-21 05:28 ` Re: ON ERROR in json_query and the like Markus Winand <[email protected]>
2024-06-21 05:38 ` Re: ON ERROR in json_query and the like David G. Johnston <[email protected]>
2024-06-21 10:59 ` Re: ON ERROR in json_query and the like Markus Winand <[email protected]>
2024-06-22 08:43 ` Re: ON ERROR in json_query and the like Amit Langote <[email protected]>
@ 2024-06-26 12:10 ` Amit Langote <[email protected]>
2024-06-28 00:49 ` Re: ON ERROR in json_query and the like Amit Langote <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Amit Langote @ 2024-06-26 12:10 UTC (permalink / raw)
To: Markus Winand <[email protected]>; +Cc: David G. Johnston <[email protected]>; Pavel Stehule <[email protected]>; pgsql-hackers; Peter Eisentraut <[email protected]>
Hi,
On Sat, Jun 22, 2024 at 5:43 PM Amit Langote <[email protected]> wrote:
> On Fri, Jun 21, 2024 at 8:00 PM Markus Winand <[email protected]> wrote:
> > So updating the three options:
> > > 1. Disallow anything but jsonb for context_item (the patch I posted yesterday)
> >
> > * Non-conforming
> > * patch available
> >
> > > 2. Continue allowing context_item to be non-json character or utf-8
> > > encoded bytea strings, but document that any parsing errors do not
> > > respect the ON ERROR clause.
> >
> > * Conforming by choosing IA050 to implement GR4: raise errors independent of the ON ERROR clause.
> > * currently committed.
> >
> > > 3. Go ahead and fix implicit casts to jsonb so that any parsing errors
> > > respect ON ERROR (no patch written yet).
> >
> > * Conforming by choosing IA050 not to implement GR4: Parsing happens later, considering the ON ERROR clause.
> > * no patch available, not trivial
> >
> > I guess I’m the only one in favour of 3 ;) My remaining arguments are that Oracle and Db2 (LUW) do it that way and also that it is IMHO what users would expect. However, as 2 is also conforming (how could I miss that?), proper documentation is a very tempting option.
>
> So, we should go with 2 for v17, because while 3 may be very
> appealing, there's a risk that it might not get done in the time
> remaining for v17.
>
> I'll post the documentation patch on Monday.
Here's that patch, which adds this note after table 9.16.3. SQL/JSON
Query Functions:
+ <note>
+ <para>
+ The <replaceable>context_item</replaceable> expression is converted to
+ <type>jsonb</type> by an implicit cast if the expression is not already of
+ type <type>jsonb</type>. Note, however, that any parsing errors that occur
+ during that conversion are thrown unconditionally, that is, are not
+ handled according to the (specified or implicit) <literal>ON
ERROR</literal>
+ clause.
+ </para>
+ </note>
Peter, sorry about the last-minute ask, but do you have any
thoughts/advice on conformance as discussed above?
--
Thanks, Amit Langote
Attachments:
[application/octet-stream] v1-0001-SQL-JSON-Document-behavior-when-context_item-is-n.patch (1.7K, ../../CA+HiwqHGVkkSde-fue+dkDxGY8_ddqBpt+pcQaAuStFzSN-MCQ@mail.gmail.com/2-v1-0001-SQL-JSON-Document-behavior-when-context_item-is-n.patch)
download | inline diff:
From aca0c308013dbf434d54da7e68d20dd45601ac99 Mon Sep 17 00:00:00 2001
From: Amit Langote <[email protected]>
Date: Wed, 26 Jun 2024 17:46:45 +0900
Subject: [PATCH v1] SQL/JSON: Document behavior when context_item is not jsonb
The input document to functions JSON_EXISTS(), JSON_QUERY(),
JSON_VALUE(), and JSON_TABLE() can be specified as character or
UTF8-encoded bytea strings. These are automatically converted to jsonb
with an implicit cast before being passed to the jsonpath machinery.
In the current implementation, errors that occur when parsing the specified
string into a valid JSON document are thrown unconditionally. This means they
are not subject to the explicit or implicit ON ERROR clause of those functions.
This behavior is standard-conforming.
Reported-by: Markus Winand
Discussion: https://postgr.es/m/F7DD1442-265C-4220-A603-CB0DEB77E91D%40winand.at
---
doc/src/sgml/func.sgml | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 85ebd65892..532c1cc28c 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -18870,6 +18870,16 @@ DETAIL: Missing "]" after array dimensions.
</tbody>
</tgroup>
</table>
+ <note>
+ <para>
+ The <replaceable>context_item</replaceable> expression is converted to
+ <type>jsonb</type> by an implicit cast if the expression is not already of
+ type <type>jsonb</type>. Note, however, that any parsing errors that occur
+ during that conversion are thrown unconditionally, that is, are not
+ handled according to the (specified or implicit) <literal>ON ERROR</literal>
+ clause.
+ </para>
+ </note>
</sect2>
<sect2 id="functions-sqljson-table">
--
2.43.0
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: ON ERROR in json_query and the like
2024-06-21 04:39 Re: ON ERROR in json_query and the like Pavel Stehule <[email protected]>
2024-06-21 04:46 ` Re: ON ERROR in json_query and the like David G. Johnston <[email protected]>
2024-06-21 05:28 ` Re: ON ERROR in json_query and the like Markus Winand <[email protected]>
2024-06-21 05:38 ` Re: ON ERROR in json_query and the like David G. Johnston <[email protected]>
2024-06-21 10:59 ` Re: ON ERROR in json_query and the like Markus Winand <[email protected]>
2024-06-22 08:43 ` Re: ON ERROR in json_query and the like Amit Langote <[email protected]>
2024-06-26 12:10 ` Re: ON ERROR in json_query and the like Amit Langote <[email protected]>
@ 2024-06-28 00:49 ` Amit Langote <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Amit Langote @ 2024-06-28 00:49 UTC (permalink / raw)
To: Markus Winand <[email protected]>; +Cc: David G. Johnston <[email protected]>; Pavel Stehule <[email protected]>; pgsql-hackers; Peter Eisentraut <[email protected]>
On Wed, Jun 26, 2024 at 9:10 PM Amit Langote <[email protected]> wrote:
> On Sat, Jun 22, 2024 at 5:43 PM Amit Langote <[email protected]> wrote:
> > On Fri, Jun 21, 2024 at 8:00 PM Markus Winand <[email protected]> wrote:
> > > So updating the three options:
> > > > 1. Disallow anything but jsonb for context_item (the patch I posted yesterday)
> > >
> > > * Non-conforming
> > > * patch available
> > >
> > > > 2. Continue allowing context_item to be non-json character or utf-8
> > > > encoded bytea strings, but document that any parsing errors do not
> > > > respect the ON ERROR clause.
> > >
> > > * Conforming by choosing IA050 to implement GR4: raise errors independent of the ON ERROR clause.
> > > * currently committed.
> > >
> > > > 3. Go ahead and fix implicit casts to jsonb so that any parsing errors
> > > > respect ON ERROR (no patch written yet).
> > >
> > > * Conforming by choosing IA050 not to implement GR4: Parsing happens later, considering the ON ERROR clause.
> > > * no patch available, not trivial
> > >
> > > I guess I’m the only one in favour of 3 ;) My remaining arguments are that Oracle and Db2 (LUW) do it that way and also that it is IMHO what users would expect. However, as 2 is also conforming (how could I miss that?), proper documentation is a very tempting option.
> >
> > So, we should go with 2 for v17, because while 3 may be very
> > appealing, there's a risk that it might not get done in the time
> > remaining for v17.
> >
> > I'll post the documentation patch on Monday.
>
> Here's that patch, which adds this note after table 9.16.3. SQL/JSON
> Query Functions:
>
> + <note>
> + <para>
> + The <replaceable>context_item</replaceable> expression is converted to
> + <type>jsonb</type> by an implicit cast if the expression is not already of
> + type <type>jsonb</type>. Note, however, that any parsing errors that occur
> + during that conversion are thrown unconditionally, that is, are not
> + handled according to the (specified or implicit) <literal>ON
> ERROR</literal>
> + clause.
> + </para>
> + </note>
I have pushed this.
--
Thanks, Amit Langote
^ permalink raw reply [nested|flat] 16+ messages in thread
* [PATCH v25 2/9] Row pattern recognition patch (parse/analysis).
@ 2024-12-21 06:19 Tatsuo Ishii <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Tatsuo Ishii @ 2024-12-21 06:19 UTC (permalink / raw)
---
src/backend/parser/parse_agg.c | 7 +
src/backend/parser/parse_clause.c | 297 +++++++++++++++++++++++++++++-
src/backend/parser/parse_expr.c | 6 +
src/backend/parser/parse_func.c | 3 +
4 files changed, 312 insertions(+), 1 deletion(-)
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 04b4596a65..6af3bcb375 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -580,6 +580,10 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr)
errkind = true;
break;
+ case EXPR_KIND_RPR_DEFINE:
+ errkind = true;
+ break;
+
/*
* There is intentionally no default: case here, so that the
* compiler will warn if we add a new ParseExprKind without
@@ -970,6 +974,9 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc,
case EXPR_KIND_CYCLE_MARK:
errkind = true;
break;
+ case EXPR_KIND_RPR_DEFINE:
+ errkind = true;
+ break;
/*
* There is intentionally no default: case here, so that the
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 979926b605..5a44c68b08 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -96,7 +96,14 @@ static WindowClause *findWindowClause(List *wclist, const char *name);
static Node *transformFrameOffset(ParseState *pstate, int frameOptions,
Oid rangeopfamily, Oid rangeopcintype, Oid *inRangeFunc,
Node *clause);
-
+static void transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef,
+ List **targetlist);
+static List *transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef,
+ List **targetlist);
+static void transformPatternClause(ParseState *pstate, WindowClause *wc,
+ WindowDef *windef);
+static List *transformMeasureClause(ParseState *pstate, WindowClause *wc,
+ WindowDef *windef);
/*
* transformFromClause -
@@ -2954,6 +2961,10 @@ transformWindowDefinitions(ParseState *pstate,
rangeopfamily, rangeopcintype,
&wc->endInRangeFunc,
windef->endOffset);
+
+ /* Process Row Pattern Recognition related clauses */
+ transformRPR(pstate, wc, windef, targetlist);
+
wc->winref = winref;
result = lappend(result, wc);
@@ -3821,3 +3832,287 @@ transformFrameOffset(ParseState *pstate, int frameOptions,
return node;
}
+
+/*
+ * transformRPR
+ * Process Row Pattern Recognition related clauses
+ */
+static void
+transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef,
+ List **targetlist)
+{
+ /*
+ * Window definition exists?
+ */
+ if (windef == NULL)
+ return;
+
+ /*
+ * Row Pattern Common Syntax clause exists?
+ */
+ if (windef->rpCommonSyntax == NULL)
+ return;
+
+ /* Check Frame option. Frame must start at current row */
+ if ((wc->frameOptions & FRAMEOPTION_START_CURRENT_ROW) == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("FRAME must start at current row when row patttern recognition is used")));
+
+ /* Transform AFTER MACH SKIP TO clause */
+ wc->rpSkipTo = windef->rpCommonSyntax->rpSkipTo;
+
+ /* Transform AFTER MACH SKIP TO variable */
+ wc->rpSkipVariable = windef->rpCommonSyntax->rpSkipVariable;
+
+ /* Transform SEEK or INITIAL clause */
+ wc->initial = windef->rpCommonSyntax->initial;
+
+ /* Transform DEFINE clause into list of TargetEntry's */
+ wc->defineClause = transformDefineClause(pstate, wc, windef, targetlist);
+
+ /* Check PATTERN clause and copy to patternClause */
+ transformPatternClause(pstate, wc, windef);
+
+ /* Transform MEASURE clause */
+ transformMeasureClause(pstate, wc, windef);
+}
+
+/*
+ * transformDefineClause
+ * Process DEFINE clause and transform ResTarget into list of
+ * TargetEntry.
+ *
+ * XXX we only support column reference in row pattern definition search
+ * condition, e.g. "price". <row pattern definition variable name>.<column
+ * reference> is not supported, e.g. "A.price".
+ */
+static List *
+transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef,
+ List **targetlist)
+{
+ /* DEFINE variable name initials */
+ static char *defineVariableInitials = "abcdefghijklmnopqrstuvwxyz";
+
+ ListCell *lc,
+ *l;
+ ResTarget *restarget,
+ *r;
+ List *restargets;
+ List *defineClause;
+ char *name;
+ int initialLen;
+ int i;
+
+ /*
+ * If Row Definition Common Syntax exists, DEFINE clause must exist. (the
+ * raw parser should have already checked it.)
+ */
+ Assert(windef->rpCommonSyntax->rpDefs != NULL);
+
+ /*
+ * Check and add "A AS A IS TRUE" if pattern variable is missing in DEFINE
+ * per the SQL standard.
+ */
+ restargets = NIL;
+ foreach(lc, windef->rpCommonSyntax->rpPatterns)
+ {
+ A_Expr *a;
+ bool found = false;
+
+ if (!IsA(lfirst(lc), A_Expr))
+ ereport(ERROR,
+ errmsg("node type is not A_Expr"));
+
+ a = (A_Expr *) lfirst(lc);
+ name = strVal(a->lexpr);
+
+ foreach(l, windef->rpCommonSyntax->rpDefs)
+ {
+ restarget = (ResTarget *) lfirst(l);
+
+ if (!strcmp(restarget->name, name))
+ {
+ found = true;
+ break;
+ }
+ }
+
+ if (!found)
+ {
+ /*
+ * "name" is missing. So create "name AS name IS TRUE" ResTarget
+ * node and add it to the temporary list.
+ */
+ A_Const *n;
+
+ restarget = makeNode(ResTarget);
+ n = makeNode(A_Const);
+ n->val.boolval.type = T_Boolean;
+ n->val.boolval.boolval = true;
+ n->location = -1;
+ restarget->name = pstrdup(name);
+ restarget->indirection = NIL;
+ restarget->val = (Node *) n;
+ restarget->location = -1;
+ restargets = lappend((List *) restargets, restarget);
+ }
+ }
+
+ if (list_length(restargets) >= 1)
+ {
+ /* add missing DEFINEs */
+ windef->rpCommonSyntax->rpDefs =
+ list_concat(windef->rpCommonSyntax->rpDefs, restargets);
+ list_free(restargets);
+ }
+
+ /*
+ * Check for duplicate row pattern definition variables. The standard
+ * requires that no two row pattern definition variable names shall be
+ * equivalent.
+ */
+ restargets = NIL;
+ foreach(lc, windef->rpCommonSyntax->rpDefs)
+ {
+ restarget = (ResTarget *) lfirst(lc);
+ name = restarget->name;
+
+ /*
+ * Add DEFINE expression (Restarget->val) to the targetlist as a
+ * TargetEntry if it does not exist yet. Planner will add the column
+ * ref var node to the outer plan's target list later on. This makes
+ * DEFINE expression could access the outer tuple while evaluating
+ * PATTERN.
+ *
+ * XXX: adding whole expressions of DEFINE to the plan.targetlist is
+ * not so good, because it's not necessary to evalute the expression
+ * in the target list while running the plan. We should extract the
+ * var nodes only then add them to the plan.targetlist.
+ */
+ findTargetlistEntrySQL99(pstate, (Node *) restarget->val,
+ targetlist, EXPR_KIND_RPR_DEFINE);
+
+ /*
+ * Make sure that the row pattern definition search condition is a
+ * boolean expression.
+ */
+ transformWhereClause(pstate, restarget->val,
+ EXPR_KIND_RPR_DEFINE, "DEFINE");
+
+ foreach(l, restargets)
+ {
+ char *n;
+
+ r = (ResTarget *) lfirst(l);
+ n = r->name;
+
+ if (!strcmp(n, name))
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("row pattern definition variable name \"%s\" appears more than once in DEFINE clause",
+ name),
+ parser_errposition(pstate, exprLocation((Node *) r))));
+ }
+ restargets = lappend(restargets, restarget);
+ }
+ list_free(restargets);
+
+ /*
+ * Create list of row pattern DEFINE variable name's initial. We assign
+ * [a-z] to them (up to 26 variable names are allowed).
+ */
+ restargets = NIL;
+ i = 0;
+ initialLen = strlen(defineVariableInitials);
+
+ foreach(lc, windef->rpCommonSyntax->rpDefs)
+ {
+ char initial[2];
+
+ restarget = (ResTarget *) lfirst(lc);
+ name = restarget->name;
+
+ if (i >= initialLen)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("number of row pattern definition variable names exceeds %d",
+ initialLen),
+ parser_errposition(pstate,
+ exprLocation((Node *) restarget))));
+ }
+ initial[0] = defineVariableInitials[i++];
+ initial[1] = '\0';
+ wc->defineInitial = lappend(wc->defineInitial,
+ makeString(pstrdup(initial)));
+ }
+
+ defineClause = transformTargetList(pstate, windef->rpCommonSyntax->rpDefs,
+ EXPR_KIND_RPR_DEFINE);
+
+ /* mark column origins */
+ markTargetListOrigins(pstate, defineClause);
+
+ /* mark all nodes in the DEFINE clause tree with collation information */
+ assign_expr_collations(pstate, (Node *) defineClause);
+
+ return defineClause;
+}
+
+/*
+ * transformPatternClause
+ * Process PATTERN clause and return PATTERN clause in the raw parse tree
+ */
+static void
+transformPatternClause(ParseState *pstate, WindowClause *wc,
+ WindowDef *windef)
+{
+ ListCell *lc;
+
+ /*
+ * Row Pattern Common Syntax clause exists?
+ */
+ if (windef->rpCommonSyntax == NULL)
+ return;
+
+ wc->patternVariable = NIL;
+ wc->patternRegexp = NIL;
+ foreach(lc, windef->rpCommonSyntax->rpPatterns)
+ {
+ A_Expr *a;
+ char *name;
+ char *regexp;
+
+ if (!IsA(lfirst(lc), A_Expr))
+ ereport(ERROR,
+ errmsg("node type is not A_Expr"));
+
+ a = (A_Expr *) lfirst(lc);
+ name = strVal(a->lexpr);
+
+ wc->patternVariable = lappend(wc->patternVariable, makeString(pstrdup(name)));
+ regexp = strVal(lfirst(list_head(a->name)));
+
+ wc->patternRegexp = lappend(wc->patternRegexp, makeString(pstrdup(regexp)));
+ }
+}
+
+/*
+ * transformMeasureClause
+ * Process MEASURE clause
+ * XXX MEASURE clause is not supported yet
+ */
+static List *
+transformMeasureClause(ParseState *pstate, WindowClause *wc,
+ WindowDef *windef)
+{
+ if (windef->rowPatternMeasures == NIL)
+ return NIL;
+
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("%s", "MEASURE clause is not supported yet"),
+ parser_errposition(pstate, exprLocation((Node *) windef->rowPatternMeasures))));
+ return NIL;
+}
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index c2806297aa..e827c59fd4 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -575,6 +575,7 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
case EXPR_KIND_COPY_WHERE:
case EXPR_KIND_GENERATED_COLUMN:
case EXPR_KIND_CYCLE_MARK:
+ case EXPR_KIND_RPR_DEFINE:
/* okay */
break;
@@ -1858,6 +1859,9 @@ transformSubLink(ParseState *pstate, SubLink *sublink)
case EXPR_KIND_GENERATED_COLUMN:
err = _("cannot use subquery in column generation expression");
break;
+ case EXPR_KIND_RPR_DEFINE:
+ err = _("cannot use subquery in DEFINE expression");
+ break;
/*
* There is intentionally no default: case here, so that the
@@ -3197,6 +3201,8 @@ ParseExprKindName(ParseExprKind exprKind)
return "GENERATED AS";
case EXPR_KIND_CYCLE_MARK:
return "CYCLE";
+ case EXPR_KIND_RPR_DEFINE:
+ return "DEFINE";
/*
* There is intentionally no default: case here, so that the
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index 9b23344a3b..4c482abb30 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -2658,6 +2658,9 @@ check_srf_call_placement(ParseState *pstate, Node *last_srf, int location)
case EXPR_KIND_CYCLE_MARK:
errkind = true;
break;
+ case EXPR_KIND_RPR_DEFINE:
+ errkind = true;
+ break;
/*
* There is intentionally no default: case here, so that the
--
2.25.1
----Next_Part(Sat_Dec_21_18_20_04_2024_526)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v25-0003-Row-pattern-recognition-patch-rewriter.patch"
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [PATCH] Add support for displaying database service in psql prompt
@ 2025-07-06 16:13 Noah Misch <[email protected]>
2025-07-07 02:06 ` Re: [PATCH] Add support for displaying database service in psql prompt Michael Paquier <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Noah Misch @ 2025-07-06 16:13 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Michael Banck <[email protected]>; Greg Sabino Mullane <[email protected]>; PostgreSQL Hackers <[email protected]>
On Wed, Dec 18, 2024 at 03:17:36PM +0900, Michael Paquier wrote:
> Done.
This new PQservice() function from commit 4b99fed75 came up in the annual
exports.txt diff. The standard in libpq has been to not clutter the API with
new functions that simply retrieve one PQconninfoOption value. PQconninfo()
provides access to all those values in a generic way. What do you think of
making psql use PQconninfo() for this, then removing PQservice()? The rest of
the commit (adding the struct field, necessary for PQconninfo() to include the
value) looks good.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [PATCH] Add support for displaying database service in psql prompt
2025-07-06 16:13 Re: [PATCH] Add support for displaying database service in psql prompt Noah Misch <[email protected]>
@ 2025-07-07 02:06 ` Michael Paquier <[email protected]>
2025-07-07 03:00 ` Re: [PATCH] Add support for displaying database service in psql prompt Noah Misch <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Michael Paquier @ 2025-07-07 02:06 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Michael Banck <[email protected]>; Greg Sabino Mullane <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sun, Jul 06, 2025 at 09:13:19AM -0700, Noah Misch wrote:
> This new PQservice() function from commit 4b99fed75 came up in the annual
> exports.txt diff. The standard in libpq has been to not clutter the API with
> new functions that simply retrieve one PQconninfoOption value. PQconninfo()
> provides access to all those values in a generic way. What do you think of
> making psql use PQconninfo() for this, then removing PQservice()? The rest of
> the commit (adding the struct field, necessary for PQconninfo() to include the
> value) looks good.
Sure, I was not aware of such a policy. Relying on PQconninfoOption
is less efficient because we would need to look through the whole set
of options when looking for the service name, and this needs one extra
allocation as PQconninfoFree() frees the values allocated. With two
callers perhaps this inefficiency is OK to live with anyway.
What do you think about the attached, then?
--
Michael
Attachments:
[text/x-diff] pqservice-removal.patch (5.7K, ../../[email protected]/2-pqservice-removal.patch)
download | inline diff:
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 9fcd2db83265..c839634d4233 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -4480,6 +4480,7 @@ SyncVariables(void)
{
char vbuf[32];
const char *server_version;
+ char *service_name;
/* get stuff from connection */
pset.encoding = PQclientEncoding(pset.db);
@@ -4489,12 +4490,16 @@ SyncVariables(void)
setFmtEncoding(pset.encoding);
SetVariable(pset.vars, "DBNAME", PQdb(pset.db));
- SetVariable(pset.vars, "SERVICE", PQservice(pset.db));
SetVariable(pset.vars, "USER", PQuser(pset.db));
SetVariable(pset.vars, "HOST", PQhost(pset.db));
SetVariable(pset.vars, "PORT", PQport(pset.db));
SetVariable(pset.vars, "ENCODING", pg_encoding_to_char(pset.encoding));
+ service_name = get_service_name();
+ SetVariable(pset.vars, "SERVICE", service_name);
+ if (service_name)
+ pg_free(service_name);
+
/* this bit should match connection_warnings(): */
/* Try to get full text form of version, might include "devel" etc */
server_version = PQparameterStatus(pset.db, "server_version");
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index d2c0a49c46c0..3593c0468310 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -2531,6 +2531,41 @@ session_username(void)
return PQuser(pset.db);
}
+/*
+ * Return the service name of the current connection.
+ *
+ * The caller is responsible for freeing the result value allocated.
+ */
+char *
+get_service_name(void)
+{
+ PQconninfoOption *opts;
+ PQconninfoOption *serviceopt = NULL;
+ char *res = NULL;
+
+ if (pset.db == NULL)
+ return NULL;
+
+ opts = PQconninfo(pset.db);
+ if (opts == NULL)
+ return NULL;
+
+ for (PQconninfoOption *opt = opts; opt->keyword != NULL; ++opt)
+ {
+ if (strcmp(opt->keyword, "service") == 0)
+ {
+ serviceopt = opt;
+ continue;
+ }
+ }
+
+ /* Take a copy of the value, as it is freed by PQconninfoFree(). */
+ if (serviceopt && serviceopt->val != NULL)
+ res = pg_strdup(serviceopt->val);
+ PQconninfoFree(opts);
+
+ return res;
+}
/* expand_tilde
*
diff --git a/src/bin/psql/common.h b/src/bin/psql/common.h
index 7f1a23de1e82..261199df3187 100644
--- a/src/bin/psql/common.h
+++ b/src/bin/psql/common.h
@@ -39,6 +39,7 @@ extern bool SendQuery(const char *query);
extern bool is_superuser(void);
extern bool standard_strings(void);
extern const char *session_username(void);
+extern char *get_service_name(void);
extern void expand_tilde(char **filename);
extern void clean_extended_state(void);
diff --git a/src/bin/psql/prompt.c b/src/bin/psql/prompt.c
index 3aa7d2d06c80..cdae9b681500 100644
--- a/src/bin/psql/prompt.c
+++ b/src/bin/psql/prompt.c
@@ -169,8 +169,15 @@ get_prompt(promptStatus_t status, ConditionalStack cstack)
break;
/* service name */
case 's':
- if (pset.db && PQservice(pset.db))
- strlcpy(buf, PQservice(pset.db), sizeof(buf));
+ {
+ char *service_name = get_service_name();
+
+ if (service_name)
+ {
+ strlcpy(buf, service_name, sizeof(buf));
+ pg_free(service_name);
+ }
+ }
break;
/* backend pid */
case 'p':
diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
index 0625cf39e9af..dbbae642d769 100644
--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -205,9 +205,8 @@ PQcancelFinish 202
PQsocketPoll 203
PQsetChunkedRowsMode 204
PQgetCurrentTimeUSec 205
-PQservice 206
-PQsetAuthDataHook 207
-PQgetAuthDataHook 208
-PQdefaultAuthDataHook 209
-PQfullProtocolVersion 210
-appendPQExpBufferVA 211
+PQsetAuthDataHook 206
+PQgetAuthDataHook 207
+PQdefaultAuthDataHook 208
+PQfullProtocolVersion 209
+appendPQExpBufferVA 210
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 51a9c4165845..09eb79812ac6 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -7461,14 +7461,6 @@ PQdb(const PGconn *conn)
return conn->dbName;
}
-char *
-PQservice(const PGconn *conn)
-{
- if (!conn)
- return NULL;
- return conn->pgservice;
-}
-
char *
PQuser(const PGconn *conn)
{
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 7d3a9df6fd55..af8004f952a5 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -400,7 +400,6 @@ extern int PQrequestCancel(PGconn *conn);
/* Accessor functions for PGconn objects */
extern char *PQdb(const PGconn *conn);
-extern char *PQservice(const PGconn *conn);
extern char *PQuser(const PGconn *conn);
extern char *PQpass(const PGconn *conn);
extern char *PQhost(const PGconn *conn);
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 298c4b38ef90..b2c2cf9eac83 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2740,26 +2740,6 @@ char *PQport(const PGconn *conn);
</listitem>
</varlistentry>
- <varlistentry id="libpq-PQservice">
- <term><function>PQservice</function><indexterm><primary>PQservice</primary></indexterm></term>
-
- <listitem>
- <para>
- Returns the service of the active connection.
-
-<synopsis>
-char *PQservice(const PGconn *conn);
-</synopsis>
- </para>
-
- <para>
- <xref linkend="libpq-PQservice"/> returns <symbol>NULL</symbol> if the
- <parameter>conn</parameter> argument is <symbol>NULL</symbol>.
- Otherwise, if there was no service provided, it returns an empty string.
- </para>
- </listitem>
- </varlistentry>
-
<varlistentry id="libpq-PQtty">
<term><function>PQtty</function><indexterm><primary>PQtty</primary></indexterm></term>
[application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [PATCH] Add support for displaying database service in psql prompt
2025-07-06 16:13 Re: [PATCH] Add support for displaying database service in psql prompt Noah Misch <[email protected]>
2025-07-07 02:06 ` Re: [PATCH] Add support for displaying database service in psql prompt Michael Paquier <[email protected]>
@ 2025-07-07 03:00 ` Noah Misch <[email protected]>
2025-07-07 23:52 ` Re: [PATCH] Add support for displaying database service in psql prompt Michael Paquier <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Noah Misch @ 2025-07-07 03:00 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Michael Banck <[email protected]>; Greg Sabino Mullane <[email protected]>; PostgreSQL Hackers <[email protected]>
On Mon, Jul 07, 2025 at 11:06:06AM +0900, Michael Paquier wrote:
> On Sun, Jul 06, 2025 at 09:13:19AM -0700, Noah Misch wrote:
> > This new PQservice() function from commit 4b99fed75 came up in the annual
> > exports.txt diff. The standard in libpq has been to not clutter the API with
> > new functions that simply retrieve one PQconninfoOption value. PQconninfo()
> > provides access to all those values in a generic way. What do you think of
> > making psql use PQconninfo() for this, then removing PQservice()? The rest of
> > the commit (adding the struct field, necessary for PQconninfo() to include the
> > value) looks good.
>
> Sure, I was not aware of such a policy. Relying on PQconninfoOption
> is less efficient because we would need to look through the whole set
> of options when looking for the service name, and this needs one extra
> allocation as PQconninfoFree() frees the values allocated. With two
> callers perhaps this inefficiency is OK to live with anyway.
I think the choice to make there is whether to call PQconninfo() once per
prompt emission or to cache the value, invalidating that cache e.g. once per
SyncVariables(). My first thought was to cache, but the decision is not too
important. A PQconninfo() call is likely negligible relative to all that
happens between prompts. Even if not negligible, the overhead of not caching
will affect only prompts using the new escape sequence.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [PATCH] Add support for displaying database service in psql prompt
2025-07-06 16:13 Re: [PATCH] Add support for displaying database service in psql prompt Noah Misch <[email protected]>
2025-07-07 02:06 ` Re: [PATCH] Add support for displaying database service in psql prompt Michael Paquier <[email protected]>
2025-07-07 03:00 ` Re: [PATCH] Add support for displaying database service in psql prompt Noah Misch <[email protected]>
@ 2025-07-07 23:52 ` Michael Paquier <[email protected]>
2025-07-09 00:41 ` Re: [PATCH] Add support for displaying database service in psql prompt Noah Misch <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Michael Paquier @ 2025-07-07 23:52 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Michael Banck <[email protected]>; Greg Sabino Mullane <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sun, Jul 06, 2025 at 08:00:09PM -0700, Noah Misch wrote:
> I think the choice to make there is whether to call PQconninfo() once per
> prompt emission or to cache the value, invalidating that cache e.g. once per
> SyncVariables(). My first thought was to cache, but the decision is not too
> important. A PQconninfo() call is likely negligible relative to all that
> happens between prompts. Even if not negligible, the overhead of not caching
> will affect only prompts using the new escape sequence.
SyncVariables() happens at startup and when re-syncing a connection
during a check, so that does not really worry me.
By the way, there is a second change in the CF that's suggesting the
addition of a SERVICEFILE variable, which also uses a separate libpq
API (forgot about this one):
https://commitfest.postgresql.org/patch/5387/
Changing the patch on the other thread to use a conninfo is
stright-forward. How about extending the get_service_name()@common.c
I've proposed upthread so as it takes a string in input and it could
be reused for more connection options than only "service" so as it
could be reused there as well?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [PATCH] Add support for displaying database service in psql prompt
2025-07-06 16:13 Re: [PATCH] Add support for displaying database service in psql prompt Noah Misch <[email protected]>
2025-07-07 02:06 ` Re: [PATCH] Add support for displaying database service in psql prompt Michael Paquier <[email protected]>
2025-07-07 03:00 ` Re: [PATCH] Add support for displaying database service in psql prompt Noah Misch <[email protected]>
2025-07-07 23:52 ` Re: [PATCH] Add support for displaying database service in psql prompt Michael Paquier <[email protected]>
@ 2025-07-09 00:41 ` Noah Misch <[email protected]>
2025-07-09 04:29 ` Re: [PATCH] Add support for displaying database service in psql prompt Michael Paquier <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Noah Misch @ 2025-07-09 00:41 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Michael Banck <[email protected]>; Greg Sabino Mullane <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tue, Jul 08, 2025 at 08:52:08AM +0900, Michael Paquier wrote:
> On Sun, Jul 06, 2025 at 08:00:09PM -0700, Noah Misch wrote:
> > I think the choice to make there is whether to call PQconninfo() once per
> > prompt emission or to cache the value, invalidating that cache e.g. once per
> > SyncVariables(). My first thought was to cache, but the decision is not too
> > important. A PQconninfo() call is likely negligible relative to all that
> > happens between prompts. Even if not negligible, the overhead of not caching
> > will affect only prompts using the new escape sequence.
>
> SyncVariables() happens at startup and when re-syncing a connection
> during a check, so that does not really worry me.
>
> By the way, there is a second change in the CF that's suggesting the
> addition of a SERVICEFILE variable, which also uses a separate libpq
> API (forgot about this one):
> https://commitfest.postgresql.org/patch/5387/
>
> Changing the patch on the other thread to use a conninfo is
> stright-forward. How about extending the get_service_name()@common.c
> I've proposed upthread so as it takes a string in input and it could
> be reused for more connection options than only "service" so as it
> could be reused there as well?
I'd prefer not to get involved in decisions affecting only psql efficiency and
psql code cosmetics. Please make that decision without my input.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [PATCH] Add support for displaying database service in psql prompt
2025-07-06 16:13 Re: [PATCH] Add support for displaying database service in psql prompt Noah Misch <[email protected]>
2025-07-07 02:06 ` Re: [PATCH] Add support for displaying database service in psql prompt Michael Paquier <[email protected]>
2025-07-07 03:00 ` Re: [PATCH] Add support for displaying database service in psql prompt Noah Misch <[email protected]>
2025-07-07 23:52 ` Re: [PATCH] Add support for displaying database service in psql prompt Michael Paquier <[email protected]>
2025-07-09 00:41 ` Re: [PATCH] Add support for displaying database service in psql prompt Noah Misch <[email protected]>
@ 2025-07-09 04:29 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Michael Paquier @ 2025-07-09 04:29 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Michael Banck <[email protected]>; Greg Sabino Mullane <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tue, Jul 08, 2025 at 05:41:32PM -0700, Noah Misch wrote:
> I'd prefer not to get involved in decisions affecting only psql efficiency and
> psql code cosmetics. Please make that decision without my input.
Okay, I have used this more extensible routine then, planning to use
it for the other patch. The prompt shortcut retrieves the value using
a GetVariable(), rather than looking at the connection options all the
time.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
end of thread, other threads:[~2025-07-09 04:29 UTC | newest]
Thread overview: 16+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-03-25 11:39 [PATCH 7/7] Add a comment to ATExecSetTableSpace. Kyotaro Horiguchi <[email protected]>
2024-06-21 04:39 Re: ON ERROR in json_query and the like Pavel Stehule <[email protected]>
2024-06-21 04:46 ` Re: ON ERROR in json_query and the like David G. Johnston <[email protected]>
2024-06-21 05:28 ` Re: ON ERROR in json_query and the like Markus Winand <[email protected]>
2024-06-21 05:38 ` Re: ON ERROR in json_query and the like David G. Johnston <[email protected]>
2024-06-21 10:59 ` Re: ON ERROR in json_query and the like Markus Winand <[email protected]>
2024-06-22 08:43 ` Re: ON ERROR in json_query and the like Amit Langote <[email protected]>
2024-06-26 12:10 ` Re: ON ERROR in json_query and the like Amit Langote <[email protected]>
2024-06-28 00:49 ` Re: ON ERROR in json_query and the like Amit Langote <[email protected]>
2024-12-21 06:19 [PATCH v25 2/9] Row pattern recognition patch (parse/analysis). Tatsuo Ishii <[email protected]>
2025-07-06 16:13 Re: [PATCH] Add support for displaying database service in psql prompt Noah Misch <[email protected]>
2025-07-07 02:06 ` Re: [PATCH] Add support for displaying database service in psql prompt Michael Paquier <[email protected]>
2025-07-07 03:00 ` Re: [PATCH] Add support for displaying database service in psql prompt Noah Misch <[email protected]>
2025-07-07 23:52 ` Re: [PATCH] Add support for displaying database service in psql prompt Michael Paquier <[email protected]>
2025-07-09 00:41 ` Re: [PATCH] Add support for displaying database service in psql prompt Noah Misch <[email protected]>
2025-07-09 04:29 ` Re: [PATCH] Add support for displaying database service in psql prompt Michael Paquier <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox