Re: Idea Feedback: psql \h misses -> Offers Links?

[Kirk Wolak]

On Fri, Apr 19, 2024 at 10:14 AM Euler Taveira  wrote:

> On Wed, Apr 17, 2024, at 2:47 PM, Kirk Wolak wrote:
>
> ...
>
> This is Question 1: Do others see the potential value here?
>
>
> Yes. However, I expect an exact and direct answer. There will be cases
> that the
> first result is not the one you are looking for. (You are expecting the
> function or parameter description but other page is on the top because it
> is
> more relevant.) The referred URL does not point you to the direct link.
> Instead, you have to click again to be able to check the content.
>

Again, this does get to the point that the current search feature at
postgresql.org could be better.  I would like to see that improved as
well...


> Question 2: What if we allowed the users to set some extra link Templates
> using \pset??
>
> \pset help_assist_link_1 =  https://www.google.com/search?q={token}'
> \pset help_assist_link_2 = '
> https://wiki.postgresql.org/index.php?title=Special%3ASearch={token}=Go
> <https://wiki.postgresql.org/index.php?title=Special%3ASearch=%7Btoken%7D=Go>
> '
>
>
> That's a different idea. Are you proposing to provide URLs if this psql
> variable is set and it doesn't find an entry (say \h foo)? I'm not sure if
> it
> is a good idea to allow third-party URLs (even if it is configurable).
>

If you want to check the patch Andrey published.  We basically set the
default value to the set variable, and then allowed the user to override
that value with multiple pipe (|) separated URLs.  It does BEG the question
if this is cool for hackers.  Personally, I like the option as there are
probably a few resources worth checking against.  But if someone doesn't
change the default, they get a good enough answer.


> IMO we should expand \h to list documentation references for functions and
> GUCs
> using SGML files. We already did it for SQL commands. Another broader idea
> is
> to build an inverted index similar to what Index [1] provides. The main
> problem
> with this approach is to create a dependency between documentation build
> and
> psql. Maybe there is a reasonable way to obtain the links for each term.
>
>
> [1] https://www.postgresql.org/docs/current/bookindex.html
>

I don't want to add more dependencies into psql to the documentation for a
ton of stuff.  To me, if we had a better search page on the website for
finding things, it would be great.  I have been resigned to just googling
"postgresql " because google does a better job searching
postgresql.org than the postgresql.org site does (even when it is a known
indexed item like a function name).

Thanks for the feedback.

Re: Idea Feedback: psql \h misses -> Offers Links?

[Kirk Wolak]

On Thu, Apr 18, 2024 at 2:37 PM Peter Eisentraut 
wrote:

> On 17.04.24 19:47, Kirk Wolak wrote:
> > *Example:*
> > \h current_setting
> > No help available for "current_setting".
> > Try \h with no arguments to see available help.
> >
> > https://www.postgresql.org/search/?u=%2Fdocs%2F16%2F=current_setting
> > <https://www.postgresql.org/search/?u=%2Fdocs%2F16%2F=current_setting>
>
> One problem is that this search URL does not actually produce any useful
> information about current_setting.
>
> I see what you mean, but doesn't that imply our web search feature is
weak?  That's the full name of an existing function, and it's in the index.
But it cannot be found if searched from the website?

Re: Oom on temp (un-analyzed table caused by JIT) V16.1 [ NOT Fixed ]

[Kirk Wolak]

On Thu, Feb 22, 2024 at 4:49 PM Michael Banck  wrote:

> Hi,
>
> On Wed, Jan 24, 2024 at 02:50:52PM -0500, Kirk Wolak wrote:
> > On Mon, Jan 22, 2024 at 1:30 AM Kirk Wolak  wrote:
> > > On Fri, Jan 19, 2024 at 7:03 PM Daniel Gustafsson 
> wrote:
> > >> > On 19 Jan 2024, at 23:09, Kirk Wolak  wrote:
> > > Thank you, that made it possible to build and run...
> > > UNFORTUNATELY this has a CLEAR memory leak (visible in htop)
> > > I am watching it already consuming 6% of my system memory.
> ...
> I had a look at this (and blogged about it here[1]) and was also
> wondering what was going on with 17devel and the recent back-branch
> releases, cause I could also reproduce those continuing memory leaks.
>
> Adding some debug logging when llvm_inline_reset_caches() is called
> solves the mystery: as you are calling a function, the fix that is in
> 17devel and the back-branch releases is not applicable and only after
> the function returns llvm_inline_reset_caches() is being called (as
> llvm_jit_context_in_use_count is greater than zero, presumably, so it
> never reaches the call-site of llvm_inline_reset_caches()).
>
> If you instead run your SQL in a DO-loop (as in the blog post) and not
> as a PL/PgSQL function, you should see that it no longer leaks. This
> might be obvious to some (and Andres mentioned it in
>
> https://www.postgresql.org/message-id/20210421002056.gjd6rpe6toumiqd6%40alap3.anarazel.de
> )
> but it took me a while to figure out/find.
>
> Thanks for confirming.  Inside a routine is required.  But disabling JIT
was good enough for us.

Curious if there was another way to end up calling the cleanup?  But it had
that "feel" that the context of the cleanup was
being lost between iterations of the loop?

Idea Feedback: psql \h misses -> Offers Links?

[Kirk Wolak]

  Hackers,
  I often use the ctrl-click on the link after getting help in psql.  A
great feature.

Challenge, when there is no help, you don't get any link.

  My thought process is to add a default response that would take them to
https://www.postgresql.org/search/?u=%2Fdocs%2F16%2F={TOKEN}


*Example:*
\h current_setting
No help available for "current_setting".
Try \h with no arguments to see available help.

https://www.postgresql.org/search/?u=%2Fdocs%2F16%2F=current_setting

  To me, this is a huge step in helping me get to the docs.

This is Question 1: Do others see the potential value here?

Question 2: What if we allowed the users to set some extra link Templates
using \pset??

\pset help_assist_link_1 =  https://www.google.com/search?q={token}'
\pset help_assist_link_2 = '
https://wiki.postgresql.org/index.php?title=Special%3ASearch={token}=Go

'

Such that the output, this time would be:
*Example:*
\h current_setting
No help available for "current_setting".
Try \h with no arguments to see available help.

https://www.postgresql.org/search/?u=%2Fdocs%2F16%2F=current_setting

https://www.google.com/search?q=current_setting
https://wiki.postgresql.org/index.php?title=Special%3ASearch=current_setting=Go

This Latter feature, I would consider applying to even successful searches?
[Based on Feedback here]

Thoughts?

Re: Oom on temp (un-analyzed table caused by JIT) V16.1 [ NOT Fixed ]

[Kirk Wolak]

On Wed, Jan 24, 2024 at 4:16 PM Thomas Munro  wrote:

> On Thu, Jan 25, 2024 at 8:51 AM Kirk Wolak  wrote:
> > getrusage(RUSAGE_SELF, );
> > memory_usage_bytes = usage.ru_maxrss * 1024;
>
> FWIW log_statement_stats = on shows that in the logs.  See ShowUsage()
> in postgres.c.
>

Thank you for this, here is the *TERMINAL *(Below is the tail of the log).
Notice that the pg_backend_memory_contexts does NOT show the memory
consumed.
But your logging sure did!  (I wonder if I enable logging during planning,
but there is like 82,000 cursors being opened... (This removed the FETCH
and still leaks)


7:01:08 kwolak@postgres= # *select pg_temp.fx(497);*
NOTICE:  ("9848 kB","10 MB","638 kB")
NOTICE:  ---after close, Count a: 82636, count b: 82636
NOTICE:  ("9997 kB","10 MB","648 kB")
 fx


(1 row)

Time: 525870.117 ms (08:45.870)


*Tail*:

024-01-24 17:01:08.752 EST [28804] DETAIL:  ! system usage stats:
!   0.001792 s user, 0.00 s system, 0.005349 s elapsed
!   [560.535969 s user, 31.441656 s system total]
!   185300 kB max resident size
!   232/0 [29219648/54937864] filesystem blocks in/out
!   0/25 [0/1016519] page faults/reclaims, 0 [0] swaps
!   0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
!   10/1 [62671/9660] voluntary/involuntary context switches
2024-01-24 17:01:08.752 EST [28804] STATEMENT:  explain SELECT DISTINCT
seid, fr_field_name, st_field_name
  FROM pg_temp.parts
 WHERE seid <> 497 AND partnum >= '1'
 ORDER BY seid;
2024-01-24 17:01:08.759 EST [28804] LOG:  QUERY STATISTICS
2024-01-24 17:01:08.759 EST [28804] DETAIL:  ! system usage stats:
!   0.006207 s user, 0.92 s system, 0.006306 s elapsed
!   [560.542262 s user, 31.441748 s system total]
!*   185300 kB max resident size*
!   0/0 [29219648/54937864] filesystem blocks in/out
!   0/4 [0/1016523] page faults/reclaims, 0 [0] swaps
!   0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
!   0/1 [62672/9661] voluntary/involuntary context switches
2024-01-24 17:01:08.759 EST [28804] STATEMENT:  SELECT 'pg_temp.fx(497); --
Not run, do \dt+ parts';
2024-01-24 17:04:30.844 EST [28746] LOG:  checkpoint starting: time
2024-01-24 17:04:32.931 EST [28746] LOG:  checkpoint complete: wrote 21
buffers (0.1%); 0 WAL file(s) added, 0 removed, 0 recycled; write=2.008 s,
sync=0.006 s, total=2.087 s; sync files=15, longest=0.001 s, average=0.001
s; distance=98 kB, estimate=134 kB; lsn=0/16304D8, redo lsn=0/1630480
2024-01-24 17:11:06.350 EST [28804] LOG:  QUERY STATISTICS
2024-01-24 17:11:06.350 EST [28804] DETAIL:  ! system usage stats:
!   515.952870 s user, 6.688389 s system, 525.869933 s elapsed
!   [1076.495280 s user, 38.130145 s system total]
!*   708104 kB max resident size*
!   37/3840 [29589648/54941712] filesystem blocks in/out
!   0/327338 [0/1343861] page faults/reclaims, 0 [0] swaps
!   0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
!   22001/5216 [84675/14878] voluntary/involuntary context
switches
2024-01-24 17:11:06.350 EST [28804] STATEMENT: * select pg_temp.fx(497);*
2024-01-24 17:12:16.162 EST [28804] LOG:  QUERY STATISTICS
2024-01-24 17:12:16.162 EST [28804] DETAIL:  ! system usage stats:
!   1.130029 s user, 0.007727 s system, 1.157486 s elapsed
!   [1077.625396 s user, 38.137921 s system total]
!   *708104 kB max resident size*
!   992/0 [29590640/54941720] filesystem blocks in/out
!   3/41 [3/1343902] page faults/reclaims, 0 [0] swaps
!   0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
!   9/68 [84685/14946] voluntary/involuntary context switches
2024-01-24 17:12:16.162 EST [28804] STATEMENT:  select now();
2024-01-24 17:12:30.944 EST [28804] LOG:  QUERY STATISTICS
2024-01-24 17:12:30.944 EST [28804] DETAIL:  ! system usage stats:
!   0.004561 s user, 0.19 s system, 0.004580 s elapsed
!   [1077.630064 s user, 38.137944 s system total]
!   *708104 kB max resident size*
!   0/0 [29590640/54941728] filesystem blocks in/out
!   0/4 [3/1343906] page faults/reclaims, 0 [0] swaps
!   0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
!   0/0 [84686/14947] voluntary/involuntary context switches
2024-01-24 17:12:30.944 EST [28804] STATEMENT:  select now();

Re: Oom on temp (un-analyzed table caused by JIT) V16.1 [ NOT Fixed ]

[Kirk Wolak]

On Mon, Jan 22, 2024 at 1:30 AM Kirk Wolak  wrote:

> On Fri, Jan 19, 2024 at 7:03 PM Daniel Gustafsson  wrote:
>
>> > On 19 Jan 2024, at 23:09, Kirk Wolak  wrote:
>>
>> ...
>> ./configure  --with-llvm
>> LLVM_CONFIG=/path/to/llvm-config
>>
>> --
>> Daniel Gustafsson
>>
>
> Thank you, that made it possible to build and run...
> UNFORTUNATELY this has a CLEAR memory leak (visible in htop)
> I am watching it already consuming 6% of my system memory.
>
>
Daniel,
  In the previous email, I made note that once the JIT was enabled, the
problem exists in 17Devel.
I re-included my script, which forced the JIT to be used...

  I attached an updated script that forced the settings.
But this is still leaking memory (outside of the
pg_backend_memory_context() calls).
Probably because it's at the LLVM level?  And it does NOT happen from
planning/opening the query.  It appears I have to fetch the rows to see the
problem.

Thanks in advance.  Let me know if I should be doing something different?

Kirk Out!
PS: I was wondering if we had a function that showed total memory of the
backend.  For helping to determine if we might have a 3rd party leak?
[increase in total memory consumed not noticed by
pg_backend_memory_contexts)

#include "postgres.h"
#include 

PG_MODULE_MAGIC;

PG_FUNCTION_INFO_V1(pg_backend_memory_footprint);

Datum pg_backend_memory_footprint(PG_FUNCTION_ARGS) {
long memory_usage_bytes = 0;
struct rusage usage;

getrusage(RUSAGE_SELF, );
memory_usage_bytes = usage.ru_maxrss * 1024;

PG_RETURN_INT64(memory_usage_bytes);
}

Re: Oom on temp (un-analyzed table caused by JIT) V16.1 [Fixed Already]

[Kirk Wolak]

On Fri, Jan 19, 2024 at 7:03 PM Daniel Gustafsson  wrote:

> > On 19 Jan 2024, at 23:09, Kirk Wolak  wrote:
>
> >  From a FUTURE email, I noticed pg_jit_available() and it's set to f??
>
> Right, then this installation does not contain the necessary library to JIT
> compile the query.
>
> > Okay, so does this require a special BUILD command?
>
> Yes, it requires that you compile with --with-llvm.  If you don't have
> llvm in
> the PATH you might need to set LLVM_CONFIG to point to your llvm-config
> binary.
> With autotools that would be something like:
>
> ./configure  --with-llvm LLVM_CONFIG=/path/to/llvm-config
>
> --
> Daniel Gustafsson
>

Thank you, that made it possible to build and run...
UNFORTUNATELY this has a CLEAR memory leak (visible in htop)
I am watching it already consuming 6% of my system memory.

I am re-attaching my script.  WHICH includes the settings to FORCE JIT.
It also does an EXPLAIN so you can verify that JIT is on (this is what I
added/noticed!)
And it takes over 20 minutes to get this far.  It's still running.
I am re-attaching the script. (as I tweaked it).

This creates 90 million rows of data, so it takes a while.
I BELIEVE that it consumes far less memory if you do not fetch any rows (I
had problems reproducing it if no rows were fetched).
So, this may be beyond the planning stages.

Thanks,

Kirk Out!
CREATE TABLE pg_temp.parts
(
seid bigint,
r_field_name_1   smallint,
fr_field_namesmallint   NOT NULL,
p1_field_namevarchar(4),
qty_field_name   integer,
p5_field_namevarchar(30),
partnum  varchar(30),
st_field_namesmallint DEFAULT 0 NOT NULL
); -- drop table pg_temp.parts;

INSERT INTO pg_temp.parts (seid, partnum, qty_field_name, fr_field_name, 
st_field_name)
SELECT (RANDOM() * 500 + 1)::bigint   AS seid,
   trunc(RANDOM() * 1)::text AS 
partnum,
   CASE
   WHEN q.rnd BETWEEN 0 AND 0.45 THEN FLOOR(RANDOM() * 900) + 100 -- 
Random number in the range [100, 999]
   WHEN q.rnd BETWEEN 0.46 AND 0.96 THEN LEAST(TRUNC(FLOOR(RANDOM() * 
99) + 1000)::int, 99::int) -- Random number in the range [1000, ]
   ELSE FLOOR(RANDOM() * 900) + 100 -- Random number in the 
range [10, 99]
   END AS 
qty_field_name,
   CASE WHEN RANDOM() < 0.72 THEN 0::smallint ELSE 1::smallint END AS 
fr_field_name,
   CASE WHEN RANDOM() < 0.46 THEN 1::smallint ELSE 2::smallint END AS 
st_field_name
  FROM (SELECT RANDOM() AS rnd, x FROM GENERATE_SERIES(1, 90_000_000) x) q;

CREATE INDEX idx_parts_supid ON pg_temp.parts USING btree (seid, p1_field_name, 
partnum, st_field_name, r_field_name_1, qty_field_name);
CREATE INDEX idx_parts_p5 ON pg_temp.parts USING btree (p5_field_name, seid, 
st_field_name, r_field_name_1, p1_field_name);
CREATE INDEX idx_parts_partnum ON pg_temp.parts USING btree (partnum, seid, 
st_field_name, r_field_name_1, p1_field_name);

CREATE OR REPLACE FUNCTION pg_temp.fx(asupplier bigint = 497 )
RETURNS void
LANGUAGE plpgsql
AS
$function$
DECLARE
supplier_parts   CURSOR (sid bigint) FOR  -- Again, selecting with 
COUNT() would reduce 1 query per row!
SELECT
partnum, qty_field_name, st_field_name, sum(qty_field_name) as qty
FROM pg_temp.parts
WHERE seid = sid AND (st_field_name = 1)
GROUP BY partnum, qty_field_name, st_field_name
ORDER BY partnum, qty_field_name, st_field_name;
supplier_part_qty_matches CURSOR (sid bigint, pnum varchar(30), pqty 
bigint) FOR
SELECT DISTINCT
seid, fr_field_name, partnum, st_field_name
FROM pg_temp.parts
WHERE seid <> sid AND partnum = pnum AND qty_field_name = pqty
ORDER BY seid, partnum;

a_partnum varchar(30);
a_qty integer;
a_st  smallint;
a_cnt integer = 0;
b_partnum varchar(30);
b_fr  smallint;
b_seidbigint;
b_st  smallint;
b_cnt bigint = 0;
BEGIN
RAISE NOTICE '%', (SELECT (PG_SIZE_PRETTY(SUM(used_bytes)), 
PG_SIZE_PRETTY(SUM(total_bytes)), PG_SIZE_PRETTY(SUM(free_bytes))) FROM 
pg_get_backend_memory_contexts());
OPEN supplier_parts (asupplier);
LOOP
FETCH supplier_parts INTO a_partnum, a_qty, a_st, a_qty;
EXIT WHEN NOT FOUND;
a_cnt := a_cnt + 1;
OPEN supplier_part_qty_matches (sid := asupplier, pnum := a_partnum, 
pqty := a_qty);
LOOP
FETCH supplier_part_qty_matches INTO b_seid, b_fr, b_partnum, b_st;
b_cnt := b_cnt + 1;
EXIT WHEN TRUE;  -- no Need to loop here  One FETCH per query 
triggers the losses.
END LOOP;
CLOSE supplier_part_qty_matches;

Re: Oom on temp (un-analyzed table caused by JIT) V16.1 [Fixed Already]

[Kirk Wolak]

On Fri, Jan 19, 2024 at 4:20 AM Laurenz Albe 
wrote:

> On Thu, 2024-01-18 at 19:50 -0500, Kirk Wolak wrote:
> >   I did a little more checking and the reason I did not see the link
> MIGHT be because EXPLAIN did not show a JIT attempt.
> > I tried to use settings that FORCE a JIT...  But to no avail.
> >
> >   I am now concerned that the problem is more hidden in my use case.
> Meaning I CANNOT conclude it is fixed.
> > But I know of NO WAY to force a JIT (I lowered costs to 1, etc.  ).
> >
> >   You don't know a way to force at least the JIT analysis to happen?
> (because I already knew if JIT was off, the leak wouldn't happen).
>
> If you set the limits to 0, you can trigger it easily:
>
> SET jit = on;
> SET jit_above_cost = 0;
> SET jit_inline_above_cost = 0;
> SET jit_optimize_above_cost = 0;
>

Okay,
  I Did exactly this (just now).  But the EXPLAIN does not contain the JIT?

---
 Sort  (cost=5458075.88..5477861.00 rows=7914047 width=12)
   Sort Key: seid
   ->  HashAggregate  (cost=3910139.62..4280784.00 rows=7914047 width=12)
 Group Key: seid, fr_field_name, st_field_name
 Planned Partitions: 128
 ->  Seq Scan on parts  (cost=0.00..1923249.00 rows=2985
width=12)
   Filter: ((seid <> 497) AND ((partnum)::text >= '1'::text))
(7 rows)

>From a FUTURE email, I noticed pg_jit_available()

and it's set to f??

Okay, so does this require a special BUILD command?

postgres=# select pg_jit_available();
 pg_jit_available
--
 f
(1 row)

postgres=# \dconfig *jit*
 List of configuration parameters
Parameter|  Value
-+-
 jit | on
 jit_above_cost  | 10
 jit_debugging_support   | off
 jit_dump_bitcode| off
 jit_expressions | on
 jit_inline_above_cost   | 50
 jit_optimize_above_cost | 50
 jit_profiling_support   | off
 jit_provider| llvmjit
 jit_tuple_deforming | on
(10 rows)

Re: Oom on temp (un-analyzed table caused by JIT) V16.1 [Fixed Already]

[Kirk Wolak]

On Tue, Jan 16, 2024 at 3:43 AM Daniel Gustafsson  wrote:

> > On 16 Jan 2024, at 02:53, Kirk Wolak  wrote:
> >
> > On Mon, Jan 15, 2024 at 9:03 AM Daniel Gustafsson  <mailto:dan...@yesql.se>> wrote:
> > > On 15 Jan 2024, at 07:24, Kirk Wolak  wol...@gmail.com>> wrote:
> >...
> > Okay, I took the latest source off of git (17devel) and got it to work
> there in a VM.
> >
> > It appears this issue is fixed.  It must have been related to the issue
> originally tagged.
>
> Thanks for testing and confirming!  Testing pre-release builds on real life
> workloads is invaluable for the development of Postgres so thank you
> taking the
> time.

Daniel,
  I did a little more checking and the reason I did not see the link MIGHT
be because EXPLAIN did not show a JIT attempt.
I tried to use settings that FORCE a JIT...  But to no avail.

  I am now concerned that the problem is more hidden in my use case.
Meaning I CANNOT conclude it is fixed.
But I know of NO WAY to force a JIT (I lowered costs to 1, etc.  ).

  You don't know a way to force at least the JIT analysis to happen?
(because I already knew if JIT was off, the leak wouldn't happen).

Thanks,

Kirk Out!
PS: I assume there is no pg_jit(1) function I can call. LOL

Re: Oom on temp (un-analyzed table caused by JIT) V16.1 [Fixed Already]

[Kirk Wolak]

On Mon, Jan 15, 2024 at 9:03 AM Daniel Gustafsson  wrote:

> > On 15 Jan 2024, at 07:24, Kirk Wolak  wrote:
>
> >   You have a commit [1] that MIGHT fix this.
> > I have a script that recreates the problem, using random data in pg_temp.
> > And a nested cursor.
>
> Running your reproducer script in a tight loop for a fair bit of time on
> the
> v16 HEAD I cannot see any memory growth, so it's plausible that the
> upcoming
> 16.2 will work better in your environment.
>

Okay, I took the latest source off of git (17devel) and got it to work
there in a VM.

It appears this issue is fixed.  It must have been related to the issue
originally tagged.

Thanks!

Re: Oom on temp (un-analyzed table caused by JIT) V16.1

[Kirk Wolak]

On Mon, Jan 15, 2024 at 9:03 AM Daniel Gustafsson  wrote:

> > On 15 Jan 2024, at 07:24, Kirk Wolak  wrote:
>
> >   You have a commit [1] that MIGHT fix this.
> > I have a script that recreates the problem, using random data in pg_temp.
> > And a nested cursor.
>
> Running your reproducer script in a tight loop for a fair bit of time on
> the
> v16 HEAD I cannot see any memory growth, so it's plausible that the
> upcoming
> 16.2 will work better in your environment.
>

The script starts by creating 90 Million rows...  In my world that part of
the script, plus the indexes, etc.  Takes about 8-9 minutes.
And has no memory loss.

I used the memory reported by HTOP to track the problem.  I Forgot to
mention this.
I am curious what you used?  (Because it doesn't display symptoms [running
dog slow] until the backend has about 85% of the machines memory)


> There are up to date snapshots of the upcoming v16 minor release which
> might
> make testing easier than building postgres from source?
>
> https://www.postgresql.org/download/snapshots/


Thank you.  I have assigned that task to the guy who maintains our
VMs/Environments.
I will report back to you.

Kirk

Oom on temp (un-analyzed table caused by JIT) V16.1

[Kirk Wolak]

Daniel,
  You have a commit [1] that MIGHT fix this.
I have a script that recreates the problem, using random data in pg_temp.
And a nested cursor.

  It took me a few days to reduce this from actual code that was
experiencing this.  If I turn off JIT, the problem goes away.  (if I don't
FETCH the first row, the memory loss does not happen.  Maybe because
opening a cursor is more decoration/prepare)

  I don't have an easy way to test this script right now against the commit.
I am hopeful that your fix fixes this.

  This was my first OOM issue in PG in 3yrs of working with it.

  The problem goes away if the TABLE is analyzed, or JIT is disabled.

  The current script, if run, will consume about 25% of my system memory
(10GB).
Just call the function below until it dies if that's what you need.  The
only way to get the memory back down is to close the connection.

SELECT pg_temp.fx(497);

Surprisingly, to me, the report from pg_get_backend_memory_contexts()
doesn't really show "missing memory", which  I thought it would.  (FWIW, we
caught this with multiple rounds of testing our code, slowing down, then
crashing...  Is there ANY way to interrogate that we are above X% of system
memory so we know to let this backend go?)

It takes about 18 minutes to run on my 4 CPU VM.

For now, we are going to add some ANALYZE statements to our code.
We will consider disabling JIT.

Thanks,
Kirk
[1] = 2cf50585e54a7b0c6bc62a087c69043ae57e4252

CREATE TABLE pg_temp.parts
(
seid bigint,
r_field_name_1   smallint,
fr_field_namesmallint   NOT NULL,
p1_field_namevarchar(4),
qty_field_name   integer,
p5_field_namevarchar(30),
partnum  varchar(30),
st_field_namesmallint DEFAULT 0 NOT NULL
); -- drop table pg_temp.parts;

INSERT INTO pg_temp.parts (seid, partnum, qty_field_name, fr_field_name, 
st_field_name)
SELECT (RANDOM() * 3821 + 1)::bigint   AS seid,
   (RANDOM() * 123456789)::textAS 
partnum,
   CASE
   WHEN q.rnd BETWEEN 0 AND 0.45 THEN FLOOR(RANDOM() * 900) + 100 -- 
Random number in the range [100, 999]
   WHEN q.rnd BETWEEN 0.46 AND 0.96 THEN LEAST(TRUNC(FLOOR(RANDOM() * 
99) + 1000)::int, 99::int) -- Random number in the range [1000, ]
   ELSE FLOOR(RANDOM() * 900) + 100 -- Random number in the 
range [10, 99]
   END AS 
qty_field_name,
   CASE WHEN RANDOM() < 0.72 THEN 0::smallint ELSE 1::smallint END AS 
fr_field_name,
   CASE WHEN RANDOM() < 0.46 THEN 1::smallint ELSE 2::smallint END AS 
st_field_name
  FROM (SELECT RANDOM() AS rnd, x FROM GENERATE_SERIES(1, 90_000_000) x) q;

CREATE INDEX idx_parts_supid ON pg_temp.parts USING btree (seid, p1_field_name, 
partnum, st_field_name, r_field_name_1, qty_field_name);
CREATE INDEX idx_parts_p5 ON pg_temp.parts USING btree (p5_field_name, seid, 
st_field_name, r_field_name_1, p1_field_name);
CREATE INDEX idx_parts_partnum ON pg_temp.parts USING btree (partnum, seid, 
st_field_name, r_field_name_1, p1_field_name);

CREATE OR REPLACE FUNCTION pg_temp.fx(asupplier bigint = 497 )
RETURNS void
LANGUAGE plpgsql
AS
$function$
DECLARE
supplier_parts   CURSOR (sid bigint) FOR  -- Again, selecting with 
COUNT() would reduce 1 query per row!
SELECT
partnum, qty_field_name, st_field_name, sum(qty_field_name) as qty
FROM pg_temp.parts
WHERE seid = sid AND (st_field_name = 1)
GROUP BY partnum, qty_field_name, st_field_name
ORDER BY partnum, qty_field_name, st_field_name;
supplier_part_qty_matches CURSOR (sid bigint, pnum varchar(30), pqty 
bigint) FOR
SELECT DISTINCT
seid, fr_field_name, partnum, st_field_name
FROM pg_temp.parts
WHERE seid <> sid AND partnum = pnum AND qty_field_name = pqty
ORDER BY seid, partnum;

a_partnum varchar(30);
a_qty integer;
a_st  smallint;
a_cnt integer = 0;
b_partnum varchar(30);
b_fr  smallint;
b_seidbigint;
b_st  smallint;
b_cnt bigint = 0;
BEGIN
RAISE NOTICE '%', (SELECT (PG_SIZE_PRETTY(SUM(used_bytes)), 
PG_SIZE_PRETTY(SUM(total_bytes)), PG_SIZE_PRETTY(SUM(free_bytes))) FROM 
pg_get_backend_memory_contexts());
OPEN supplier_parts (asupplier);
LOOP
FETCH supplier_parts INTO a_partnum, a_qty, a_st, a_qty;
EXIT WHEN NOT FOUND;
a_cnt := a_cnt + 1;
OPEN supplier_part_qty_matches (sid := asupplier, pnum := a_partnum, 
pqty := a_qty);
LOOP
FETCH supplier_part_qty_matches INTO b_seid, b_fr, b_partnum, b_st;
b_cnt := b_cnt + 1;
EXIT WHEN TRUE;  -- no Need to 

Re: A tiny improvement of psql

[Kirk Wolak]

On Tue, Dec 26, 2023 at 11:26 AM Kevin Wang  wrote:

> Hello hackers!
>
> I am an Oracle/PostgreSQL DBA, I am not a PG hacker.  During my daily job,
> I find a pain that should be fixed.
>
> As you know, we can use the UP arrow key to get the previous command to
> avoid extra typing. This is a wonderful feature to save the lives of every
> DBA. However, if I type the commands like this sequence: A, B, B, B, B, B,
> B,  as you can see, B is the last command I execute.
>
> But if I try to get command A, I have to press the UP key 7 times.  I
> think the best way is: when you press the UP key, plsql should show the
> command that is different from the previous command, so the recall sequence
> should be B -> A, not B -> B -> ... -> A.  Then I only press the UP key 2
> times to get command A.
>
> I think this should change little code in psql, but it will make all DBA's
> lives much easier.  This is a strong requirement from the real DBA. Hope to
> get some feedback on this.
>
> Kevin,
  with readline, I use ctrl-r (incremental search backwards).
but if you are willing to modify your .inputrc  you can enable the "windows
cmd F5/F8 keys... Search Fwd/Bwd".
where you type B

inputrc:
# Map F8 (back) F5(forward) search like CMD
"\e[19~": history-search-backward
"\e[15~": history-search-forward

There are commented out lines tying them to Page Up/Page Down...  But 30
yrs in a CMD prompt...

The upside is that this works in bash and other programs as well...

HTH

Kirk Out!

>

Re: Adding SHOW CREATE TABLE

[Kirk Wolak]

On Fri, Jun 30, 2023 at 1:56 PM Kirk Wolak  wrote:

> On Wed, Jun 21, 2023 at 8:52 PM Kirk Wolak  wrote:
>
>> On Mon, Jun 5, 2023 at 7:43 AM Jelte Fennema  wrote:
>>
>>> On Thu, 1 Jun 2023 at 18:57, Kirk Wolak  wrote:
>>>
>>
Definitely have the questions from the previous email, but I CERTAINLY
appreciate this output.
(Don't like the +, but pg_get_viewdef() creates the view the same way)...
Will psql doing \st pg_class be able to just call/output this so that the
output is nice and clean?

At this point... I will keep pressing forward, cleaning things up.  And
then send a patch for others to play with
(Probably bad timing with wrapping up V16)



*select pg_get_tabledef('pg_class'::regclass);*
pg_get_tabledef

CREATE TABLE pg_class (oid oid NOT NULL,+
 relname name NOT NULL COLLATE "C", +
 relnamespace oid NOT NULL, +
 reltype oid NOT NULL,  +
 reloftype oid NOT NULL,+
 relowner oid NOT NULL, +
 relam oid NOT NULL,+
 relfilenode oid NOT NULL,  +
 reltablespace oid NOT NULL,+
 relpages integer NOT NULL, +
 reltuples real NOT NULL,   +
 relallvisible integer NOT NULL,+
 reltoastrelid oid NOT NULL,+
 relhasindex boolean NOT NULL,  +
 relisshared boolean NOT NULL,  +
 relpersistence "char" NOT NULL,+
 relkind "char" NOT NULL,   +
 relnatts smallint NOT NULL,+
 relchecks smallint NOT NULL,   +
 relhasrules boolean NOT NULL,  +
 relhastriggers boolean NOT NULL,   +
 relhassubclass boolean NOT NULL,   +
 relrowsecurity boolean NOT NULL,   +
 relforcerowsecurity boolean NOT NULL,  +
 relispopulated boolean NOT NULL,   +
 relreplident "char" NOT NULL,  +
 relispartition boolean NOT NULL,   +
 relrewrite oid NOT NULL,   +
 relfrozenxid xid NOT NULL, +
 relminmxid xid NOT NULL,   +
 relacl aclitem[],  +
 reloptions text[] COLLATE "C", +
 relpartbound pg_node_tree COLLATE "C"  +
) USING heap

Re: Adding SHOW CREATE TABLE

[Kirk Wolak]

On Wed, Jun 21, 2023 at 8:52 PM Kirk Wolak  wrote:

> On Mon, Jun 5, 2023 at 7:43 AM Jelte Fennema  wrote:
>
>> On Thu, 1 Jun 2023 at 18:57, Kirk Wolak  wrote:
>> > Can this get turned into a Patch?  Were you offering this code up for
>> others (me?) to pull, and work into a patch?
>> > [If I do the patch, I am not sure it gives you the value of reducing
>> what CITUS has to maintain.  But it dawns on
>> > me that you might be pushing a much bigger patch...  But I would take
>> that, as I think there is other value in there]
>>
>

> Yeah, the Citus code only handles things that Citus supports in
>> distributed tables. Which is quite a lot, but indeed not everything
>> yet. Temporary and inherited tables are not supported in this code
>> afaik. Possibly more. See the commented out
>> EnsureRelationKindSupported for what should be supported (normal
>> tables and partitioned tables afaik).
>>
>
>
Okay, apologies for the long delay on this.  I have the code Jelte
submitted working.  And I have (almost) figured out how to add the function
so it shows up in the pg_catalog...  (I edited files I should not have, I
need to know the proper process... Anyone...)

Not sure if it is customary to attach the code when asking about stuff.
For the most part, it was what Jelte Gave us with a pg_get_tabledef()
wrapper to call...

Here is the output it produces for *select
pg_get_tabledef('pg_class'::regclass); * (Feedback Welcome)

CREATE TABLE pg_class (oid oid NOT NULL, relname name NOT NULL COLLATE "C",
relnamespace oid NOT NULL, reltype oid NOT NULL, reloftype oid NOT NULL,
relowner oid NOT NULL, relam oid NOT NULL, relfilenode oid NOT NULL,
reltablespace oid NOT NULL, relpages integer NOT NULL, reltuples real NOT
NULL, relallvisible integer NOT NULL, reltoastrelid oid NOT NULL,
relhasindex boolean NOT NULL, relisshared boolean NOT NULL, relpersistence
"char" NOT NULL, relkind "char" NOT NULL, relnatts smallint NOT NULL,
relchecks smallint NOT NULL, relhasrules boolean NOT NULL, relhastriggers
boolean NOT NULL, relhassubclass boolean NOT NULL, relrowsecurity boolean
NOT NULL, relforcerowsecurity boolean NOT NULL, relispopulated boolean NOT
NULL, relreplident "char" NOT NULL, relispartition boolean NOT NULL,
relrewrite oid NOT NULL, relfrozenxid xid NOT NULL, relminmxid xid NOT
NULL, relacl aclitem[], reloptions text[] COLLATE "C", relpartbound
pg_node_tree COLLATE "C") USING heap

==
My Comments/Questions:
1) I would prefer Legible output, like below
2) I would prefer to leave off COLLATE "C"  IFF that is the DB Default
3) The USING heap... I want to pull UNLESS the value is NOT the default
(That's a theme in my comments)
4) I *THINK* including the schema would be nice?
5) This version will work with a TEMP table, but NOT EMIT "TEMPORARY"...
Thoughts?  Is emitting [pg_temp.] good enough?
6) This version enumerates sequence values (Drop always, or Drop if they
are the default values?)
7) Should I enable the pg_get_seqdef() code
8) It does NOT handle Inheritance (Yet... Is this important?  Is it okay to
just give the table structure for this table?)
9) I have not tested against Partitions, etc...  I SIMPLY want initial
feedback on Formatting

-- Legible:
CREATE TABLE pg_class (oid oid NOT NULL,
 relname name NOT NULL COLLATE "C",
 relnamespace oid NOT NULL,
 reltype oid NOT NULL,
 ...
 reloptions text[] COLLATE "C",
 relpartbound pg_node_tree COLLATE "C"
)

-- Too verbose with "*DEFAULT*" Sequence Values:
CREATE TABLE t1 (id bigint GENERATED BY DEFAULT AS IDENTITY *(INCREMENT BY
1 MINVALUE 1 MAXVALUE 9223372036854775807 START WITH 1 CACHE 1 NO CYCLE)*
NOT NULL,
 f1 text
) WITH (autovacuum_vacuum_cost_delay='0', fillfactor='80',
autovacuum_vacuum_insert_threshold='-1',
autovacuum_analyze_threshold='5',
autovacuum_vacuum_threshold='5',
autovacuum_vacuum_scale_factor='1.5')

Thanks,

Kirk...
PS: After I get feedback on Formatting the output, etc.  I will gladly
generate a new .patch file and send it along.  Otherwise Jelte gets 100% of
the credit, and I don't want to look like I am changing that.

Re: Add some more corruption error codes to relcache

[Kirk Wolak]

On Fri, Jun 16, 2023 at 9:18 AM Andrey M. Borodin 
wrote:

> Hi hackers,
>
> Relcache errors from time to time detect catalog corruptions. For example,
> recently I observed following:
> 1. Filesystem or nvme disk zeroed out leading 160Kb of catalog index. This
> type of corruption passes through data_checksums.
> 2. RelationBuildTupleDesc() was failing with "catalog is missing 1
> attribute(s) for relid 2662".
> 3. We monitor corruption error codes and alert on-call DBAs when see one,
> but the message is not marked as XX001 or XX002. It's XX000 which happens
> from time to time due to less critical reasons than data corruption.
> 4. High-availability automation switched primary to other host and other
> monitoring checks did not ring too.
>
> This particular case is not very illustrative. In fact we had index
> corruption that looked like catalog corruption.
> But still it looks to me that catalog inconsistencies (like relnatts !=
> number of pg_attribute rows) could be marked with ERRCODE_DATA_CORRUPTED.
> This particular error code in my experience proved to be a good indicator
> for early corruption detection.
>
> What do you think?
> What other subsystems can be improved in the same manner?
>
> Best regards, Andrey Borodin.
>

Andrey, I think this is a good idea.  But your #1 item sounds familiar.
There was a thread about someone creating/dropping lots of databases, who
found some kind of race condition that would ZERO out pg_ catalog entries,
just like you are mentioning.  I think he found the problem with that
relations could not be found and/or the DB did not want to start.  I just
spent 30 minutes looking for it, but my "search-fu" is apparently failing.

Which leads me to ask if there is a way to detect the corrupting write
(writing all zeroes to the file when we know better?  A Zeroed out header
when one cannot exist?)  Hoping this triggers a bright idea on your end...

Kirk...

Re: Do we want a hashset type?

[Kirk Wolak]

On Mon, Jun 26, 2023 at 4:55 PM Joel Jacobson  wrote:

> On Mon, Jun 26, 2023, at 13:06, jian he wrote:
> > Can you try to glue the attached to the hashset data type input
> > function.
> > the attached will parse cstring with double quote and not. so '{1,2,3}'
> > == '{"1","2","3"}'. obviously quote will preserve the inner string as
> > is.
> > currently int4hashset input is delimited by comma, if you want deal
> > with range then you need escape the comma.
>
> Not sure what you're trying to do here; what's the problem with
> the current int4hashset_in()?
>
> I think it might be best to focus on null semantics / three-valued logic
> before moving on and trying to implement support for more types,
> otherwise we would need to rewrite more code if we find general
> thinkos that are problems in all types.
>
> Help wanted to reason about what the following queries should return:
>
> SELECT hashset_union(NULL::int4hashset, '{}'::int4hashset);
>
> SELECT hashset_intersection(NULL::int4hashset, '{}'::int4hashset);
>
> SELECT hashset_difference(NULL::int4hashset, '{}'::int4hashset);
>
> SELECT hashset_symmetric_difference(NULL::int4hashset, '{}'::int4hashset);
>
> Should they return NULL, the empty set or something else?
>
> I've renamed hashset_merge() -> hashset_union() to better match
> SQL's MULTISET feature which has a MULTISET UNION.
>

Shouldn't they return the same thing that left(NULL::text,1) returns?
(NULL)...
Typically any operation on NULL is NULL.

Kirk...

Re: pgbnech: allow to cancel queries during benchmark

[Kirk Wolak]

On Mon, Jun 26, 2023 at 9:46 AM Yugo NAGATA  wrote:

> Hello,
>
> This attached patch enables pgbench to cancel queries during benchmark.
>
> Formerly, Ctrl+C during benchmark killed pgbench immediately, but backend
> processes executing long queries remained for a while. You can simply
> reproduce this problem by cancelling the pgbench running a custom script
> executing "SELECT pg_sleep(10)".
>
> The patch fixes this so that cancel requests are sent for all connections
> on
> Ctrl+C, and all running queries are cancelled before pgbench exits.
>
> In thread #0, setup_cancel_handler is called before the loop, the
> CancelRequested flag is set when Ctrl+C is issued. In the loop, cancel
> requests are sent when the flag is set only in thread #0. SIGINT is
> blocked in other threads, but the threads will exit after their query
> are cancelled. If thread safety is disabled or OS is Windows, the signal
> is not blocked because pthread_sigmask cannot be used.
> (I didn't test the patch on WIndows yet, though.)
>
> I choose the design that the signal handler and the query cancel are
> performed only in thread #0 because I wanted to make the behavior as
> predicable as possible. However, another design that all thread can
> received SIGINT and that the first thread that catches the signal is
> responsible to sent cancel requests for all connections may also work.
>
> Also, the array of CState that contains all clients state is changed to
> a global variable so that all connections can be accessed within a thread.
>
>
> +1
  I also like the thread #0 handling design.  I have NOT reviewed/tested
this yet.

Re: RFC: Adding \history [options] [filename] to psql (Snippets and Shared Queries)

[Kirk Wolak]

On Tue, Jun 13, 2023 at 1:59 AM Gurjeet Singh  wrote:

> On Mon, Jun 5, 2023 at 8:58 AM Kirk Wolak  wrote:
> >
> > Everyone,
> >   After recently deep diving on some readline features and optimizing my
> bash environment to have a static set of "snippets" that I can always
> find...
> >
> >   it takes just a couple of history API calls to add some interesting
> features for those  that want them.  The result of adding 3-4 such commands
> (all under \history, and with compatible flags):
> >
> > - Saving your current history without exiting (currently doable as \s
> :HISTFILE)
> > - Reloading your history file (so you can easily share something across
> sessions) w/o exiting.
> > - Stack Loading of specific history (like a shared snippets library, and
> a personal snippets library) [clearing your history, then loading them in a
> custom order]
> >
> >   The upside is really about clearly identifying and sharing permanent
> snippets, while having that list be editable externally.  Again, bringing
> teams online who don't always know the PG way of doing things (Waits,
> Locks, Space, High CPU queries, Running Queries).
> >
> >   My intention is to leverage the way PSQL keeps the Comment above the
> SQL with the SQL.
> > Then I can step backwards searching for "context" markers (Ctrl-R) or
> > --  [F8] {history-search-backward}
> >
> >   To rip through my snippets
> >
> > Kirk...
> > PS: I could do all of this under \s [options] [filename] it's just less
> clear...
>
> Understandably, there doesn't seem to be a lot of enthusiasm for this.
> If you could show others a sample/demo session of what the UI and UX
> would look like, maybe others can chime in with either their opinion
> of the behaviour, or perhaps a better/different way of achieving that.
>
> Gurjeet,
  I agree.  I've decided to do an implementation, and then explain its
usage.  There are 2-3 different use cases.
Like pasting a huge script of one liners into \e  and then executing them.
But not wanting them in your history.
\s -c  -- Clear the history
\s -r :HISTFILE

  The magic I want for snippets is (inside .psqlrc):
\s -r snippets.sql
and then let the normal histfile load.
or use
\s -r :HISTFILE and force it to  load.

  Then the hard examples and the invocation will make more sense.

Thanks for the feedback!




> Best regards,
> Gurjeet
> http://Gurje.et
>

Re: Adding SHOW CREATE TABLE

[Kirk Wolak]

On Mon, Jun 5, 2023 at 7:43 AM Jelte Fennema  wrote:

> On Thu, 1 Jun 2023 at 18:57, Kirk Wolak  wrote:
> > Can this get turned into a Patch?  Were you offering this code up for
> others (me?) to pull, and work into a patch?
> > [If I do the patch, I am not sure it gives you the value of reducing
> what CITUS has to maintain.  But it dawns on
> > me that you might be pushing a much bigger patch...  But I would take
> that, as I think there is other value in there]
>
> Attached is a patch which adds the relevant functions from Citus to
> Postgres (it compiles without warnings on my machine). I checked with
> a few others on the Citus team at Microsoft and everyone thought that
> upstreaming this was a good idea, because it's quite a bit of work to
> update with every postgres release.
>
> To set expectations though, I don't really have time to work on this
> patch. So if you can take it over from here that would be great.
>
> The patch only contains the C functions which generate SQL based on
> some oids. The wrappers such as the master_get_table_ddl_events
> function were too hard for me to pull out of Citus code, because they
> integrated a lot with other pieces. But the bulk of the logic is in
> the functions in this patch. Essentially all that
> master_get_table_ddl_events does is call the functions in this patch
> in the right order.
>
> > The ONLY thing I did not see was "CREATE TEMPORARY " syntax?  If you did
> this on a  TEMP table,
> > does it generate normal table syntax or TEMPORARY TABLE syntax???
>
> Yeah, the Citus code only handles things that Citus supports in
> distributed tables. Which is quite a lot, but indeed not everything
> yet. Temporary and inherited tables are not supported in this code
> afaik. Possibly more. See the commented out
> EnsureRelationKindSupported for what should be supported (normal
> tables and partitioned tables afaik).
>

Jelte,
  Thank you for this.
  Let me see what I can do with this, it seems like a solid starting point.
  At this point, based on previous feedback, finding a way to make
get_tabledef() etc. to work as functions is my goal.
I will see how inherited tables and temporary tables will be dealt with.

  Hopefully, this transfer works to please anyone concerned with
integrating this code into our project from the Citus code.

Kirk...

Re: Let's make PostgreSQL multi-threaded

[Kirk Wolak]

On Tue, Jun 6, 2023 at 2:00 PM Robert Haas  wrote:

>
> I'm also not quite convinced that there's no long-term use case for
> multi-process mode. Maybe you're right and there isn't, but that
> amounts to arguing that every extension in the world will be happy to
> run in a multi-threaded world rather than not. I don't know if I quite
> believe that. It also amounts to arguing that performance is going to
> be better for everyone in this new multi-threaded mode, and that it
> won't cause unforeseen problems for any significant numbers of users,
> and maybe those things are true, but I think we need to get this new
> system in place and get some real-world experience before we can judge
> these kinds of things. I agree that, in theory, it would be nice to
> get to a place where the multi-process mode is a dinosaur and that we
> can just rip it out ... but I don't share your confidence that we can
> get there in any short time period.
>

First, I am enjoying the activity of this thread.  But my first question is
"to what end"?
Do I consider threads better? (yes... and no)

I do wonder if we could add better threading within any given
session/process to get a hybrid?
[maybe this gets us closer to solving some of the problems incrementally?]

If I could have anything (today)... I would prefer a Master-Master
Implementation leveraging some
of the ultra-fast server-server communication protocols to help sync
things.  Then I wouldn't care.
I could avoid the O/S  Overwhelm caused by excessive processes, via
spinning up machines.
[Unfortunately I know that PG leverages the filesystem cache, etc to such a
degree that communicating
from one master to another would require a really special architecture
there.  And the N! communication lines].

Kirk...

RFC: Adding \history [options] [filename] to psql (Snippets and Shared Queries)

[Kirk Wolak]

Everyone,
  After recently deep diving on some readline features and optimizing my
bash environment to have a static set of "snippets" that I can always
find...

  it takes just a couple of history API calls to add some interesting
features for those  that want them.  The result of adding 3-4 such commands
(all under \history, and with compatible flags):

- Saving your current history without exiting (currently doable as \s
:HISTFILE)
- Reloading your history file (so you can easily share something across
sessions) w/o exiting.
- Stack Loading of specific history (like a shared snippets library, and a
personal snippets library) [clearing your history, then loading them in a
custom order]

  The upside is really about clearly identifying and sharing permanent
snippets, while having that list be editable externally.  Again, bringing
teams online who don't always know the PG way of doing things (Waits,
Locks, Space, High CPU queries, Running Queries).

  My intention is to leverage the way PSQL keeps the Comment above the SQL
with the SQL.
Then I can step backwards searching for "context" markers (Ctrl-R) or
--  [F8] {history-search-backward}

  To rip through my snippets

Kirk...
PS: I could do all of this under \s [options] [filename] it's just less
clear...

Re: [BUG] pg_dump does not properly deal with BEGIN ATOMIC function

[Kirk Wolak]

On Sun, Jun 4, 2023 at 1:41 PM Tom Lane  wrote:

> Kirk Wolak  writes:
> .. to strings of other lengths.  So the new output (before 016107478
> fixed it) is
>
> pg_dump: warning: could not resolve dependency loop among these items:
> pg_dump: detail: FUNCTION a_f  (ID 216 OID 40532)
> pg_dump: detail: CONSTRAINT a_pkey  (ID 3466 OID 40531)
> pg_dump: detail: POST-DATA BOUNDARY  (ID 3612)
> pg_dump: detail: TABLE DATA a  (ID 3610 OID 40525)
> pg_dump: detail: PRE-DATA BOUNDARY  (ID 3611)
>
> regards, tom lane
>
+1

Re: [BUG] pg_dump does not properly deal with BEGIN ATOMIC function

[Kirk Wolak]

On Sat, Jun 3, 2023 at 2:28 PM Tom Lane  wrote:

> Kirk Wolak  writes:
> > On Fri, Jun 2, 2023 at 8:16 AM Tom Lane  wrote:
> >   If I comprehend the suggestion, it will label each line with a warning.
> > Which implies I have 6 Warnings.
>
> Right, I'd forgotten that pg_log_warning() will interpose "warning:".
> Attached are two more-carefully-thought-out suggestions.  The easy
> way is to use pg_log_warning_detail(), which produces output like
>
> pg_dump: warning: could not resolve dependency loop among these items:
> pg_dump: detail:   FUNCTION a_f  (ID 216 OID 40532)
> pg_dump: detail:   CONSTRAINT a_pkey  (ID 3466 OID 40531)
> pg_dump: detail:   POST-DATA BOUNDARY  (ID 3612)
> pg_dump: detail:   TABLE DATA a  (ID 3610 OID 40525)
> pg_dump: detail:   PRE-DATA BOUNDARY  (ID 3611)
>
> Alternatively, we could assemble the details by hand, as in the
> second patch, producing
>
> pg_dump: warning: could not resolve dependency loop among these items:
>   FUNCTION a_f  (ID 216 OID 40532)
>   CONSTRAINT a_pkey  (ID 3466 OID 40531)
>   POST-DATA BOUNDARY  (ID 3612)
>   TABLE DATA a  (ID 3610 OID 40525)
>   PRE-DATA BOUNDARY  (ID 3611)
>
> I'm not really sure which of these I like better.  The first one
> is a much simpler code change, and there is some value in labeling
> the output like that.  The second patch's output seems less cluttered,
> but it's committing a modularity sin by embedding formatting knowledge
> at the caller level.  Thoughts?
>

Honestly the double space in front of the strings with either the Original
version,
or the "detail:" version is great.

While I get the "Less Cluttered" version.. It "detaches" it a bit too much
from the lead in, for me.

Kirk...

Re: [BUG] pg_dump does not properly deal with BEGIN ATOMIC function

[Kirk Wolak]

On Fri, Jun 2, 2023 at 8:16 AM Tom Lane  wrote:

> or with -v:
>
> pg_dump: warning: could not resolve dependency loop among these items:
> pg_dump:   FUNCTION a_f  (ID 218 OID 40664)
> pg_dump:   CONSTRAINT a_pkey  (ID 4131 OID 40663)
> pg_dump:   POST-DATA BOUNDARY  (ID 4281)
> pg_dump:   TABLE DATA a  (ID 4278 OID 40657)
> pg_dump:   PRE-DATA BOUNDARY  (ID 4280)
>
> ...
> BTW, now that I see a case the default printout here seems
> completely ridiculous.  I think we need to do
>
> pg_log_warning("could not resolve dependency loop among these items:");
> for (i = 0; i < nLoop; i++)
> {
> charbuf[1024];
>
> describeDumpableObject(loop[i], buf, sizeof(buf));
> -   pg_log_info("  %s", buf);
> +   pg_log_warning("  %s", buf);
> }
>

-1
  Not that I matter, but as a "consumer" the current output tells me:
- You have a Warning...
+ Here are the supporting details (visually, very clearly)

  If I comprehend the suggestion, it will label each line with a warning.
Which implies I have 6 Warnings.
It feels "off" to do it that way, especially since the only way we get the
additional details is with "-v"?

Kirk...

Re: Adding SHOW CREATE TABLE

[Kirk Wolak]

On Thu, Jun 1, 2023 at 4:46 PM Andrew Dunstan  wrote:

> On 2023-06-01 Th 16:39, Kirk Wolak wrote:
>
> On Thu, Jun 1, 2023 at 3:13 PM Andrew Dunstan  wrote:
>
>> On 2023-06-01 Th 12:57, Kirk Wolak wrote:
>>
>> PS: It dawned on me that if pg_dump had used server side code to generate
>> its DDL, its complexity would drop.
>>
>> Maybe, that remains to be seen. pg_dump needs to produce SQL that is
>> suitable for the target database, not the source database.
>>
>
> First, pg_dump has some special needs in addressing how it creates tables,
> to be able to load the data BEFORE indexing, and constraining (lest you
> have to struggle with dependencies of FKs, etc etc)...
>
> But version checking is interesting... Because I found no way to tell
> pg_dump what DB to target.
>
> The target version is implicitly the version it's built from.
>
Andrew,
  Thank you.  Someone else confirmed for me that the code is designed to
create accurate DDL for the pg_dump version.
So, for example, WITH OIDs are not included with later versions of pg_dump,
even though they are hitting a server with them.
Great to know.

  I like that CITUS offered up something.  I think that might be the
current best approach.  It's a win-win.  They get something,
we get something.

Kirk...

Re: Adding SHOW CREATE TABLE

[Kirk Wolak]

On Thu, Jun 1, 2023 at 3:13 PM Andrew Dunstan  wrote:

> On 2023-06-01 Th 12:57, Kirk Wolak wrote:
>
> PS: It dawned on me that if pg_dump had used server side code to generate
> its DDL, its complexity would drop.
>
> Maybe, that remains to be seen. pg_dump needs to produce SQL that is
> suitable for the target database, not the source database.
>

First, pg_dump has some special needs in addressing how it creates tables,
to be able to load the data BEFORE indexing, and constraining (lest you
have to struggle with dependencies of FKs, etc etc)...

But version checking is interesting... Because I found no way to tell
pg_dump what DB to target.  The code I did see was checking what server
options were available. (I clearly could have missed something)...  But
exactly how would that work?  All it knows is who it is (pg_dump V14, for
example.  So the best case is that it ignores new things that would be in
V15, V16 because it doesn't even know what to check for or what to do).
Which, to me, makes it more of a side-effect than a feature, no?

And that if it used a server side solution, it gets an interesting "super
power".  In that even pg_dump V20 could dump a V21 db with V21 syntax
enhancements.  (I cannot say if that is desirable or not as I lack 20yrs of
history here).  Finally, the thoughts of implementing this change at this
point, when it would impact anyone using the NEW version to dump an OLD
version without the server side support...  So that migration may never
happen.  Or only after the only supported DBs have the required server side
functions...

The feedback is welcome...

Re: Adding SHOW CREATE TABLE

[Kirk Wolak]

On Thu, May 25, 2023 at 9:23 AM Jelte Fennema  wrote:

> On Mon, 22 May 2023 at 13:52, Andrew Dunstan  wrote:
> > A performant server side set of functions would be written in C and
> follow the patterns in ruleutils.c.
>
> We have lots of DDL ruleutils in our Citus codebase:
>
> https://github.com/citusdata/citus/blob/main/src/backend/distributed/deparser/citus_ruleutils.c
>
> I'm pretty sure we'd be happy to upstream those if that meant, we
> wouldn't have to update them for every postgres release.
>
> We also have the master_get_table_ddl_events UDF, which does what SHOW
> CREATE TABLE would do.
>

Jelte, this looks promising, although it is a radically different approach
(Querying from it to get the details).

I was just getting ready to write up a bit of  an RFC... On the following
approach...

I have been trying to determine how to "focus" this effort to move it
forward.  Here is where I am at:
1) It should be 100% server side (and psql \st would only work by calling
the server side code, if it was there)
 In reviewing... This simplifies the implementation to the current
version of PG DDL being generated.
 Also, as others have mentioned, it should be C based code, and use
only the internal tables.
2) Since pg_get_{ triggerdef | indexdef | constraintdef } already exists, I
was strongly recommending to not include those.
-- Although including the inlined constraints would be fine by me
(potentially a boolean to turn it off?)
3) Then focusing the reloptions WITH (%s)

It appears CITUS code handles ALL of this on a cursory review!

The ONLY thing I did not see was "CREATE TEMPORARY " syntax?  If you did
this on a  TEMP table,
does it generate normal table syntax or TEMPORARY TABLE syntax???

So, from my take... This is a great example of solving the problem with
existing "Production Quality" Code...
I like it...

Can this get turned into a Patch?  Were you offering this code up for
others (me?) to pull, and work into a patch?
[If I do the patch, I am not sure it gives you the value of reducing what
CITUS has to maintain.  But it dawns on
me that you might be pushing a much bigger patch...  But I would take that,
as I think there is other value in there]

Others???

Thanks,

Kirk...
PS: It dawned on me that if pg_dump had used server side code to generate
its DDL, its complexity would drop.

Re: Should CSV parsing be stricter about mid-field quotes?

[Kirk Wolak]

On Mon, May 22, 2023 at 12:13 PM Daniel Verite 
wrote:

> Joel Jacobson wrote:
>
> > Is there a valid reason why \. is needed for COPY FROM filename?
> > It seems to me it would only be necessary for the COPY FROM STDIN case,
> > since files have a natural end-of-file and a known file size.
>
> Looking at CopyReadLineText() over at [1], I don't see a reason why
> the unquoted \. could not be handled with COPY FROM file.
> Even COPY FROM STDIN looks like it could be benefit, so that
> \copy from file csv would hopefully not choke or truncate the data.
> There's still the case when the CSV data is embedded in a psql script
> (psql is unable to know where it ends), but for that, "don't do that"
> might be a reasonable answer.
>

Don't have what looks like a pg_dump script?
We specifically create such SQL files with embedded data.  Depending on the
circumstances,
we either confirm that indexes dropped and triggers are disabled...  [Or we
create a dynamic name,
and insert it into a queue table for later processing], and then we COPY
the data, ending in
\.

We do NOT do "CSV", we mimic pg_dump.

Now, if you are talking about only impacting a fixed data file format...
Sure.  But impacting how psql
processes these \i  included files...  (that could hurt)

Re: Adding SHOW CREATE TABLE

[Kirk Wolak]

On Fri, May 19, 2023 at 1:08 PM Andrew Dunstan  wrote:

> I think the ONLY place we should have this is in server side functions.
> More than ten years ago I did some work in this area (see below), but it's
> one of those things that have been on my ever growing personal TODO list
>
> See 
>  and
> 
> 
>
Andrew,
  Thanks for sharing that.  I reviewed your code.  10yrs, clearly it's not
working (as-is, but close), something interesting about the
structure you ended up in.  You check the type of the object and redirect
accordingly at the top level.  Hmmm...
What I liked was that each type gets handled (I was focused on "table"),
but I realized similarities.

  I don't know what the group would think, but I like the thought of
calling this, and having it "Correct" to call the appropriate function.
But not sure it will stand.  It does make obvious that some of these should
be spun out as "pg_get_typedef"..
pg_get_typedef
pg_get_domaindef
pg_get_sequencedef

  Finally, since you started this a while back, part of me is "leaning"
towards a function:
pg_get_columndef

  Which returns a properly formatted column for a table, type, or domain?
(one of the reasons for this, is that this is
the function with the highest probability to change, and potentially the
easiest to share reusability).

  Finally, I am curious about your opinion.  I noticed you used the
internal pg_ tables, versus the information_schema...
I am *thinking* that the information_schema will be more stable over
time... Thoughts?

Thank you for sharing your thoughts...
Kirk...

Re: Adding SHOW CREATE TABLE

[Kirk Wolak]

On Sat, May 20, 2023 at 2:33 PM Stephen Frost  wrote:

> Greetings,
>
> On Sat, May 20, 2023 at 13:32 David G. Johnston <
> david.g.johns...@gmail.com> wrote:
>
>> On Sat, May 20, 2023 at 10:26 AM Stephen Frost 
>> wrote:
>>
>>> > A server function can be conveniently called from any client code.
>>>
>>> Clearly any client using libpq can conveniently call code which is in
>>> libpq.
>>>
>>
>> Clearly there are clients that don't use libpq.  JDBC comes to mind.
>>
>
> Indeed … as I mentioned up-thread already.
>
> Are we saying that we want this to be available server side, and largely
> duplicated, specifically to cater to non-libpq users?  I’ll put out there,
> again, the idea that perhaps we put it into the common library then and
> make it available via both libpq and as a server side function ..?
>
> We also have similar code in postgres_fdw.. ideally, imv anyway, we’d not
> end up with three copies of it.
>
> Thanks,
>
> Stephen
>

First, as the person chasing this down, and a JDBC user, I really would
prefer pg_get_tabledef() as Laurenz mentioned.

Next, I have reviewed all 3 implementations (pg_dump [most appropriate],
psql \d (very similar), and the FDW which is "way off",
since it actually focuses on "CREATE FOREIGN TABLE" exclusively, and
already fails to handle many pieces not required in
creating a "real" table, as it creates a "reflection" of table.

I am using pg_dump as my source of truth.  But I noticed it does not create
"TEMPORARY" tables with that syntax.
[Leading to a question on mutating the pg_temp_# schema name back to
pg_temp. or just stripping it, in favor of the TEMPORARY]

I was surprised to see ~ 2,000 lines of code in the FDW and in psql...
Whereas pg_dump is shorter because it gets more detailed
table information in a structure passed in.

I would love to leverage existing code, in the end.  But I want to take my
time on this, and become intimate with the details.
Each of the above 3 approaches have different goals.  And I would prefer
the lowest risk:reward possible, and the least expensive
maintenance.  Having it run server side hides a ton of details, and as Tom
pointed out, obviates DDL versioning control for other server versions.

Thanks for the references to the old discussions.  I have queued them up to
review.

Kirk...

Re: Should CSV parsing be stricter about mid-field quotes?

[Kirk Wolak]

On Wed, May 17, 2023 at 5:47 PM Joel Jacobson  wrote:

> On Wed, May 17, 2023, at 19:42, Andrew Dunstan wrote:
> > You can use CSV mode pretty reliably for TSV files. The trick is to use a
> > quoting char that shouldn't appear, such as E'\x01' as well as setting
> the
> > delimiter to E'\t'. Yes, it's far from obvious.
>
> I've been using that trick myself many times in the past, but thanks to
> this
> deep-dive into this topic, it looks to me like TEXT would be a better
> format
> fit when dealing with unquoted TSV files, or?
>
> OTOH, one would then need to inspect the TSV file doesn't contain \. on an
> empty
> line...
>
> I was about to suggest we perhaps should consider adding a TSV format, that
> is like TEXT excluding the PostgreSQL specific things like \. and \N,
> but then I tested exporting TSV from Numbers on Mac and Google Sheets,
> and I can see there are incompatible differences. Numbers quote fields
> that contain double-quote marks, while Google Sheets doesn't.
> None of them (unsurpringly) uses midfield quoting though.
>
> Anyone using Excel that could try exporting the following example as
> CSV/TSV?
>
> CREATE TABLE t (a text, b text, c text, d text);
> INSERT INTO t (a, b, c, d)
> VALUES ('unquoted','a "quoted" string', 'field, with a comma', E'field\t
> with a tab');
>
>
Here you go. Not horrible handling.  (I use DataGrip so I saved it from
there directly as TSV,
just for an extra datapoint).

FWIW, if you copy/paste in windows, the data, the field with the tab gets
split into another column in Excel.
But saving it as a file, and opening it.
Saving it as XLSX, and then having Excel save it as a TSV (versus opening a
text file, and saving it back)

Kirk...
a   b   c   d
unquoted"a ""quoted"" string"   "field, with a comma"   "field   with a 
tab"
a,b,c,d
unquoted,"a ""quoted"" string","field, with a comma","field	 with a tab"
a   b   c   d
unquoted"a ""quoted"" string"   "field, with a comma"   "field   with a 
tab"


t_test_DataGrip.tsv
Description: Binary data

Re: psql: Could we get "-- " prefixing on the **** QUERY **** outputs? (ECHO_HIDDEN)

[Kirk Wolak]

On Wed, May 17, 2023 at 2:13 PM Tom Lane  wrote:

> Laurenz Albe  writes:
> > You removed the  QUERY  at the end of the query.
>
> Fixed
Also Fixed Pavel's name.
Also Added Laurenze as a Reviewed By: (not sure, never want to NOT ack
someone)

>
> Also, you'd have to avoid copying-and-pasting the query output
> anyway, so I'm not entirely sold that there's much of
> a usability gain here.
>

My output never contains query output results intermixed.  I get a handful
of queries.
Then I get the output of the "\d t1"  (Which makes me wonder if I am doing
something wrong,
or there is another use case I should be testing).

I labelled this v2.  I also edited the Thread: (I realized I can find the
thread, go to the Whole Thread,
and then include the link to the first item in the thread.  I assume that
is what's expected).

Kirk...

psql>
create table t1(id bigint not null primary key generated always as
identity);
\set ECHO_HIDDEN on
\d t1

Generates:
/* QUERY **/
... Clipped ...
FROM pg_catalog.pg_publication p
WHERE p.puballtables AND pg_catalog.pg_relation_is_publishable('24577')
ORDER BY 1;
/**/

/* QUERY **/
SELECT c.oid::pg_catalog.regclass
FROM pg_catalog.pg_class c, pg_catalog.pg_inherits i
WHERE c.oid = i.inhparent AND i.inhrelid = '24577'
  AND c.relkind != 'p' AND c.relkind != 'I'
ORDER BY inhseqno;
/**/

/* QUERY **/
SELECT c.oid::pg_catalog.regclass, c.relkind, inhdetachpending,
pg_catalog.pg_get_expr(c.relpartbound, c.oid)
FROM pg_catalog.pg_class c, pg_catalog.pg_inherits i
WHERE c.oid = i.inhrelid AND i.inhparent = '24577'
ORDER BY pg_catalog.pg_get_expr(c.relpartbound, c.oid) = 'DEFAULT',
c.oid::pg_catalog.regclass::pg_catalog.text;
/**/

   Table "public.t1"
 ... End Clip...
-- NOTICE: there is no output between queries using ECHO_HIDDEN
From e8b520941154a09b350adf73cabc18dcc0596515 Mon Sep 17 00:00:00 2001
From: Kirk Wolak 
Date: Wed, 17 May 2023 16:59:02 -0400
Subject: [PATCH] [PATCH v2] Change output of ECHO_HIDDEN comments to be SQL 
comments

Simply Add the "/" Required before/after the * QUERY ** 
(SHOW_HIDDEN on)
so that they are comments and are ignored if you copy and paste a series of 
them.

Author: Kirk Wolak 
Reviewed By: Pavel Stehule 
Reviewed By: Laurenz Albe 
Thread: 
https://postgr.es/m/caclu5mtfjrjytbvmz26txggmxwqo0hkghh2o3hequupmsbg...@mail.gmail.com
---
 src/bin/psql/command.c | 8 
 src/bin/psql/common.c  | 8 
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 607a57715a..186beb83f5 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -5388,16 +5388,16 @@ echo_hidden_command(const char *query)
 {
if (pset.echo_hidden != PSQL_ECHO_HIDDEN_OFF)
{
-   printf(_("* QUERY **\n"
+   printf(_("/* QUERY **/\n"
 "%s\n"
-"**\n\n"), query);
+"/**/\n\n"), query);
fflush(stdout);
if (pset.logfile)
{
fprintf(pset.logfile,
-   _("* QUERY **\n"
+   _("/* QUERY **/\n"
  "%s\n"
- "**\n\n"), 
query);
+ "/**/\n\n"), 
query);
fflush(pset.logfile);
}
 
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index c0e6e8e6ed..07a6c33fb4 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -589,16 +589,16 @@ PSQLexec(const char *query)
 
if (pset.echo_hidden != PSQL_ECHO_HIDDEN_OFF)
{
-   printf(_("* QUERY **\n"
+   printf(_("/* QUERY **/\n"
 "%s\n"
-"**\n\n"), query);
+"/**/\n\n"), query);
fflush(stdout);
if (pset.logfile)
{
fprintf(pset.logfile,
-   _("* QUERY **\n"
+   _("/* QUERY **/\n"
  "%s\n"
- "**\n\n"), 
query);
+ "/**/\n\n"), 
query);
fflush(pset.logfile);
}
 
-- 
2.34.1

Re: psql: Could we get "-- " prefixing on the **** QUERY **** outputs? (ECHO_HIDDEN)

[Kirk Wolak]

On Mon, May 15, 2023 at 9:05 PM Kirk Wolak  wrote:

> On Mon, May 15, 2023 at 10:28 AM Tom Lane  wrote:
>
>> Alvaro Herrera  writes:
>> > It's worth considering what will readline history do with the comment.
>>
>
Hmmm... We could put a SPACE before the comment, that usually stops
readline from saving it?

>
>> Meh ... the one after serves to separate a query from its output.
>>
>> regards, tom lane
>>
>
>
> I just tested whether or not you see the trailing comment line.  And I
> ONLY see it in the windows version of PSQL.
> And ONLY if you paste it directly in at the command line.
> [Because it sends the text line by line, I assume]
>
> ,,,With that said, I DEFINITELY Move to Remove the secondary comment.
> It's just noise.
> and /* */ comments it will be for the topside.
>
>
Here's the patch.  I removed touching on .po files.
I made the change apply to the logging (fair warning) for consistency.

All feedback is welcome.  These small patches help me work through the
process.

Kirk...
OUTPUT:
/* QUERY **/
SELECT c.oid::pg_catalog.regclass
FROM pg_catalog.pg_class c, pg_catalog.pg_inherits i
WHERE c.oid = i.inhparent AND i.inhrelid = '24577'
  AND c.relkind != 'p' AND c.relkind != 'I'
ORDER BY inhseqno;

/* QUERY **/
SELECT c.oid::pg_catalog.regclass, c.relkind, inhdetachpending,
pg_catalog.pg_get_expr(c.relpartbound, c.oid)
FROM pg_catalog.pg_class c, pg_catalog.pg_inherits i
WHERE c.oid = i.inhrelid AND i.inhparent = '24577'
ORDER BY pg_catalog.pg_get_expr(c.relpartbound, c.oid) = 'DEFAULT',
c.oid::pg_catalog.regclass::pg_catalog.text;

   Table "public.t1"
 Column |  Type  | Collation | Nullable |   Default
++---+--+--
 id | bigint |   | not null | generated always as identity
Indexes:
"t1_pkey" PRIMARY KEY, btree (id)
From 31d85797a45e86660266e693d6c23c30b34935cc Mon Sep 17 00:00:00 2001
From: Kirk Wolak 
Date: Wed, 17 May 2023 13:15:57 -0400
Subject: [PATCH] Change output of ECHO_HIDDEN comments to be SQL comments

Simply Add the "/" Required before/after the * QUERY ** 
(SHOW_HIDDEN on)
so that they are comments and are ignored if you copy and paste a series of 
them.
Also removed the trailing row of asterisks.  When you have more than one query
they are just noise.  The query ends with an extra \n for spacing.

Author: Kirk Wolak 
Reviewed-By: "ok...@github.com" 
Thread: 
https://postgr.es/m/caclu5msysc-7waxne0bsrph7qw9bq9dw5m26swmq6x6isd1...@mail.gmail.com
---
 src/bin/psql/command.c | 10 --
 src/bin/psql/common.c  | 10 --
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 607a57715a..c26b807590 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -5388,16 +5388,14 @@ echo_hidden_command(const char *query)
 {
if (pset.echo_hidden != PSQL_ECHO_HIDDEN_OFF)
{
-   printf(_("* QUERY **\n"
-"%s\n"
-"**\n\n"), query);
+   printf(_("/* QUERY **/\n"
+"%s\n\n"), query);
fflush(stdout);
if (pset.logfile)
{
fprintf(pset.logfile,
-   _("* QUERY **\n"
- "%s\n"
- "**\n\n"), 
query);
+   _("/* QUERY **/\n"
+ "%s\n\n"), query);
fflush(pset.logfile);
}
 
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index c0e6e8e6ed..39155ca0d8 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -589,16 +589,14 @@ PSQLexec(const char *query)
 
if (pset.echo_hidden != PSQL_ECHO_HIDDEN_OFF)
{
-   printf(_("* QUERY **\n"
-"%s\n"
-"**\n\n"), query);
+   printf(_("/* QUERY **/\n"
+"%s\n\n"), query);
fflush(stdout);
if (pset.logfile)
{
fprintf(pset.logfile,
-   _("* QUERY **\n"
- "%s\n"
- "**\n\n"), 
query);
+   _("/* QUERY **/\n"
+ "%s\n\n"), query);
fflush(pset.logfile);
}
 
-- 
2.34.1

How do I set a different language to test psql? (/**** QUERY ****/)

[Kirk Wolak]

Team,
  I made the / QUERY / changes.
And I found the .po files, and modified those to match.

  make checkworld   -- worked

  Anyway, I have NO IDEA how I run psql in a different language.
I tried gnome-language-selector, I installed Russian/Czech.
I switched my entire system to Russian.

  git, make, all respond in russian.

  But not psql?  (First, I assume that I should check that the translations
worked),
Second... Does ANYONE do this?  (I snapshotted my VM in case it gets so bad
that I have to revert to get English back, so at least I have a safety net).

Thanks in advance.

Kirk...
PS: I would have posted the patch files, but I want to work on the tooling
to generate proper patch headers.

Re: Adding SHOW CREATE TABLE

[Kirk Wolak]

On Sun, May 14, 2023 at 2:20 AM Kirk Wolak  wrote:

> On Sat, May 13, 2023 at 3:34 PM Jeremy Smith 
> wrote:
>
>>
>>
>> On Sat, May 13, 2023, 3:25 AM Kirk Wolak  wrote:
>>
>>> Does this imply SQL SYNTAX like:
>>>
>>> SHOW CREATE TABLE 
>>>   [ INCLUDING { ALL | INDEXES |  SEQUENCES | ??? }]
>>>   [EXCLUDING { PK | FK | COMMENTS | STORAGE | } ]
>>>   [FOR {V11 | V12 | V13 | V14 | V15 }] ??
>>> ?
>>>
>>
>> Personally, I would expect a function, like pg_get_tabledef(oid), to
>> match the other pg_get_*def functions instead of overloading SHOW.  To me,
>> this also argues that we shouldn't include indexes because we already have
>> a pg_get_indexdef function.
>>
>>   -Jeremy
>>
> +1
>
> In fact, making it a function will make my life easier for testing, that's
> for certain.  I don't need to involve the parser,etc.  Others can help with
> that after the function works.
> Thanks for the suggestion!
>

I am moving this over to the Hackers Group.
My approach for now is to develop this as the \st command.
After reviewing the code/output from the 3 sources (psql, fdw, and
pg_dump).  This trivializes the approach,
and requires the smallest set of changes (psql is already close with
existing queries, etc).

And frankly, I would rather have an \st feature that handles 99% of the use
cases then go 15yrs waiting for a perfect solution.
Once this works well for the group.  Then, IMO, that would be the time to
discuss moving it.

Support Or Objections Appreciated.
Kirk...

Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN

[Kirk Wolak]

On Tue, May 16, 2023 at 1:14 AM Michael Paquier  wrote:

> On Mon, May 15, 2023 at 06:01:02PM -0700, Andres Freund wrote:
> > Why those tweaks are necessary is precisely what I am asking for.
>
> Then the perl script generates the same structures for all the wait
> event classes, with all the events associated to that.  There may be a
> point in keeping extension and buffer pin out of this refactoring, but
> reworking these like that moves in a single place all the wait event
> definitions, making future additions easier.  Buffer pin actually
> needed a small rename to stick with the naming rules of the other
> classes.
>
+1 (Nice explanation, Improving things, Consistency. Always good)

>
> These are the two things refactored in the patch, explaining the what.
> The reason behind the why is to make the script in charge of
> generating all these structures and functions consistent for all the
> wait event classes, simply.  Treating all the wait event classes
> together eases greatly the generation of the documentation, so that it
> is possible to enforce an ordering of the tables of each class used to
> list each wait event type attached to them.  Does this explanation
> make sense?
>
+1 (To me, but I am not important. But having this saved in the thread!!!)

>
> Lock and LWLock are funkier because of the way they grab wait events
> for the inner function, still these had better have their
> documentation generated so as all the SGML tables created for all the
> wait event tables are ordered alphabetically, in the same way as
> HEAD.
> --
> Michael
>
+1 (Whatever helps automate/generate the docs)

Kirk...

pgbench: can we add a way to specify the schema to write to?

[Kirk Wolak]

Currently, I believe that we are frowning on writing things directly to
Public by default.

Also, we had already taken that step in our systems.  Furthermore we force
Public to be the first Schema, so this restriction enforces that everything
created must be assigned to the appropriate schema.

That could make us a bit more sensitive to how pgbench operates.  But this
becomes an opportunity to sharpen a tool.

But logging in to our regular users it has no ability to create it's tables
and do it's work.  At the same time, we want exactly this login, because we
want to benchmark things in our application.

Our lives would be simplified if there was a simple way to specify a schema
for pgbench to use.  It would always reference that schema, and it would
not be in the path.  Making it operate "just like our apps" while benching
marking our items.

While we are here, SHOULD we consider having pgbench have a default schema
name it creates and then cleans up?  And then if someone wants to override
that, they can?

Kirk...

Re: psql: Could we get "-- " prefixing on the **** QUERY **** outputs? (ECHO_HIDDEN)

[Kirk Wolak]

On Mon, May 15, 2023 at 10:28 AM Tom Lane  wrote:

> Alvaro Herrera  writes:
> > It's worth considering what will readline history do with the comment.
> > As I recall, we keep /* comments */ together with the query that
> > follows, but the -- comments are keep in a separate history entry.
> > So that's one more reason to prefer /*  */
>
> Good point.
>
> > (To me, that also suggests to remove the asterisk line after each query,
> > and to keep just the one before.)
>
> Meh ... the one after serves to separate a query from its output.
>
> regards, tom lane
>

Actually, I love the feedback!

I just tested whether or not you see the trailing comment line.  And I ONLY
see it in the windows version of PSQL.
And ONLY if you paste it directly in at the command line.
[Because it sends the text line by line, I assume]

Further Testing:

calling with: psql -f  -- no output of the comments (or the query is seen)
-- Windows/Linux

with \e editing... In Linux nothing is displayed from the query!

with \e editing in Windows... I found it buggy when I tossed in (\pset
pager 0) as the first line.  It blew everything up (LOL)
\pset: extra argument "attcollation," ignored
\pset: extra argument "a.attidentity," ignored
\pset: extra argument "a.attgenerated" ignored
\pset: extra argument "FROM" ignored
\pset: extra argument "pg_catalog.pg_attribute" ignored


With that said, I DEFINITELY Move to Remove the secondary comment.  It's
just noise.
and /* */ comments it will be for the topside.

Also, I will take a quick peek at the parse failure that is in windows \e
[Which always does this weird doubling of lines].  But no promises here.
It will be good enough to identify the problem.

Kirk...

Re: psql: Could we get "-- " prefixing on the **** QUERY **** outputs? (ECHO_HIDDEN)

[Kirk Wolak]

On Mon, May 15, 2023 at 2:37 AM Pavel Stehule 
wrote:

> Hi
>
> Dne po 15. 5. 2023 8:01 uživatel Kirk Wolak  napsal:
>
>> This would be a trivial change.  Willing to do it, and push it.
>>
>> In effect, we have this GREAT feature:
>> \set ECHO_HIDDON on
>> -- **
>>
>> Kirk...
>>
>
> This looks little bit strange
>
> What about /* comments
>
> Like
>
> /*** Query /
>
> Or just
>
>  Query 
>
> Regards
>
> Pavel
>

Actually, I am open to suggestions.
/* */
Are good comments, usually safer!

psql: Could we get "-- " prefixing on the **** QUERY **** outputs? (ECHO_HIDDEN)

[Kirk Wolak]

This would be a trivial change.  Willing to do it, and push it.

In effect, we have this GREAT feature:
\set ECHO_HIDDON on

Which outputs a bunch of queries (as you all know).
But somehow nobody thought that a user might want to paste ALL of the
queries into their query editor, or even into another psql session, via (\e)
and NOT get a ton of syntax errors?

As an example: (added -- and a space)

-- * QUERY **
SELECT c2.relname, i.indisprimary, i.indisunique, i.indisclustered,
i.indisvalid, pg_catalog.pg_get_indexdef(i.indexrelid, 0, true),
  pg_catalog.pg_get_constraintdef(con.oid, true), contype, condeferrable,
condeferred, i.indisreplident, c2.reltablespace
FROM pg_catalog.pg_class c, pg_catalog.pg_class c2, pg_catalog.pg_index i
  LEFT JOIN pg_catalog.pg_constraint con ON (conrelid = i.indrelid AND
conindid = i.indexrelid AND contype IN ('p','u','x'))
WHERE c.oid = '21949943' AND c.oid = i.indrelid AND i.indexrelid = c2.oid
ORDER BY i.indisprimary DESC, c2.relname;
-- **

-- * QUERY **
SELECT pol.polname, pol.polpermissive,
  CASE WHEN pol.polroles = '{0}' THEN NULL ELSE
pg_catalog.array_to_string(array(select rolname from pg_catalog.pg_roles
where oid = any (pol.polroles) order by 1),',') END,
  pg_catalog.pg_get_expr(pol.polqual, pol.polrelid),
  pg_catalog.pg_get_expr(pol.polwithcheck, pol.polrelid),
  CASE pol.polcmd
WHEN 'r' THEN 'SELECT'
WHEN 'a' THEN 'INSERT'
WHEN 'w' THEN 'UPDATE'
WHEN 'd' THEN 'DELETE'
END AS cmd
FROM pg_catalog.pg_policy pol
WHERE pol.polrelid = '21949943' ORDER BY 1;
-- **

Kirk...

PSQL Should \sv & \ev work with materialized views?

[Kirk Wolak]

Personally I would appreciate it if \sv actually showed you the DDL.
Oftentimes I will \ev something to review it, with syntax highlighting.

Obviously this won't go in until V17, but looking at other tab-completion
fixes.

This should not be that difficult.  Just looking for feedback.
Admittedly \e is questionable, because you cannot really apply the changes.
ALTHOUGH, I would consider that I could
BEGIN;
DROP MATERIALIZED VIEW ...;
CREATE MATERIALIZED VIEW ...;

Which I had to do to change the WITH DATA so it creates with data when we
reload our object.s

Kirk...

Re: psql tests hangs

[Kirk Wolak]

On Fri, May 12, 2023 at 2:40 AM Pavel Stehule 
wrote:

> pá 12. 5. 2023 v 8:20 odesílatel Kirk Wolak  napsal:
>
>> On Fri, May 12, 2023 at 1:46 AM Pavel Stehule 
>> wrote:
>>
>>> pá 12. 5. 2023 v 6:50 odesílatel Kirk Wolak  napsal:
>>>
>>>> On Fri, May 12, 2023 at 12:14 AM Tom Lane  wrote:
>>>>
>>>>> Kirk Wolak  writes:
>>>>> > Did you try the print statement that Andrey asked Pavel to try?
>>>>> ...
>>>>
>>>>
>>> The strange thing is hanging. Broken tests depending on locale are
>>> usual. But I didn't remember hanging.
>>>
>>> Regards
>>>
>>> Pavel
>>>
>>
>> So, if you do psql -c "..."
>> with both of those \watch  instructions, do either one hang? (I am now
>> guessing "no")
>>
>> I know that perl is using a special library to "remote control psql"
>> (like a pseudo terminal, I guess).
>> [I had to abort some of the perl testing in Windows because that perl
>> library didn't work with my psql in Windows]
>>
>> Next, can you detect which process is hanging? (is it perl, the library,
>> psql, ?).
>>
>
> It hangs in perl
>
> but now I found there is dependency on PSQL_PAGER setting
>
> it started pager in background, I had lot of zombie pspg processes
>
> Unfortunately, when I unset this variable, the test hangs still
>
> here is backtrace
>
> Missing separate debuginfos, use: dnf debuginfo-install
> perl-interpreter-5.36.1-496.fc38.x86_64
> (gdb) bt
> #0  0x7fbbc1129ade in select () from /lib64/libc.so.6
> #1  0x7fbbc137363b in Perl_pp_sselect () from /lib64/libperl.so.5.36
> #2  0x7fbbc1317958 in Perl_runops_standard () from
> /lib64/libperl.so.5.36
> #3  0x7fbbc128259d in perl_run () from /lib64/libperl.so.5.36
> #4  0x56392bd9034a in main ()
>
> It is waiting on reading from pipe probably
>
> psql is living too, and it is waiting too
>
> Using host libthread_db library "/lib64/libthread_db.so.1".
> 0x7f071740bc37 in wait4 () from /lib64/libc.so.6
> Missing separate debuginfos, use: dnf debuginfo-install
> glibc-2.37-4.fc38.x86_64 ncurses-libs-6.4-3.20230114.fc38.x86_64
> readline-8.2-3.fc38.x86_64
> (gdb) bt
> #0  0x7f071740bc37 in wait4 () from /lib64/libc.so.6
> #1  0x7f07173a9a10 in _IO_proc_close@@GLIBC_2.2.5 () from
> /lib64/libc.so.6
> #2  0x7f07173b51e9 in __GI__IO_file_close_it () from /lib64/libc.so.6
> #3  0x7f07173a79fb in fclose@@GLIBC_2.2.5 () from /lib64/libc.so.6
> #4  0x00406be4 in do_watch (query_buf=query_buf@entry=0x5ae540,
> sleep=sleep@entry=0.01, iter=0, iter@entry=3) at command.c:5348
> #5  0x004087a5 in exec_command_watch 
> (scan_state=scan_state@entry=0x5ae490,
> active_branch=active_branch@entry=true, query_buf=query_buf@entry=0x5ae540,
> previous_buf=previous_buf@entry=0x5ae560) at command.c:2875
> #6  0x0040d4ba in exec_command (previous_buf=0x5ae560,
> query_buf=0x5ae540, cstack=0x5ae520, scan_state=0x5ae490, cmd=0x5ae9a0
> "watch") at command.c:413
> #7  HandleSlashCmds (scan_state=scan_state@entry=0x5ae490,
> cstack=cstack@entry=0x5ae520, query_buf=0x5ae540, previous_buf=0x5ae560)
> at command.c:230
>
> I am not sure, it is still doesn't work but probably there are some
> dependencies on my setting
>
> PSQL_PAGER and PSQL_WATCH_PAGER
>
> so this tests fails due my setting
>
> [pavel@localhost postgresql.master]$ set |grep PSQL
> PSQL_PAGER='pspg -X'
> PSQL_WATCH_PAGER='pspg -X --stream'
>
> Regards
>
> Pavel
>
>
Ummm... We are testing PSQL \watch  and you potentially have a
PSQL_WATCH_PAGER that is kicking in?
By chance does that attempt to read/process/understand the \watch ?
Also, if it is interfering with the stream, that would explain it.  The
perl library is trying to "control" psql.
If it ends up talking to you instead... All bets are off, imo.  I don't
know enough about PSQL_WATCH_PAGER.

Now I would be curious if you changed the test from
SELECT 1 \watch c=3 0.01

to
SELECT 1 \watch 0.01

because that should work.  Then I would test
SELECT \watch 0.01 c=3

If you are trying to parse the watch at all, that could break.  Then your
code might be trying to "complain",
and then that is screwing up the planned interaction (Just Guessing).

Kirk...

Re: psql tests hangs

[Kirk Wolak]

On Fri, May 12, 2023 at 1:46 AM Pavel Stehule 
wrote:

> pá 12. 5. 2023 v 6:50 odesílatel Kirk Wolak  napsal:
>
>> On Fri, May 12, 2023 at 12:14 AM Tom Lane  wrote:
>>
>>> Kirk Wolak  writes:
>>> > Did you try the print statement that Andrey asked Pavel to try?
>>> ...
>>
>>
> The strange thing is hanging. Broken tests depending on locale are usual.
> But I didn't remember hanging.
>
> Regards
>
> Pavel
>

So, if you do psql -c "..."
with both of those \watch  instructions, do either one hang? (I am now
guessing "no")

I know that perl is using a special library to "remote control psql" (like
a pseudo terminal, I guess).
[I had to abort some of the perl testing in Windows because that perl
library didn't work with my psql in Windows]

Next, can you detect which process is hanging? (is it perl, the library,
psql, ?).

I would be curious now about the details of your perl install, and your
perl libraries...

Re: psql tests hangs

[Kirk Wolak]

On Fri, May 12, 2023 at 12:14 AM Tom Lane  wrote:

> Kirk Wolak  writes:
> > Did you try the print statement that Andrey asked Pavel to try?
>
> Yeah, and I get exactly the results I expect:
>
> Your results MATCHED Pavels  (Hmm).  Piping ONE of those into psql should
fail, and the other one should work, right?

I know Pavel is Czech... So I have to Wonder...
Are both of you using the same Collation inside of PG? (Or did I miss that
the testing forces that setting?)

Kirk...

Re: psql tests hangs

[Kirk Wolak]

On Thu, May 11, 2023 at 8:08 PM Tom Lane  wrote:

> Kirk Wolak  writes:
> > Can you change the 0.01 to just 1 or 0?
> > I assume it will work then! (and better than a full removal)?
>
> IMO the point of that test is largely to exercise this locale-dependent
> behavior, so I'm very unwilling to dumb it down to that extent.
>
> Sorry, I meant simply as opposed to deleting the test to get it to pass.


> What seems to be happening is that the spawned psql process is making
> a different choice about what the LC_NUMERIC locale is than its parent
> perl process did.  That seems like it might be a bug in itself, since
> POSIX is pretty clear about how you're supposed to derive the locale
> from the relevant environment variables.  But maybe it's Perl's bug?
>
> regards, tom lane
>

Did you try the print statement that Andrey asked Pavel to try?
Because it gave 2 different results for Pavel.  And Pavel's system has the
problem, but yours does not.

cat test.pl
use locale;
my $result = sprintf('SELECT 1 \watch c=3 i=%g', 0.01);
print ">>$result<<\n";

and when Pavel ran it, he got:

[pavel@localhost psql]$ perl test.pl
>>SELECT 1 \watch c=3 i=0,01<<
[pavel@localhost psql]$ LANG=C perl test.pl
>>SELECT 1 \watch c=3 i=0.01<<

Now I am curious what you get?

Because yours works.  This should identify the difference.

Kirk...

Re: psql tests hangs

[Kirk Wolak]

On Wed, May 10, 2023 at 12:59 AM Pavel Stehule 
wrote:

> Hi
>
> When I remove this test, then all tests passed
>
> diff --git a/src/bin/psql/t/001_basic.pl b/src/bin/psql/t/001_basic.pl
> index 596746de17..631a1a7335 100644
> --- a/src/bin/psql/t/001_basic.pl
> +++ b/src/bin/psql/t/001_basic.pl
> @@ -353,11 +353,6 @@ psql_like(
>
>  # Check \watch
>  # Note: the interval value is parsed with locale-aware strtod()
> -psql_like(
> -   $node,
> -   sprintf('SELECT 1 \watch c=3 i=%g', 0.01),
> -   qr/1\n1\n1/,
> -   '\watch with 3 iterations');
>
>  # Check \watch errors
>  psql_fails_like(
>
> Can somebody repeat this testing of FC38?
>
> Regards
>
> Pavel
>
> Can you change the 0.01 to just 1 or 0?

I assume it will work then! (and better than a full removal)?

Re: v16 regression - wrong query results with LEFT JOINs + join removal

[Kirk Wolak]

On Thu, May 11, 2023 at 11:50 AM Robert Haas  wrote:

> On Thu, May 11, 2023 at 10:14 AM Robert Haas 
> wrote:
> > Ouch, so we've had a known queries-returning-wrong-answers bug for
> > more than two months. That's not great. I'll try to find time today to
> > check whether the patches on that thread resolve this issue.
>
> I tried out the v3 patches from that thread and they don't seem to
> make any difference for me in this test case. So either (1) it's a
> different issue or (2) those patches don't fully fix it or (3) I'm bad
> at testing things.
>
> --
> Robert Haas
> EDB: http://www.enterprisedb.com
>
> Forgive the noob question...  But does this trigger a regression test to
be created?
And who tracks/pushes that?

Kirk...

Re: Discussion: psql \et -> edit the trigger function

[Kirk Wolak]

On Wed, May 10, 2023 at 1:33 PM Tom Lane  wrote:

> I wrote:
> > Hmm, I wonder how useful that's really going to be, considering
> > that trigger names aren't unique across tables.  Wouldn't it
> > need to be more like "\et table-name trigger-name"?
>
> Different line of thought: \et seems awfully single-purpose.
> Perhaps we should think more of "\st table-name trigger-name"
> (show trigger), which perhaps could print something along the
> lines of
>
> CREATE TRIGGER after_ins_stmt_trig AFTER INSERT ON main_table
> FOR EACH STATEMENT EXECUTE FUNCTION trigger_func('after_ins_stmt');
>
> CREATE FUNCTION public.trigger_func()
>  RETURNS trigger
> ... the rest like \sf for the trigger function
>
> If you indeed want to edit the function, it's a quick copy-and-paste
> from here.  But if you just want to see the trigger definition,
> this is more wieldy than looking at the whole "\d table-name"
> output.  Also we have less of an overloading problem with the
> command name.
>

I agree that the argument for \et or \etf fails.  Simply on the one to many
issues.
And I agree that a more consistent approach is best.

Having just cleaned up 158 Triggers/Trigger Functions... Just having \eft
 work would be nice.

Which would solve my problem of quickly getting the tables triggers and
reviewing the code.

I like the idea of adding to the \s* options.  As in "show".
but the "t" is very common (table, trigger, type).  I think \st \str \sty
could work, but this is the first place where we would be doing this?

Honestly I think \st is "missing", especially to throw something in
dbfiddle or another tool.

And if we drop "trigger" from this, then \st and \sT  where T would be for
Types as elsewhere.

Now that feels more consistent?

So, currently thinking:
1) lets get \ef?  working
2) Discuss: \st \sT  for outputting Table and Type Creation DDL...

Something is telling me that #2 (\st) might be a can of worms, since it
seems so obviously "missing"?


>
> regards, tom lane
>

I appreciate the feedback!

Re: psql tests hangs

[Kirk Wolak]

On Thu, May 11, 2023 at 3:06 PM Pavel Stehule 
wrote:

>
>
> čt 11. 5. 2023 v 20:44 odesílatel Tom Lane  napsal:
>
>> Pavel Stehule  writes:
>> > When I remove this test, then all tests passed
>>
>> This works fine for me on Fedora 37:
>>
>
> I have Fedora 38
>
>>
>> $ cd src/bin/psql
>> $ LANG=cs_CZ.utf8 make installcheck
>> make -C ../../../src/backend generated-headers
>> make[1]: Vstupuje se do adresáře „/home/tgl/pgsql/src/backend“
>> ...
>> # +++ tap install-check in src/bin/psql +++
>> t/001_basic.pl ... ok
>> t/010_tab_completion.pl .. ok
>> t/020_cancel.pl .. ok
>> All tests successful.
>> Files=3, Tests=169,  6 wallclock secs ( 0.06 usr  0.02 sys +  2.64 cusr
>> 0.99 csys =  3.71 CPU)
>> Result: PASS
>>
>> I wonder if you have something inconsistent in your locale
>> configuration.  What do you see from
>>
>> $ env | grep '^L[CA]'
>>
>
>  [pavel@localhost psql]$ env | grep '^L[CA]'
> LANG=cs_CZ.UTF-8
>
> Regards
>
> Pavel
>

Stranger things, but is LANG case sensitive, or formatted differently?

tom> $ LANG=cs_CZ.utf8 make installcheck
 you> LANG=cs_CZ.*UTF-8*



>
>
>> regards, tom lane
>>
>

Re: Discussion: psql \et -> edit the trigger function

[Kirk Wolak]

On Wed, May 10, 2023 at 12:20 PM Pavel Stehule 
wrote:

> Hi
>
> st 10. 5. 2023 v 17:33 odesílatel Kirk Wolak  napsal:
>
>> We already have
>> \ef
>> \ev
>>
>> The use case here is simply that it saves me from:
>> \d 
>> [scroll through all the fields]
>> [often scroll right]
>> select function name
>> \ef [paste function name]
>>
>> and tab completion is much narrower
>>
>> When doing conversions and reviews all of this stuff has to be reviewed.
>> Oftentimes, renamed, touched.
>>
>> I am 100% willing to write the code, docs, etc. but would appreciate
>> feedback.
>>
>
> \et can be little bit confusing, because looks like editing trigger, not
> trigger function
>
> what \eft triggername
>
> ?
>
> Pavel, I am "torn" because of my OCD, I would expect
\eft 
to list functions that RETURN TRIGGER as opposed to the level of
indirection I was aiming for.

where
\et 
  Would specifically let me complete the Trigger_Name, but find the function

It makes me wonder, now if:
\etf

Is better for this (edit trigger function... given the trigger name).
And as another poster suggested.  As we do the AUTOCOMPLETE for that, we
could address it for:
\ef?

because:
\eft 
is valuable as well, and deserves to work just like all \ef? items

It seems like a logical way to break it down.

> regards
>
> Pavel
>
>
>
>>
>> Kirk...
>>
>

Discussion: psql \et -> edit the trigger function

[Kirk Wolak]

We already have
\ef
\ev

The use case here is simply that it saves me from:
\d 
[scroll through all the fields]
[often scroll right]
select function name
\ef [paste function name]

and tab completion is much narrower

When doing conversions and reviews all of this stuff has to be reviewed.
Oftentimes, renamed, touched.

I am 100% willing to write the code, docs, etc. but would appreciate
feedback.

Kirk...

Re: enhancing plpgsql debug api - hooks on statements errors and function errors

[Kirk Wolak]

On Tue, Apr 25, 2023 at 11:33 AM Pavel Stehule 
wrote:

> Hi
> út 25. 4. 2023 v 10:27 odesílatel Pavel Stehule 
> napsal:
>
>> Hi
>>
>> When I implemented profiler and coverage check to plpgsql_check I had to
>> write a lot of hard maintaining code related to corect finishing some
>> operations (counter incrementing) usually executed by stmt_end and func_end
>> hooks. It is based on the fmgr hook and its own statement call stack. Can
>> be nice if I can throw this code and use some nice buildin API.
>>
>> Can we enhance dbg API with two hooks stmt_end_err func_end_err ?
>>
>> These hooks can be called from exception handlers before re raising.
>>
>> Or we can define new hooks like executor hooks - stmt_exec and func_exec.
>> In custom hooks the exception can be catched.
>>
>> What do you think about this proposal?
>>
>> +1.  I believe I bumped into a few of these use cases with plpgsql_check
(special handling for security definer and exception handling).
  My cursory review of the patch file is that despite the movement of the
code, it feels pretty straight forward.

The 7% overhead appears in a "tight loop", so it's probably really
overstated.  I will see if I can apply this and do a more realistic test.
[I have a procedure that takes ~2hrs to process a lot of data, I would be
curious to see this impact and report back]


> I did quick and ugly benchmark on worst case
>
> CREATE OR REPLACE FUNCTION public.speedtest(i integer)
>  RETURNS void
>  LANGUAGE plpgsql
>  IMMUTABLE
> AS $function$
> declare c int = 0;
> begin
>   while c < i
>   loop
> c := c + 1;
>   end loop;
>   raise notice '%', c;
> end;
> $function$
>
> and is possible to write some code (see ugly patch) without any
> performance impacts if the hooks are not used. When hooks are active, then
> there is 7% performance lost. It is not nice - but this is the worst case.
> The impact on real code should be significantly lower
>
> Regards
>
> Pavel
>
>

Re: Commitfest 2023-03 starting tomorrow!

[Kirk Wolak]

On Fri, Apr 7, 2023 at 6:29 PM Tom Lane  wrote:

> Kirk Wolak  writes:
> > The %T added to the PSQL Prompt is about 5 lines of code.  Reviewed and
> > Ready to commit.
> > That could knock one more off really quickly :-)
>
> I'm still objecting to it, for the same reason as before.
>
> regards, tom lane
>

Tom,
  I got no  response to my point that the backquote solution is cumbersome
because I have to use* psql in both windows*
*and in linux environments* (realizing I am the odd duck in this group).
But my fall back was a common script file.  Then I shared my
psqlrc file with a co-worker, and they ran into the missing script file.
[ie, the same command does not work in both systems].

  I won't argue beyond this point, I'd just like to hear that you
considered this final point...
and I can move on.

Thanks, Kirk

Re: Commitfest 2023-03 starting tomorrow!

[Kirk Wolak]

On Fri, Apr 7, 2023 at 10:21 AM Greg Stark  wrote:

> As announced on this list feature freeze is at 00:00 April 8 AoE.
> That's less than 24 hours away. If you need to set your watches to AoE
> timezone it's currently:
>
> $ TZ=AOE+12 date
> Fri 07 Apr 2023 02:05:50 AM AOE
>
> As we stand we have:
>
> Status summary:
>   Needs review: 82
>   Waiting on Author:16
>   Ready for Committer:  27
>   Committed:   115
>   Moved to next CF: 38
>   Returned with Feedback:   10
>   Rejected:  9
>   Withdrawn:22
> Total: 319.
>
> In less than 24h most of the remaining patches will get rolled forward
> to the next CF. The 16 that are Waiting on Author might be RwF
> perhaps. The only exceptions would be non-features like Bug Fixes and
> cleanup patches that have been intentionally held until the end --
> those become Open Issues for the release.
>
> So if we move forward all the remaining patches (so these numbers are
> high by about half a dozen) the *next* CF would look like:
>
> Commitfest 2023-07:Now  April 8
>   Needs review: 46. 128
>   Waiting on Author:17.  33
>   Ready for Committer:   3.  30
> Total:  66  191
>
> I suppose that's better than the 319 we came into this CF with but
> there's 3 months to accumulate more unreviewed patches...
>
> I had hoped to find lots of patches that I could bring the hammer down
> on and say there's just no interest in or there's no author still
> maintaining. But that wasn't the case. Nearly all the patches still
> had actively interested authors and looked like they were legitimately
> interesting and worthwhile features that people just haven't had the
> time to review or commit.
>
>
> --
> greg
>
> The %T added to the PSQL Prompt is about 5 lines of code.  Reviewed and
Ready to commit.
That could knock one more off really quickly :-)

Excellent work to everyone.

Thanks, Kirk

Re: Schema variables - new implementation for Postgres 15

[Kirk Wolak]

On Thu, Mar 30, 2023 at 4:06 AM Pavel Stehule 
wrote:

> Hi
>
> ne 26. 3. 2023 v 19:44 odesílatel Dmitry Dolgov <9erthali...@gmail.com>
> napsal:
>
>> > On Fri, Mar 24, 2023 at 08:04:08AM +0100, Pavel Stehule wrote:
>> > čt 23. 3. 2023 v 19:54 odesílatel Pavel Stehule <
>> pavel.steh...@gmail.com>
>> > napsal:
>> >
>> > > čt 23. 3. 2023 v 16:33 odesílatel Peter Eisentraut <
>> > > peter.eisentr...@enterprisedb.com> napsal:
>> > >
>> > >> The other issue is that by its nature this patch adds a lot of code
>> in a
>> > >> lot of places.  Large patches are more likely to be successful if
>> they
>> ...
>> I agree, the patch scale is a bit overwhelming. It's worth noting that
>> due to the nature of this change certain heavy lifting has to be done in
>> any case, plus I've got an impression that some part of the patch are
>> quite solid (although I haven't reviewed everything, did anyone achieve
>> that milestone?). But still, it would be of great help to simplify the
>> current implementation, and I'm afraid the only way of doing this is to
>> make trades-off about functionality vs change size & complexity.
>>
>
> There is not too much space for reduction - more - sometimes there is code
> reuse between features.
>
> I can reduce temporary session variables, but the same AtSubXact routines
> are used by memory purging routines, and if only if  you drop all dependent
> features, then you can get some interesting number of reduced lines. I can
> imagine very reduced feature set like
>
> 1) no temporary variables, no reset at transaction end
> 2) without default expressions - default is null
> 3) direct memory cleaning on drop (without possibility of saved value
> after reverted drop) or cleaning at session end always
>
> Note - @1 and @3 shares code
>
> Please don't remove #2.  With Default Values, I was eyeballing these as
pseudo constants.  I find I have a DRY (Don't Repeat Yourself) issue in our
current code base (PLPGSQL) because of the lack of shared constants
throughout the application layer.  We literally created a CONST schema with
SQL functions that return a set value.  It's kludgy, but clear enough.  (We
have approximately 50 of these).

Regards, Kirk

Re: Schema variables - new implementation for Postgres 15

[Kirk Wolak]

On Wed, Apr 5, 2023 at 1:58 PM Pavel Stehule 
wrote:

>
>
> st 5. 4. 2023 v 19:20 odesílatel Greg Stark  napsal:
>
>> On Sun, 26 Mar 2023 at 07:34, Julien Rouhaud  wrote:
>> >
>> > This feature can significantly increase log size, so it's disabled by
>> default.
>> > For testing or development environments it's recommended to enable it
>> if you
>> > use session variables.
>>
>> I think it's generally not practical to have warnings for valid DML.
>> Effectively warnings in DML are errors since they make the syntax just
>> unusable. I suppose it's feasible to have it as a debugging option
>> that defaults to off but I'm not sure it's really useful.
>>
>
> It is a tool that should help with collision detection.  Without it, it
> can be pretty hard to detect it. It is similar to plpgsql's extra warnings.
>
>
>> I suppose it raises the question of whether session variables should
>> be in pg_class and be in the same namespace as tables so that
>> collisions are impossible. I haven't looked at the code to see if
>> that's feasible or reasonable. But this feels a bit like what happened
>> with sequences where they used to be a wholly special thing and later
>> we realized everything was simpler if they were just a kind of
>> relation.
>>
>
> The first patch did it. But at the end, it doesn't reduce conflicts,
> because usually the conflicts are between variables and table's attributes
> (columns).
>
> example
>
> create variable a as int;
> create table foo(a int);
>
> select a from foo; -- the "a" is ambiguous, variable "a" is shadowed
>
> This is a basic case, and the unique names don't help. The variables are
> more aggressive in namespace than tables, because they don't require be in
> FROM clause. This is the reason why we specify so variables are always
> shadowed. Only this behaviour is safe and robust. I cannot break any query
> (that doesn't use variables) by creating any variable. On second hand, an
> experience from Oracle's PL/SQL or from old PLpgSQL is, so unwanted
> shadowing can be hard to investigate (without some tools).
>
> PL/pgSQL doesn't allow conflict between PL/pgSQL variables, and SQL (now),
> and I think so it is best. But the scope of PLpgSQL variables is relatively
> small, so very strict behaviour is acceptable.
>
> The session variables are some between tables and attributes. The catalog
> pg_class can be enhanced about columns for variables, but it does a lot
> now, so I think it is not practical.
>
>>
>> I agree about shadowing schema variables.  But is there no way to fix
that so that you can dereference the variable?
[Does an Alias work inside a procedure against a schema var?]
Does adding a schema prefix resolve it  properly, so your example, I could
do:
SELECT schema_var.a AS var_a, a as COL_A from t;

Again, I like the default that it is hidden, but I can envision needing
both?

Regards, Kirk

Re: [PATCH] Add function to_oct

[Kirk Wolak]

On Thu, Feb 23, 2023 at 6:32 AM Peter Eisentraut <
peter.eisentr...@enterprisedb.com> wrote:

> On 20.12.22 23:08, Eric Radman wrote:
> > This patch is a new function based on the implementation of to_hex(int).
> >
> > Since support for octal integer literals was added, to_oct(int) allows
> > octal values to be easily stored and returned in query results.
> >
> >to_oct(0o755) = '755'
> >
> > This is probably most useful for storing file system permissions.
>
> Note this subsequent discussion about the to_hex function:
>
> https://www.postgresql.org/message-id/flat/CAEZATCVbkL1ynqpsKiTDpch34%3DSCr5nnau%3DnfNmiy2nM3SJHtw%40mail.gmail.com
>
> Also, I think there is no "to binary" function, so perhaps if we're
> going down this road one way or the other, we should probably complete
> the set.
>
> The code reads clearly.  It works as expected (being an old PDP-11
guy!)... And the docs make sense and build as well.
Nothing larger than an int gets in.  I was "missing" the bigint version,
but read through ALL of the messages to see (and agree)
That that's okay.
Marked Ready for Committer.

Thanks, Kirk

Re: psql \watch 2nd argument: iteration count

[Kirk Wolak]

On Fri, Mar 24, 2023 at 10:32 PM Andrey Borodin 
wrote:

> On Thu, Mar 23, 2023 at 10:15 PM Yugo NAGATA  wrote:
> >
> > Here is my review on the v9 patch.
> >
> > +   /* we do not prevent numerous names iterations like
> i=1 i=1 i=1 */
> > +   have_sleep = true;
> >
> > Why this is allowed here? I am not sure there is any reason to allow to
> specify
> > multiple "interval" options. (I would apologize it if I missed past
> discussion.)
> I do not know, it just seems normal to me. I've fixed this.
>
> >  postgres=# select 1 \watch interval=3 4
> >  \watch: incorrect interval value '4'
> >
> > I think it is ok, but this error message seems not user-friendly because,
> > in the above example, interval values itself is correct, but it seems
> just
> > a syntax error. I wonder it is better to use "watch interval must be
> specified
> > only once" or such here, as the past patch.
> Done.
>
> >
> > +
> > +If number of iterations is specified - query will be executed
> only
> > +given number of times.
> > +
> >
> > Is it common to use "-" here?  I think using comma like
> > "If number of iterations is specified, "
> > is natural.
> Done.
>
> Thank for the review!
>
>
> Best regards, Andrey Borodin.
>

Okay, I tested this. It handles bad param names, values correctly.  Nice
Feature, especially if you leave a 1hr task running and you need to step
away...
Built/Reviewed the Docs.  They are correct.
Reviewed \? command.  It has the parameters updated, shown as optional

Marked as Ready for Committer.

Re: SELECT INTO without columns or star

[Kirk Wolak]

On Fri, Mar 31, 2023 at 11:26 AM Tom Lane  wrote:

> "David G. Johnston"  writes:
> > On Fri, Mar 31, 2023 at 8:10 AM Zhang Mingli 
> wrote:
> >> When I exec a sql SELECT INTO without columns or * by mistake, it
> succeeds:
>
> > Yes, a table may have zero columns by design.
>
> Yup, we've allowed that for some time now; see the compatibility comments
> at the bottom of the SELECT man page.
>
> psql's display of zero-column results is a bit weird, which maybe
> somebody should fix sometime:
>
> regression=# select from generate_series(1,4);
> --
> (4 rows)
>
> I'd expect four blank lines there.  Expanded format is even less sane:
>
> regression=# \x
> Expanded display is on.
> regression=# select from generate_series(1,4);
> (4 rows)
>
> ISTM that should produce
>
> [ RECORD 1 ]
> [ RECORD 2 ]
> [ RECORD 3 ]
> [ RECORD 4 ]
>
> and no "(4 rows)" footer, because \x mode doesn't normally print that.
>
> This is all just cosmetic of course, but it's still confusing.
>
> regards, tom lane
>

Tom,
  I wouldn't mind working on a patch to fix this... (Especially if it helps
the %T get into PSQL).
I find this output confusing as well.

  Should I start a new email thread: Proposal: Fix psql output when
selecting no columns
And get the discussion moving.  I'd like to get a clear answer on what to
output.   But I have
become more comfortable with PSQL due to messing with readline for windows,
and 2-3 other patches
I've been working on.

Thanks, Kirk

Re: zstd compression for pg_dump

[Kirk Wolak]

On Tue, Mar 28, 2023 at 12:23 PM Tomas Vondra 
wrote:

> On 3/27/23 19:28, Justin Pryzby wrote:
> > On Fri, Mar 17, 2023 at 03:43:31AM +0100, Tomas Vondra wrote:
> >> On 3/16/23 05:50, Justin Pryzby wrote:
> >>> On Fri, Mar 10, 2023 at 12:48:13PM -0800, Jacob Champion wrote:
>  On Wed, Mar 8, 2023 at 10:59 AM Jacob Champion <
> jchamp...@timescale.com> wrote:
> > I did some smoke testing against zstd's GitHub release on Windows. To
> ...
> OK. I don't have access to a Windows machine so I can't test that. Is it
> possible to disable the zstd threading, until we figure this out?
>
> Thomas since I appear to be one of the few windows users (I use both), can
I help?
I can test pg_dump... for you, easy to do.  I do about 5-10 pg_dumps a day
on windows while developing.

Also, I have an AWS instance I created to build PG/Win with readline back
in November.
I could give you access to that...  (you are not the only person who has
made this statement here).
I've made such instances available for other Open Source developers, to
support them.

 Obvi I would share connection credentials privately.

Regards, Kirk

Re: possible proposal plpgsql GET DIAGNOSTICS oid = PG_ROUTINE_OID

[Kirk Wolak]

On Sun, Mar 26, 2023 at 5:37 PM Kirk Wolak  wrote:

> On Wed, Feb 8, 2023 at 10:56 AM Kirk Wolak  wrote:
>
>> On Wed, Feb 8, 2023 at 3:08 AM Pavel Stehule 
>> wrote:
>>
>>> hi
>>>
>>> st 8. 2. 2023 v 7:33 odesílatel Julien Rouhaud 
>>> napsal:
>>>
>>>> On Tue, Feb 07, 2023 at 08:48:22PM +0100, Pavel Stehule wrote:
>>>> >
>>>> > GET DIAGNOSTICS _oid = PG_ROUTINE_OID;
>>>> > RAISE NOTICE '... % ... %', _oid, _oid::regproc::text;
>>>> >
>>>> > Do you think it can be useful feature?
>>>>
>>>> +1, it would have been quite handy in a few of my projects.
>>>>
>>>
>>> it can looks like that
>>>
>>> create or replace function foo(a int)
>>> returns int as $$
>>> declare s text; n text; o oid;
>>> begin
>>>   get diagnostics s = pg_current_routine_signature,
>>>   n = pg_current_routine_name,
>>>   o = pg_current_routine_oid;
>>>   raise notice 'sign:%,  name:%,  oid:%', s, n, o;
>>>   return a;
>>> end;
>>> $$ language plpgsql;
>>> CREATE FUNCTION
>>> (2023-02-08 09:04:03) postgres=# select foo(10);
>>> NOTICE:  sign:foo(integer),  name:foo,  oid:16392
>>> ┌─┐
>>> │ foo │
>>> ╞═╡
>>> │  10 │
>>> └─┘
>>> (1 row)
>>>
>>> The name - pg_routine_oid can be confusing, because there is not clean
>>> if it is oid of currently executed routine or routine from top of exception
>>>
>>> Regards
>>>
>>> Pavel
>>>
>>
>> I agree that the name changed to pg_current_routine_...  makes the most
>> sense, great call...
>>
>> +1
>>
>
> Okay, I reviewed this.  I tested it (allocating too small of
> varchar's for values, various "signature types"),
> and also a performance test... Wow, on my VM, 10,000 Calls in a loop was
> 2-4ms...
>
> The names are clear.  Again, I tested with various options, and including
> ROW_COUNT, or not.
>
> This functions PERFECTLY  Except there are no documentation changes.
> Because of that, I set it to Waiting on Author.
> Which might be unfair, because I could take a stab at doing the
> documentation (but docs are not compiling on my setup yet).
>
> The documentation changes are simple enough.
> If I can get the docs compiled on my rig, I will see if I can make the
> changes, and post an updated patch,
> that contains both...
>
> But I don't want to be stepping on toes, or having it look like I am
> taking credit.
>
> Regards - Kirk
>

Okay, I have modified the documentation and made sure it compiles.  They
were simple enough changes.
I am attaching this updated patch.

I have marked the item Ready for Commiter...

Thanks for your patience.  I now have a workable hacking environment!

Regards - Kirk
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 7c8a49fe43..19dfe529cf 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -1639,6 +1639,21 @@ GET DIAGNOSTICS integer_var = ROW_COUNT;
  line(s) of text describing the current call stack
   (see )
 
+   
+ PG_CURRENT_ROUTINE_SIGNATURE
+ text
+ text describing the current routine with paramater 
types
+
+
+ PG_CURRENT_ROUTINE_NAME
+ text
+ text name of the function without parenthesis
+
+
+ PG_CURRENT_ROUTINE_OID
+ oid
+ oid of the function currently running
+

   
  
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index b0a2cac227..bb2f3ff828 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -2475,6 +2475,28 @@ exec_stmt_getdiag(PLpgSQL_execstate *estate, 
PLpgSQL_stmt_getdiag *stmt)
}
break;
 
+   case PLPGSQL_GETDIAG_CURRENT_ROUTINE_NAME:
+   {
+   char   *funcname;
+
+   funcname = 
get_func_name(estate->func->fn_oid);
+   exec_assign_c_string(estate, var, 
funcname);
+   if (funcname)
+   pfree(funcname);
+   }
+   break;
+
+   case PLPGSQL_GETDIAG_CURRENT_ROUTINE_OID:
+   exec_assign_value(estate, var,
+   

Re: Documentation Not Compiling (http://docbook... not https:.//...)

[Kirk Wolak]

On Sun, Mar 26, 2023 at 9:12 PM Kirk Wolak  wrote:

> Andres,
>   Apologies to pick on you directly.
>   But it appears that sites are refusing HTTP requests,
> and it's affecting compilation of docs in a new configuration.
>
>   I was surprised to see NON-HTTPS references in 2023, tbh...
> I cannot even curl these references.
>
>   Maybe I am missing a simple flag...
>
>   Or should I offer to search/replace to fix everything to HTTPS,
> and submit a patch?
>
> Regards, Kirk
>

Okay, for future reference I had to install a few things (fop, dbtoepub,
docbook-xsl)

Not sure why the original ./configure did not bring those in...

Regards, Kirk

Documentation Not Compiling (http://docbook... not https:.//...)

[Kirk Wolak]

Andres,
  Apologies to pick on you directly.
  But it appears that sites are refusing HTTP requests,
and it's affecting compilation of docs in a new configuration.

  I was surprised to see NON-HTTPS references in 2023, tbh...
I cannot even curl these references.

  Maybe I am missing a simple flag...

  Or should I offer to search/replace to fix everything to HTTPS,
and submit a patch?

Regards, Kirk

Re: possible proposal plpgsql GET DIAGNOSTICS oid = PG_ROUTINE_OID

[Kirk Wolak]

On Wed, Feb 8, 2023 at 10:56 AM Kirk Wolak  wrote:

> On Wed, Feb 8, 2023 at 3:08 AM Pavel Stehule 
> wrote:
>
>> hi
>>
>> st 8. 2. 2023 v 7:33 odesílatel Julien Rouhaud 
>> napsal:
>>
>>> On Tue, Feb 07, 2023 at 08:48:22PM +0100, Pavel Stehule wrote:
>>> >
>>> > GET DIAGNOSTICS _oid = PG_ROUTINE_OID;
>>> > RAISE NOTICE '... % ... %', _oid, _oid::regproc::text;
>>> >
>>> > Do you think it can be useful feature?
>>>
>>> +1, it would have been quite handy in a few of my projects.
>>>
>>
>> it can looks like that
>>
>> create or replace function foo(a int)
>> returns int as $$
>> declare s text; n text; o oid;
>> begin
>>   get diagnostics s = pg_current_routine_signature,
>>   n = pg_current_routine_name,
>>   o = pg_current_routine_oid;
>>   raise notice 'sign:%,  name:%,  oid:%', s, n, o;
>>   return a;
>> end;
>> $$ language plpgsql;
>> CREATE FUNCTION
>> (2023-02-08 09:04:03) postgres=# select foo(10);
>> NOTICE:  sign:foo(integer),  name:foo,  oid:16392
>> ┌─┐
>> │ foo │
>> ╞═╡
>> │  10 │
>> └─┘
>> (1 row)
>>
>> The name - pg_routine_oid can be confusing, because there is not clean if
>> it is oid of currently executed routine or routine from top of exception
>>
>> Regards
>>
>> Pavel
>>
>
> I agree that the name changed to pg_current_routine_...  makes the most
> sense, great call...
>
> +1
>

Okay, I reviewed this.  I tested it (allocating too small of
varchar's for values, various "signature types"),
and also a performance test... Wow, on my VM, 10,000 Calls in a loop was
2-4ms...

The names are clear.  Again, I tested with various options, and including
ROW_COUNT, or not.

This functions PERFECTLY  Except there are no documentation changes.
Because of that, I set it to Waiting on Author.
Which might be unfair, because I could take a stab at doing the
documentation (but docs are not compiling on my setup yet).

The documentation changes are simple enough.
If I can get the docs compiled on my rig, I will see if I can make the
changes, and post an updated patch,
that contains both...

But I don't want to be stepping on toes, or having it look like I am taking
credit.

Regards - Kirk

Re: Operation log for major operations

[Kirk Wolak]

On Thu, Mar 2, 2023 at 4:09 PM Dmitry Koval  wrote:

> I'll try to expand my explanation.
> I fully understand and accept the arguments about "limited sense to go
> into the control file" and "about recording *anything* in the control
> file". This is totally correct for vanilla.
> But vendors have forks of PostgreSQL with custom features and extensions.
> Sometimes (especially at the first releases) these custom components
> have bugs which can causes rare problems in data.
> These problems can migrate with using pg_upgrade and "lazy" upgrade of
> pages to higher versions of PostgreSQL fork.
>
> So in error cases "recording crash information" etc. is not the only
> important information.
> Very important is history of this database (pg_upgrades, promotions,
> pg_resets, pg_rewinds etc.).
> Often these "history" allows you to determine from which version of the
> PostgreSQL fork the error came from and what causes of errors we can
> discard immediately.
>
> This "history" is the information that our technical support wants (and
> reason of this patch), but this information is not needed for vanilla...
>
> Another important condition is that the user should not have easy ways
> to delete information about "history" (about reason to use pg_control
> file as "history" storage, but write into it from position 4kB, 8kB,...).
>
> --
> With best regards,
> Dmitry Koval
>
> Postgres Professional: http://postgrespro.com
>
> Dmitry, this is a great explanation.  Thinking outside the box, it feels
like:
We need some kind of semaphore flag that tells us something awkward
happened.
When it happened, and a little bit of extra information.

You also make the point that if such things have happened, it would
probably be a good idea to NOT
allow pg_upgrade to run.  It might even be a reason to constantly bother
someone until the issue is repaired.

To that point, this feels like a "postgresql_panic.log" file (within the
postgresql files?)...  Something that would prevent pg_upgrade,
etc.  That everyone recognizes is serious.  Especially 3rd party vendors.

I see the need for such a thing.  I have to agree with others about
questioning the proper place to write this.

Are there other places that make sense, that you could use, especially if
knowing it exists means there was a serious issue?

Kirk

Re: proposal: psql: show current user in prompt

[Kirk Wolak]

On Sat, Feb 4, 2023 at 3:33 PM Pavel Stehule 
wrote:

> Hi
>
> pá 3. 2. 2023 v 21:43 odesílatel Pavel Stehule 
> napsal:
>
>>
>>
>> pá 3. 2. 2023 v 21:21 odesílatel Tom Lane  napsal:
>>
>>> Pavel Stehule  writes:
>>> > Both patches are very simple - and they use almost already prepared
>>> > infrastructure.
>>>
>>> It's not simple at all to make the psql feature depend on marking
>>> "role" as GUC_REPORT when it never has been before.  That will
>>> cause the feature to misbehave when using older servers.  I'm
>>> even less impressed by having it fall back on PQuser(), which
>>> would be misleading at exactly the times when it matters.
>>>
>>
>> It is a good note. This can be disabled for older servers, and maybe it
>> can  introduce its own GUC (and again - it can be disallowed for older
>> servers).
>>
>
> Here is another version. For older servers it shows the string ERR0A000.
> That is  ERR code of "feature is not supported"
>
>
>> My goal at this moment is to get some prototype. We can talk if this
>> feature request is valid or not, and we can talk about implementation.
>>
>> There is another possibility to directly execute "select current_user()"
>> instead of looking at status parameters inside prompt processing. It can
>> work too.
>>
>
> I tested using the query SELECT CURRENT_USER, but I don't think it is
> usable now, because it doesn't work in the broken transaction.
>
> Regards
>
> Pavel
>
>
>
>>
>> Regards
>>
>> Pavel
>>
>>
>>
>>
>>
>>> regards, tom lane
>>>
>>
I've tested this w/regards to psql.  Latest commit.
It works as described.  'none' is displayed for the default role. (SET ROLE
DEFAULT), otherwise the specific ROLE is displayed.

I tried this patch on 15.2, but guc_files.c does not exist in that version,
so it did not install.
I also tried applying the %T patch, but since they touch the same file, it
would not install with it, without rebasing, repatching.

The Docs are updated, and it's a relatively contained patch.

Changed status to Ready for Committer. (100% Guessing here...)

Kirk

Re: Proposal: %T Prompt parameter for psql for current time (like Oracle has)

[Kirk Wolak]

On Thu, Mar 2, 2023 at 9:56 AM Laurenz Albe 
wrote:

> On Wed, 2023-03-01 at 11:13 -0500, Kirk Wolak wrote:
> > Thanks, corrected, and confirmed Unix line endings.
>
> The patch builds fine and works as intended.
>
> I leave it to the committers to decide whether the patch is worth the
> effort or not, given that you can get a similar effect with %`date`.
> It adds some value by being simpler and uniform across all platforms.
>
> I'll mark the patch as "ready for committer".
>
> Yours,
> Laurenz Albe
>

Thanks Laurenz.

To be clear, I use windows AND linux, and I share my file between them.

in linux:  `date +%H:%M:%S`   is used
in windows: `ECHO %time%`

so, I wrote a ts.cmd and ts.sh  so I could share one prompt:   `ts`
but now every time I connect a new account to this file, I have to go
find/copy my ts file.
Same when I share it with other developers.

This was the pain that started the quest.
Thanks to everyone for their support!

Re: Proposal: %T Prompt parameter for psql for current time (like Oracle has)

[Kirk Wolak]

On Wed, Mar 1, 2023 at 11:55 AM Jim Jones  wrote:

> On 01.03.23 17:13, Kirk Wolak wrote:
>
> Thanks, corrected, and confirmed Unix line endings.
> FWIW, the simplest way to test it is with this command (I usually get it
> wrong on the first guess)
>
> \set PROMPT1 %T ' ' :PROMPT1
>
> Kirk
>
> Nice. The patch applies clean and the cfbots seem much happier now - all
> passed.
>
> 17:23:19 postgres=# SELECT now();
>   now
> ---
>  2023-03-01 17:23:19.807339+01
> (1 row)
>
> The docs render also just fine. I'm now wondering if HH24:MI:SS should be
> formatted with, e.g. using 
>
> "The current time on the client in HH24:MI:SS format."
>
> But that I'll leave to the docs experts to judge :)
>
> Best, Jim
>
Thanks Jim.

I hope one of the Docs experts chime in.  It's easy enough to fix.  Just
not sure if it's required.
What a great learning experience!

Re: Proposal: %T Prompt parameter for psql for current time (like Oracle has)

[Kirk Wolak]

On Wed, Mar 1, 2023 at 4:41 AM Jim Jones  wrote:

> On 01.03.23 01:59, Kirk Wolak wrote:
> > I cannot get the last email to show up for the commitfest.
> > This is version 2 of the original patch. [1]
> > Thanks Jim!
> >
> > [1]
> https://postgr.es/m/CACLU5mSRwHr_8z%3DenMj-nXF1tmC7%2BJn5heZQNiKuLyxYUtL2fg%40mail.gmail.com
> >
> > Regards Kirk.
>
> The patch didn't pass the SanityCheck:
>
> https://cirrus-ci.com/task/5445242183221248?logs=build#L1337
>
> missing a header perhaps?
>
> #include "time.h"
>
> Best, Jim
>

Thanks, corrected, and confirmed Unix line endings.
FWIW, the simplest way to test it is with this command (I usually get it
wrong on the first guess)

\set PROMPT1 %T ' ' :PROMPT1

Kirk
From bfafeaec64d01a404fe36f26ec4355607776e66b Mon Sep 17 00:00:00 2001
From: Kirk Wolak 
Date: Wed, 1 Mar 2023 16:02:10 +
Subject: [PATCH] [PATCH v3] Time option added to psql prompt

This adds a useful time option to the prompt: %T. Which does not
require a wasteful backquoted shell command which is also not
compatible between operating systems.
The format is simply HH24:MI:SS no other options available by design!

Author: Kirk Wolak 
Reviewed-By: Andrey Borodin 
Reviewed-By: Nikolay Samokhvalov 
Thread: 
https://postgr.es/m/CACLU5mSRwHr_8z%3DenMj-nXF1tmC7%2BJn5heZQNiKuLyxYUtL2fg%40mail.gmail.com
---
 doc/src/sgml/ref/psql-ref.sgml |  9 +
 src/bin/psql/prompt.c  | 11 ++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index dc6528dc11d..04ab9eeb8c0 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4575,6 +4575,15 @@ testdb= INSERT INTO my_table VALUES 
(:'content');
 
   
 
+  
+%T
+
+ 
+  The current time on the client in HH24:MI:SS format.
+ 
+
+  
+
   
 %x
 
diff --git a/src/bin/psql/prompt.c b/src/bin/psql/prompt.c
index 969cd9908e5..24dffcd461c 100644
--- a/src/bin/psql/prompt.c
+++ b/src/bin/psql/prompt.c
@@ -18,6 +18,7 @@
 #include "libpq/pqcomm.h"
 #include "prompt.h"
 #include "settings.h"
+#include "time.h"
 
 /*--
  * get_prompt
@@ -41,6 +42,7 @@
  * or a ! if session is not connected to a database;
  * in prompt2 -, *, ', or ";
  * in prompt3 nothing
+ * %T - time in HH24:MI:SS format
  * %x - transaction status: empty, *, !, ? (unknown or no connection)
  * %l - The line number inside the current statement, starting from 1.
  * %? - the error code of the last query (not yet implemented)
@@ -223,7 +225,14 @@ get_prompt(promptStatus_t status, ConditionalStack cstack)
break;
}
break;
-
+   /* output HH24:MI:SS */
+   case 'T':
+   {
+   time_t current_time = 
time(NULL);
+   struct tm *tm_info = 
localtime(_time);
+   sprintf(buf, "%02d:%02d:%02d", 
tm_info->tm_hour, tm_info->tm_min, tm_info->tm_sec);
+   }
+   break;
case 'x':
if (!pset.db)
buf[0] = '?';
-- 
GitLab

Proposal: %T Prompt parameter for psql for current time (like Oracle has)

[Kirk Wolak]

I cannot get the last email to show up for the commitfest.
This is version 2 of the original patch. [1]
Thanks Jim!

[1]
https://postgr.es/m/CACLU5mSRwHr_8z%3DenMj-nXF1tmC7%2BJn5heZQNiKuLyxYUtL2fg%40mail.gmail.com

Regards Kirk.
From b9db157177bbdeeeb6d35c3623ca9355141419d7 Mon Sep 17 00:00:00 2001
From: Jim Jones 
Date: Wed, 1 Mar 2023 00:07:55 +0100
Subject: [PATCH v2] Time option added to psql prompt

This adds a useful time option to the prompt: %T. Which does not
require a wasteful backquoted shell command which is also not
compatible between operating systems.
The format is simply HH24:MI:SS no other options available by design!

Author: Kirk Wolak 
Reviewed-By: Andrey Borodin 
Reviewed-By: Nikolay Samokhvalov 
Thread: 
https://postgr.es/m/CACLU5mSRwHr_8z%3DenMj-nXF1tmC7%2BJn5heZQNiKuLyxYUtL2fg%40mail.gmail.com
---
 doc/src/sgml/ref/psql-ref.sgml |  9 +
 src/bin/psql/prompt.c  | 10 +-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index dc6528dc11..04ab9eeb8c 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4575,6 +4575,15 @@ testdb= INSERT INTO my_table VALUES 
(:'content');
 
   
 
+  
+%T
+
+ 
+  The current time on the client in HH24:MI:SS format.
+ 
+
+  
+
   
 %x
 
diff --git a/src/bin/psql/prompt.c b/src/bin/psql/prompt.c
index 969cd9908e..0c0c725df5 100644
--- a/src/bin/psql/prompt.c
+++ b/src/bin/psql/prompt.c
@@ -41,6 +41,7 @@
  * or a ! if session is not connected to a database;
  * in prompt2 -, *, ', or ";
  * in prompt3 nothing
+ * %T - time in HH24:MI:SS format
  * %x - transaction status: empty, *, !, ? (unknown or no connection)
  * %l - The line number inside the current statement, starting from 1.
  * %? - the error code of the last query (not yet implemented)
@@ -223,7 +224,14 @@ get_prompt(promptStatus_t status, ConditionalStack cstack)
break;
}
break;
-
+   /* output HH24:MI:SS */
+   case 'T':
+   {
+   time_t current_time = 
time(NULL);
+   struct tm *tm_info = 
localtime(_time);
+   sprintf(buf, "%02d:%02d:%02d", 
tm_info->tm_hour, tm_info->tm_min, tm_info->tm_sec);
+   }
+   break;
case 'x':
if (!pset.db)
buf[0] = '?';
-- 
2.25.1

Re: Proposal: %T Prompt parameter for psql for current time (like Oracle has)

[Kirk Wolak]

On Thu, Feb 23, 2023 at 2:05 PM Kirk Wolak  wrote:

> On Thu, Feb 23, 2023 at 9:52 AM Tom Lane  wrote:
>
>> Heikki Linnakangas  writes:
>> > On 23/02/2023 13:20, Peter Eisentraut wrote:
>> >> If you don't have \timing turned on before the query starts, psql won't
>> >> record what the time was before the query, so you can't compute the run
>> >> time afterwards.  This kind of feature would only work if you always
>> >> take the start time, even if \timing is turned off.
>>
>> > Correct. That seems acceptable though? gettimeofday() can be slow on
>> > some platforms, but I doubt it's *that* slow, that we couldn't call it
>> > two times per query.
>>
>> Yeah, you'd need to capture both the start and stop times even if
>> \timing isn't on, in case you get asked later.  But the backend is
>> going to call gettimeofday at least once per query, likely more
>> depending on what features you use.  And there are inherently
>> multiple kernel calls involved in sending a query and receiving
>> a response.  I tend to agree with Heikki that this overhead would
>> be unnoticeable.  (Of course, some investigation proving that
>> wouldn't be unwarranted.)
>>
>> regards, tom lane
>>
>
> Note, for this above feature, I was thinking we have a  ROW_COUNT variable
> I use \set to see.
> The simplest way to add this is maybe a set variable:  EXEC_TIME
> And it's set when ROW_COUNT gets set.
> +1
>
> ==
> Now, since this opened a lively discussion, I am officially submitting my
> first patch.
> This includes the small change to prompt.c and the documentation.  I had
> help from Andrey Borodin,
> and Pavel Stehule, who have supported me in how to propose, and use
> gitlab, etc.
>
> We are programmers... It's literally our job to sharpen our tools.  And
> PSQL is one of my most used.
> A small frustration, felt regularly was the motive.
>
> Regards, Kirk
> PS: If I am supposed to edit the subject to say there is a patch here, I
> did not know
> PPS: I appreciate ANY and ALL feedback... This is how we learn!
>

Patch Posted with one edit, for line editings (Thanks Jim!)
From b9db157177bbdeeeb6d35c3623ca9355141419d7 Mon Sep 17 00:00:00 2001
From: Jim Jones 
Date: Wed, 1 Mar 2023 00:07:55 +0100
Subject: [PATCH v2] Time option added to psql prompt

This adds a useful time option to the prompt: %T. Which does not
require a wasteful backquoted shell command which is also not
compatible between operating systems.
The format is simply HH24:MI:SS no other options available by design!

Author: Kirk Wolak 
Reviewed-By: Andrey Borodin 
Reviewed-By: Nikolay Samokhvalov 
Thread: 
https://postgr.es/m/CACLU5mSRwHr_8z%3DenMj-nXF1tmC7%2BJn5heZQNiKuLyxYUtL2fg%40mail.gmail.com
---
 doc/src/sgml/ref/psql-ref.sgml |  9 +
 src/bin/psql/prompt.c  | 10 +-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index dc6528dc11..04ab9eeb8c 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4575,6 +4575,15 @@ testdb= INSERT INTO my_table VALUES 
(:'content');
 
   
 
+  
+%T
+
+ 
+  The current time on the client in HH24:MI:SS format.
+ 
+
+  
+
   
 %x
 
diff --git a/src/bin/psql/prompt.c b/src/bin/psql/prompt.c
index 969cd9908e..0c0c725df5 100644
--- a/src/bin/psql/prompt.c
+++ b/src/bin/psql/prompt.c
@@ -41,6 +41,7 @@
  * or a ! if session is not connected to a database;
  * in prompt2 -, *, ', or ";
  * in prompt3 nothing
+ * %T - time in HH24:MI:SS format
  * %x - transaction status: empty, *, !, ? (unknown or no connection)
  * %l - The line number inside the current statement, starting from 1.
  * %? - the error code of the last query (not yet implemented)
@@ -223,7 +224,14 @@ get_prompt(promptStatus_t status, ConditionalStack cstack)
break;
}
break;
-
+   /* output HH24:MI:SS */
+   case 'T':
+   {
+   time_t current_time = 
time(NULL);
+   struct tm *tm_info = 
localtime(_time);
+   sprintf(buf, "%02d:%02d:%02d", 
tm_info->tm_hour, tm_info->tm_min, tm_info->tm_sec);
+   }
+   break;
case 'x':
if (!pset.db)
buf[0] = '?';
-- 
2.25.1

Re: Proposal: :SQL_EXEC_TIME (like :ROW_COUNT) Variable (psql)

[Kirk Wolak]

On Sun, Feb 26, 2023 at 11:45 PM Pavel Stehule 
wrote:

> po 27. 2. 2023 v 5:08 odesílatel Kirk Wolak  napsal:
>
>> On Fri, Feb 24, 2023 at 10:56 PM Kirk Wolak  wrote:
>>
>>> On Fri, Feb 24, 2023 at 2:11 AM Gurjeet Singh  wrote:
>>>
>>>> On Thu, Feb 23, 2023 at 8:42 PM Kirk Wolak  wrote:
>>>>
>>> ...
>>
>>> ...
>>>
>>> Okay, I've written and tested this using SQL_EXEC_ELAPSED (suggested
>> name improvement).
>> First, the instant you have ANY output, it swamps the impact. (I settled
>> on: SELECT 1 as v \gset xxx) for no output
>> Second, the variability of running even a constant script is mind-blowing.
>> Third, I've limited the output...  I built this in layers (init.sql
>> initializes the psql variables I use), run_100.sql runs
>> another file (\i tst_2000.sql) 100 times.  Resulting in 200k selects.
>>
>
> This is the very worst case.
>
> But nobody will run from psql 200K selects - can you try little bit more
> real but still synthetic test case?
>
> create table foo(a int);
> begin
>   insert into foo values(1);
>  ...
>   insert into foo values(20);
> commit;
>

*Without timing:*
postgres=# \i ins.sql
Elapsed Time: 29.518647 (seconds)
postgres=# \i ins.sql
Elapsed Time: 24.973943 (seconds)
postgres=# \i ins.sql
Elapsed Time: 21.916432 (seconds)
postgres=# \i ins.sql
Elapsed Time: 25.440978 (seconds)
postgres=# \i ins.sql
Elapsed Time: 24.848986 (seconds)

-- Because that was slower than expected, I exited, and tried again...
Getting really different results
postgres=# \i ins.sql
Elapsed Time: 17.763167 (seconds)
postgres=# \i ins.sql
Elapsed Time: 19.210436 (seconds)
postgres=# \i ins.sql
Elapsed Time: 19.903553 (seconds)
postgres=# \i ins.sql
Elapsed Time: 21.687750 (seconds)
postgres=# \i ins.sql
Elapsed Time: 19.046642 (seconds)



*With timing:*
\i ins.sql
Elapsed Time: 20.479442 (seconds)
postgres=# \i ins.sql
Elapsed Time: 21.493303 (seconds)
postgres=# \i ins.sql
Elapsed Time: 22.732409 (seconds)
postgres=# \i ins.sql
Elapsed Time: 20.246637 (seconds)
postgres=# \i ins.sql
Elapsed Time: 20.493607 (seconds)

Again, it's really hard to measure the difference as the impact, again, is
a bit below the variance.
In this case, I could see about a 1s - 2s (max)  difference in total time.
for 200k statements.
Run 5 times (for 1 million).

It's a little worse than noise.  But if I used the first run, the timing
version would have seemed faster.

I think this is sufficiently fast, and the patch simplifies the code.  We
end up only checking "if (timing)"
in the few places that we print the timing...

Anything else to provide?


>
> Regards
>
> Pavel
>
>
>>
>> Executive Summary:  1,000,000 statements executed, consumes ~2 - 2.5
>> seconds of extra time (Total)
>>
>> So, the per statement cost is: 2.5s / 1,000,000 = 0.000,0025 s per
>> statement
>> Roughly: 2.5us
>>
>> Unfortunately, my test lines look like this:
>> Without Timing
>> done 0.198215 (500) *total *98.862548 *min* 0.167614 *avg*
>> 0.197725096000 *max *0.290659
>>
>> With Timing
>> done 0.191583 (500) *total* 100.729868 *min *0.163280 *avg 
>> *0.201459736000
>> *max *0.275787
>>
>> Notice that the With Timing had a lower min, and a lower max.  But a
>> higher average.
>> The distance between min - avg  AND min - max, is big (those are for
>> 1,000 selects each)
>>
>> Are these numbers at the "So What" Level?
>>
>> While testing, I got the distinct impression that I am measuring
>> something that changes, or that the
>> variance in the system itself really swamps this on a per statement
>> basis.  It's only impact is felt
>> on millions of PSQL queries, and that's a couple of seconds...
>>
>> Curious what others think before I take this any further.
>>
>> regards, Kirk
>>
>>>
>>> Thanks!
>>>
>>>>
>>>> [1]:
>>>> https://github.com/gurjeet/home/blob/08f1051fb854f4fc8fbc4f1326f393ed507a55ce/.bashrc#L278
>>>> [2]:
>>>> https://github.com/gurjeet/home/blob/08f1051fb854f4fc8fbc4f1326f393ed507a55ce/.bashrc#L262
>>>>
>>>> Best regards,
>>>> Gurjeet
>>>> http://Gurje.et
>>>>
>>>

Re: Proposal: :SQL_EXEC_TIME (like :ROW_COUNT) Variable (psql)

[Kirk Wolak]

On Fri, Feb 24, 2023 at 10:56 PM Kirk Wolak  wrote:

> On Fri, Feb 24, 2023 at 2:11 AM Gurjeet Singh  wrote:
>
>> On Thu, Feb 23, 2023 at 8:42 PM Kirk Wolak  wrote:
>>
> ...

> >   I think like ROW_COUNT, it should not change because of internal
>> commands.
>> ...
>
> By using \timing, the user is explicitly opting into any overhead
>> caused by time-keeping. With this feature, the timing info will be
>> collected all the time. So do consider evaluating the performance
>> impact this can cause on people's workloads. They may not care for the
>> impact in interactive mode, but in automated scripts, even a moderate
>> performance overhead would be a deal-breaker.
>>
> Excellent point.  I run lots of long scripts, but I usually set \timing
> on, just because I turn off everything else.
> I tested 2,000+ lines of select 1; (Fast sql shouldn't matter, it's the
> most impacted)
> Honestly, it was imperceptible,  Maybe approximating 0.01 seconds
> With timing on:  ~ seconds 0.28
> With timing of:   ~ seconds 0.27
>
> The \timing incurs no realistic penalty at this point.  The ONLY penalty
> we could face is the time to
> write it to the variable, and that cannot be tested until implemented.
> But I will do that.  And I will
> report the results of the impact.  But I do not expect a big impact.  We
> update SQL_COUNT without an issue.
> And that might be much more expensive to get.
>

Okay, I've written and tested this using SQL_EXEC_ELAPSED (suggested name
improvement).
First, the instant you have ANY output, it swamps the impact. (I settled
on: SELECT 1 as v \gset xxx) for no output
Second, the variability of running even a constant script is mind-blowing.
Third, I've limited the output...  I built this in layers (init.sql
initializes the psql variables I use), run_100.sql runs
another file (\i tst_2000.sql) 100 times.  Resulting in 200k selects.

Executive Summary:  1,000,000 statements executed, consumes ~2 - 2.5
seconds of extra time (Total)

So, the per statement cost is: 2.5s / 1,000,000 = 0.000,0025 s per statement
Roughly: 2.5us

Unfortunately, my test lines look like this:
Without Timing
done 0.198215 (500) *total *98.862548 *min* 0.167614 *avg*
0.197725096000 *max *0.290659

With Timing
done 0.191583 (500) *total* 100.729868 *min *0.163280 *avg
*0.201459736000
*max *0.275787

Notice that the With Timing had a lower min, and a lower max.  But a higher
average.
The distance between min - avg  AND min - max, is big (those are for 1,000
selects each)

Are these numbers at the "So What" Level?

While testing, I got the distinct impression that I am measuring something
that changes, or that the
variance in the system itself really swamps this on a per statement basis.
It's only impact is felt
on millions of PSQL queries, and that's a couple of seconds...

Curious what others think before I take this any further.

regards, Kirk

>
> Thanks!
>
>>
>> [1]:
>> https://github.com/gurjeet/home/blob/08f1051fb854f4fc8fbc4f1326f393ed507a55ce/.bashrc#L278
>> [2]:
>> https://github.com/gurjeet/home/blob/08f1051fb854f4fc8fbc4f1326f393ed507a55ce/.bashrc#L262
>>
>> Best regards,
>> Gurjeet
>> http://Gurje.et
>>
>

Re: Proposal: :SQL_EXEC_TIME (like :ROW_COUNT) Variable (psql)

[Kirk Wolak]

On Fri, Feb 24, 2023 at 7:09 AM Jim Jones  wrote:

> On 23.02.23 20:55, Kirk Wolak wrote:
> > Everyone,
> ... SQL_EXEC_TIME
> >   I think like ROW_COUNT, it should not change because of internal
> > commands.
> > So, you guys +1 this thing, give additional comments.  When the
> > feedback settles, I commit to making it happen.
> >
> > Thanks, Kirk
> >
> I can see it being pretty handy to check if a certain task involving two
> different terminal windows was done in the right order. Basically to see
> what went wrong, e.g. "did I really stop the master database before
> promoting the replica?"
>
> +1 !
>
> Best, Jim
>

Jim, thanks, here is that patch for the %T option, but I think you did a +1
for the new psql variable :SQL_EXEC_TIME.
I realized my communication style needs to be cleaner, I caused that with
the lead in.

I created this proposal because I felt it was an excellent suggestion, and
I think it will be trivial to implement, although
it will involve a lot more testing...  MANY times, I have run a query that
took a touch too long, and I was wondering how long EXACTLY did that take.
This makes it easy  \echo :SQL_EXEC_TIME  (Well, I think it will be
SQL_EXEC_ELAPSED)

regards, kirk
From eaf6d05028052a3ccaa8d980953ac4fd75255250 Mon Sep 17 00:00:00 2001
From: Kirk Wolak 
Date: Thu, 23 Feb 2023 17:52:09 +
Subject: [PATCH] Time option added to psql prompt

This adds a useful time option to the prompt: %T. Which does not
require a wasteful backquoted shell command which is also not
compatible between operating systems.
The format is simply HH24:MI:SS no other options available by design!

Author: Kirk Wolak 
Reviewed-By: Andrey Borodin 
Reviewed-By: Nikolay Samokhvalov 
Thread: 
https://postgr.es/m/CACLU5mSRwHr_8z%3DenMj-nXF1tmC7%2BJn5heZQNiKuLyxYUtL2fg%40mail.gmail.com
---
 doc/src/sgml/ref/psql-ref.sgml |  9 +
 src/bin/psql/prompt.c  | 10 +-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index dc6528dc11d..04ab9eeb8c0 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4575,6 +4575,15 @@ testdb= INSERT INTO my_table VALUES 
(:'content');
 
   
 
+  
+%T
+
+ 
+  The current time on the client in HH24:MI:SS format.
+ 
+
+  
+
   
 %x
 
diff --git a/src/bin/psql/prompt.c b/src/bin/psql/prompt.c
index 969cd9908e5..a590c27389b 100644
--- a/src/bin/psql/prompt.c
+++ b/src/bin/psql/prompt.c
@@ -41,6 +41,7 @@
  * or a ! if session is not connected to a database;
  * in prompt2 -, *, ', or ";
  * in prompt3 nothing
+ * %T - time in HH24:MI:SS format
  * %x - transaction status: empty, *, !, ? (unknown or no connection)
  * %l - The line number inside the current statement, starting from 1.
  * %? - the error code of the last query (not yet implemented)
@@ -223,7 +224,14 @@ get_prompt(promptStatus_t status, ConditionalStack cstack)
break;
}
break;
-
+   /* output HH24:MI:SS */
+   case 'T':
+   {
+   time_t current_time = 
time(NULL);
+   struct tm *tm_info = 
localtime(_time);
+   sprintf(buf, "%02d:%02d:%02d", 
tm_info->tm_hour, tm_info->tm_min, tm_info->tm_sec);
 
+   }   
+   break;
case 'x':
if (!pset.db)
buf[0] = '?';
-- 
GitLab

Re: Proposal: :SQL_EXEC_TIME (like :ROW_COUNT) Variable (psql)

[Kirk Wolak]

On Fri, Feb 24, 2023 at 2:11 AM Gurjeet Singh  wrote:

> On Thu, Feb 23, 2023 at 8:42 PM Kirk Wolak  wrote:
> >   I love that my proposal for %T in the prompt, triggered some great
> conversations.
> >
> >   This is not instead of that.  That lets me run a query and come back
> HOURS later, and know it finished before 7PM like it was supposed to!
>
> Neat! I have this info embedded in my Bash prompt [1], but many a
> times this is not sufficient to reconstruct the time it took to run
> the shell command.
> ...
> >   I think like ROW_COUNT, it should not change because of internal
> commands.
>
> +1
>
> > So, you guys +1 this thing, give additional comments.  When the feedback
> settles, I commit to making it happen.
>
> This is definitely a useful feature. I agree with everything in the
> proposed UI (reporting in milliseconds, don't track internal commands'
> timing).
>
> I think 'duration' or 'elapsed' would be a better words in this
> context. So perhaps the name could be one of :sql_exec_duration (sql
> prefix feels superfluous), :exec_duration, :command_duration, or
> :elapsed_time.
>

I chose that prefix because it sorts near ROW_COUNT (LOL) when you do \SET

I agree that the name wasn't perfect...
I like SQL_EXEC_ELAPSED
keeping the result closer to ROW_COUNT, and it literally ONLY applies to SQL


> By using \timing, the user is explicitly opting into any overhead
> caused by time-keeping. With this feature, the timing info will be
> collected all the time. So do consider evaluating the performance
> impact this can cause on people's workloads. They may not care for the
> impact in interactive mode, but in automated scripts, even a moderate
> performance overhead would be a deal-breaker.
>
Excellent point.  I run lots of long scripts, but I usually set \timing on,
just because I turn off everything else.
I tested 2,000+ lines of select 1; (Fast sql shouldn't matter, it's the
most impacted)
Honestly, it was imperceptible,  Maybe approximating 0.01 seconds
With timing on:  ~ seconds 0.28
With timing of:   ~ seconds 0.27

The \timing incurs no realistic penalty at this point.  The ONLY penalty we
could face is the time to
write it to the variable, and that cannot be tested until implemented.  But
I will do that.  And I will
report the results of the impact.  But I do not expect a big impact.  We
update SQL_COUNT without an issue.
And that might be much more expensive to get.

Thanks!

>
> [1]:
> https://github.com/gurjeet/home/blob/08f1051fb854f4fc8fbc4f1326f393ed507a55ce/.bashrc#L278
> [2]:
> https://github.com/gurjeet/home/blob/08f1051fb854f4fc8fbc4f1326f393ed507a55ce/.bashrc#L262
>
> Best regards,
> Gurjeet
> http://Gurje.et
>

Proposal: :SQL_EXEC_TIME (like :ROW_COUNT) Variable (psql)

[Kirk Wolak]

Everyone,
  I love that my proposal for %T in the prompt, triggered some great
conversations.

  This is not instead of that.  That lets me run a query and come back
HOURS later, and know it finished before 7PM like it was supposed to!

  This feature is simple.  We forget to set \timing on...
We run a query, and we WONDER... how long did  that take.

  This, too, should be a trivial problem (the code will tell).

  I am proposing this to get feedback (I don't have a final design in mind,
but I will start by reviewing when and how ROW_COUNT gets set, and what
\timing reports).

  Next up, as I learn (and make mistakes), this toughens me up...

  I am not sure the name is right, but I would like to report it in the
same (ms) units as \timing, since there is an implicit relationship in what
they are doing.

  I think like ROW_COUNT, it should not change because of internal commands.
So, you guys +1 this thing, give additional comments.  When the feedback
settles, I commit to making it happen.

Thanks, Kirk

Re: Proposal: %T Prompt parameter for psql for current time (like Oracle has)

[Kirk Wolak]

On Thu, Feb 23, 2023 at 9:52 AM Tom Lane  wrote:

> Heikki Linnakangas  writes:
> > On 23/02/2023 13:20, Peter Eisentraut wrote:
> >> If you don't have \timing turned on before the query starts, psql won't
> >> record what the time was before the query, so you can't compute the run
> >> time afterwards.  This kind of feature would only work if you always
> >> take the start time, even if \timing is turned off.
>
> > Correct. That seems acceptable though? gettimeofday() can be slow on
> > some platforms, but I doubt it's *that* slow, that we couldn't call it
> > two times per query.
>
> Yeah, you'd need to capture both the start and stop times even if
> \timing isn't on, in case you get asked later.  But the backend is
> going to call gettimeofday at least once per query, likely more
> depending on what features you use.  And there are inherently
> multiple kernel calls involved in sending a query and receiving
> a response.  I tend to agree with Heikki that this overhead would
> be unnoticeable.  (Of course, some investigation proving that
> wouldn't be unwarranted.)
>
> regards, tom lane
>

Note, for this above feature, I was thinking we have a  ROW_COUNT variable
I use \set to see.
The simplest way to add this is maybe a set variable:  EXEC_TIME
And it's set when ROW_COUNT gets set.
+1

==
Now, since this opened a lively discussion, I am officially submitting my
first patch.
This includes the small change to prompt.c and the documentation.  I had
help from Andrey Borodin,
and Pavel Stehule, who have supported me in how to propose, and use gitlab,
etc.

We are programmers... It's literally our job to sharpen our tools.  And
PSQL is one of my most used.
A small frustration, felt regularly was the motive.

Regards, Kirk
PS: If I am supposed to edit the subject to say there is a patch here, I
did not know
PPS: I appreciate ANY and ALL feedback... This is how we learn!
From eaf6d05028052a3ccaa8d980953ac4fd75255250 Mon Sep 17 00:00:00 2001
From: Kirk Wolak 
Date: Thu, 23 Feb 2023 17:52:09 +
Subject: [PATCH] Time option added to psql prompt

This adds a useful time option to the prompt: %T. Which does not
require a wasteful backquoted shell command which is also not
compatible between operating systems.
The format is simply HH24:MI:SS no other options available by design!

Author: Kirk Wolak 
Reviewed-By: Andrey Borodin 
Reviewed-By: Nikolay Samokhvalov 
Thread: 
https://postgr.es/m/CACLU5mSRwHr_8z%3DenMj-nXF1tmC7%2BJn5heZQNiKuLyxYUtL2fg%40mail.gmail.com
---
 doc/src/sgml/ref/psql-ref.sgml |  9 +
 src/bin/psql/prompt.c  | 10 +-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index dc6528dc11d..04ab9eeb8c0 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4575,6 +4575,15 @@ testdb= INSERT INTO my_table VALUES 
(:'content');
 
   
 
+  
+%T
+
+ 
+  The current time on the client in HH24:MI:SS format.
+ 
+
+  
+
   
 %x
 
diff --git a/src/bin/psql/prompt.c b/src/bin/psql/prompt.c
index 969cd9908e5..a590c27389b 100644
--- a/src/bin/psql/prompt.c
+++ b/src/bin/psql/prompt.c
@@ -41,6 +41,7 @@
  * or a ! if session is not connected to a database;
  * in prompt2 -, *, ', or ";
  * in prompt3 nothing
+ * %T - time in HH24:MI:SS format
  * %x - transaction status: empty, *, !, ? (unknown or no connection)
  * %l - The line number inside the current statement, starting from 1.
  * %? - the error code of the last query (not yet implemented)
@@ -223,7 +224,14 @@ get_prompt(promptStatus_t status, ConditionalStack cstack)
break;
}
break;
-
+   /* output HH24:MI:SS */
+   case 'T':
+   {
+   time_t current_time = 
time(NULL);
+   struct tm *tm_info = 
localtime(_time);
+   sprintf(buf, "%02d:%02d:%02d", 
tm_info->tm_hour, tm_info->tm_min, tm_info->tm_sec);
 
+   }   
+   break;
case 'x':
if (!pset.db)
buf[0] = '?';
-- 
GitLab

Re: Proposal: %T Prompt parameter for psql for current time (like Oracle has)

[Kirk Wolak]

On Thu, Feb 23, 2023 at 1:16 PM Maciek Sakrejda 
wrote:

> On Thu, Feb 23, 2023, 09:55 Nikolay Samokhvalov 
> wrote:
>
>> On Thu, Feb 23, 2023 at 9:05 AM Maciek Sakrejda 
>> wrote:
>>
>>> I think Heikki's solution is probably more practical since (1) ..
>>
>>
>> Note that these ideas target two *different* problems:
>> - what was the duration of the last query
>> - when was the last query executed
>>
>> So, having both solved would be ideal.
>>
>
> Fair point, but since the duration solution needs to capture two
> timestamps anyway, it could print start time as well as duration.
>
> The prompt timestamp could still be handy for more intricate session
> forensics, but I don't know if that's a common-enough use case.
>
> Thanks,
> Maciek
>

It's really common during migrations, and forensics.  I often do a bunch of
stuff in 2 systems.  Then check the overlap.
Andrey brought up the value of 2 people separate working on things, being
able to go back and review when did you change that setting? Which has
happened to many of us in support sessions...

Thanks!

Re: Proposal: %T Prompt parameter for psql for current time (like Oracle has)

[Kirk Wolak]

On Wed, Feb 22, 2023 at 1:14 PM Heikki Linnakangas  wrote:

> On 22/02/2023 19:59, Nikolay Samokhvalov wrote:
> > On Wed, Feb 22, 2023 at 9:55 AM Tom Lane  > > wrote:
> >
> > On the whole I'd rather not eat more of the limited namespace for
> > psql prompt codes for this.
> >
> >
> > It depends on personal preferences. When I work on a large screen, I can
> > afford to spend some characters in prompts, if it gives convenience –
> > and many do (looking, for example, at modern tmux/zsh prompts showing
> > git branch context, etc).
> >
> > Default behavior might remain short – it wouldn't make sense to extend
> > it for everyone.
>
> I have no objections to adding a %T option, although deciding what
> format to use is a hassle. -1 for changing the default.
>
> But let's look at the original request:
>
> > This has been in sqlplus since I can remember, and I find it really
> > useful when I forgot to time something, or to review for Time spent
> > on a problem, or for how old my session is...
> I've felt that pain too. You run a query, and it takes longer than I
> expected. How long did it actually take? Too bad I didn't enable \timing
> beforehand..
>
> How about a new backslash command or psql variable to show how long the
> previous statement took? Something like:
>
> postgres=# select 
>   ?column?
> --
>123
> (1 row)
>
> postgres=# \time
>
> Time: 14011.975 ms (00:14.012)
>
> This would solve the "I forgot to time something" problem.
>
> - Heikki
>
> TBH, I have that turned on by default.  Load a script.  Have 300 of those
lines, and tell me how long it took?
In my case, it's much easier.  The other uses cases, including noticing I
changed some configuration and I
should reconnect (because I use multiple sessions, and I am in the early
stages with lots of changes).

Re: Proposal: %T Prompt parameter for psql for current time (like Oracle has)

[Kirk Wolak]

On Wed, Feb 22, 2023 at 12:55 PM Tom Lane  wrote:

> Kirk Wolak  writes:
> > Proposal:  Simply add the %T (PROMPT variable) to output the current time
> > (HH24:MI:SS) into the prompt.
>
> I'm not really convinced that %`date` isn't a usable solution for this,
> especially since it seems like a very niche requirement.  The next
> person who wants it might well have a different desire than you
> for exactly what gets shown.  The output of date can be customized,
> but a hard-wired prompt.c feature not so much.
>
> On the whole I'd rather not eat more of the limited namespace for
> psql prompt codes for this.
>
> regards, tom lane
>
Tom,
  I totally respect where you are coming from, and you are rightfully the
big dog!

In reverse order.  That limited namespace.  I assume you mean the 52 alpha
characters, of which, we are using 7,
and this change would make it 8.  Can we agree that at the current pace of
consumption it will be decades before
we get to 26, and they appear to be pretty well defended?

I already requested ONLY the HH24 format.  8 characters of output.  no
options.  It's a waste of time.
After all these years, sqlplus still has only one setting (show it, or
not).  I am asking the same here.
And I will gladly defend not changing it!  Ever!

I believe that leaves the real question:
Can't we just shell out? (which is what I do no, with issues as stated, and
a lot harder to do from memory if someplace new)

It's far easier in linux than windows to get what you want.
It's much more complicated if you try to use the same pgsqlrc file for
multiple environments and users.

We are talking about adding this much code, and consuming 1 of the
remaining 45 namespace items.
case 'T':
time_t current_time = time(NULL);
struct tm *tm_info = localtime(_time);
sprintf(buf, "%02d:%02d:%02d", tm_info->tm_hour,
tm_info->tm_min, tm_info->tm_sec);
break;

Does this help my case at all?
If I crossed any lines, it's not my intention.  I was tired of dealing with
this, and helping others to set it up.

With Respect,

Kirk

Proposal: %T Prompt parameter for psql for current time (like Oracle has)

[Kirk Wolak]

Proposal:  Simply add the %T (PROMPT variable) to output the current time
(HH24:MI:SS) into the prompt.  This has been in sqlplus since I can
remember, and I find it really useful when I forgot to time something, or
to review for Time spent on a problem, or for how old my session is...

I am recommending no formatting options, just keep it simple.  No, I don't
care about adding the date.  If I don't know the date of some line in my
history, it's already a problem!  (And date would logically be some other
variable)

Yes, I've found ways around it using the shell backquote.  This is hacky,
and it's also really ugly in windows. I also found it impossible to share
my plpgsqlrc file because between linux and windows.

This would be current time on the local machine.  Keeping it simple.

It feels like a small change.  The simplest test would be to capture the
prompt, select sleep(1.1); and make sure the prompt change.  This code
should be trivially stable.

If it seems useful, I believe I can work with others to get it implemented,
and the documentation changed, and a patch generated.  (I need to develop
these skills)

What does the community say?  Is there support for this?

Regards, Kirk

Re: possible proposal plpgsql GET DIAGNOSTICS oid = PG_ROUTINE_OID

[Kirk Wolak]

On Wed, Feb 8, 2023 at 3:08 AM Pavel Stehule 
wrote:

> hi
>
> st 8. 2. 2023 v 7:33 odesílatel Julien Rouhaud 
> napsal:
>
>> On Tue, Feb 07, 2023 at 08:48:22PM +0100, Pavel Stehule wrote:
>> >
>> > GET DIAGNOSTICS _oid = PG_ROUTINE_OID;
>> > RAISE NOTICE '... % ... %', _oid, _oid::regproc::text;
>> >
>> > Do you think it can be useful feature?
>>
>> +1, it would have been quite handy in a few of my projects.
>>
>
> it can looks like that
>
> create or replace function foo(a int)
> returns int as $$
> declare s text; n text; o oid;
> begin
>   get diagnostics s = pg_current_routine_signature,
>   n = pg_current_routine_name,
>   o = pg_current_routine_oid;
>   raise notice 'sign:%,  name:%,  oid:%', s, n, o;
>   return a;
> end;
> $$ language plpgsql;
> CREATE FUNCTION
> (2023-02-08 09:04:03) postgres=# select foo(10);
> NOTICE:  sign:foo(integer),  name:foo,  oid:16392
> ┌─┐
> │ foo │
> ╞═╡
> │  10 │
> └─┘
> (1 row)
>
> The name - pg_routine_oid can be confusing, because there is not clean if
> it is oid of currently executed routine or routine from top of exception
>
> Regards
>
> Pavel
>

I agree that the name changed to pg_current_routine_...  makes the most
sense, great call...

+1

Re: possible proposal plpgsql GET DIAGNOSTICS oid = PG_ROUTINE_OID

[Kirk Wolak]

On Tue, Feb 7, 2023 at 2:49 PM Pavel Stehule 
wrote:

> Hi
>
> I have a question about the possibility of simply getting the name of the
> currently executed function. The reason for this request is simplification
> of writing debug messages.
>
> GET DIAGNOSTICS _oid = PG_ROUTINE_OID;
> RAISE NOTICE '... % ... %', _oid, _oid::regproc::text;
>
> The advantage of this dynamic access to function name is always valid
> value not sensitive to some renaming or moving between schemas.
>
> I am able to separate a name from context, but it can be harder to write
> this separation really robustly. It can be very easy to enhance the GET
> DIAGNOSTICS statement to return the oid of currently executed function.
>
> Do you think it can be useful feature?
>

I was hoping it could be a CONSTANT like TG_OP (so the extra GET
DIAGNOSTICS wasn't invoked, but I have no idea the weight of that CODE
CHANGE)

Regardless, this concept is what we are looking for.  We prefer to leave
some debugging scaffolding in our DB Procedures, but disable it by default.
We are looking for a way to add something like this as a filter on the
level of output.

Our Current USE CASE is
  CALL LOGGING('Msg');  -- And by default nothing happens, unless we set
some session variables appropriately

We are looking for
  CALL LOGGING('Msg', __PG_ROUTINE_OID );  -- Now we can enable logging by
the routine we are interested in!

The LOGGING routine currently checks a session variable to see if logging
is EVEN Desired, if not it exits (eg PRODUCTION).

Now we can add a single line check, if p_funcoid  is IN my list of routines
I am debugging, send the output.

I will gladly work on the documentation side to help this happen!

+10




>
> The implementation should be trivial.
>
> Comments, notes?
>
> Regards
>
> Pavel
>
>
>

Help running 010_tab_completion.pl on windows

[Kirk Wolak]

I have psql working with readline (roughly) in windows 10!
in my attempt to test it...

>> 1..0 # SKIP IO::Pty is needed to run this test

I would like to run these tests to see how far off I am...
(Randomly typing sql and squealing like a child has its limits)

I have  built this using VS 2022 Community Edition.

The quick search failed to find an obvious answer.
One note in one of the strawberry .pm files read:
>ptys are not supported yet under Win32, but will be emulated...

Thanks in advance!

Re: PL/pgSQL cursors should get generated portal names by default

[Kirk Wolak]

On Mon, Nov 7, 2022 at 11:10 AM Jan Wieck  wrote:

> On 11/4/22 19:46, Tom Lane wrote:
> > Jan Wieck  writes:
> >> I need to do some testing on this. I seem to recall that the naming was
> >> originally done because a reference cursor is basically a named cursor
> >> that can be handed around between functions and even the top SQL level
> >> of the application. For the latter to work the application needs to
> know
> >> the name of the portal.
> >
> > Right.  With this patch, it'd be necessary to hand back the actual
> > portal name (by returning the refcursor value), or else manually
> > set the refcursor value before OPEN to preserve the previous behavior.
> > But as far as I saw, all our documentation examples show handing back
> > the portal name, so I'm hoping most people do it like that already.
>
> I was mostly concerned that we may unintentionally break underdocumented
> behavior that was originally implemented on purpose. As long as everyone
> is aware that this is breaking backwards compatibility in the way it
> does, that's fine.
>

I respect the concern, and applied some deeper thinking to it...

Here is the logic I am applying to this compatibility issue and what may
break.
[FWIW, my motto is to be wrong out  loud, as you learn faster]

At first pass, I thought "Well, since this does not break a refcursor,
which is the obvious use case for RETURNING/PASSING, we are fine!"

But in trying to DEFEND this case, I have come up with example of code
(that makes some SENSE, but would break):

CREATE FUNCTION test() RETURNS refcursor() LANGUAGE plpgsql AS  $$
DECLARE
cur_this cursor FOR SELECT 1;
ref_cur refcursor;
BEGIN
OPEN cur_this;
ref_cur := 'cur_this';  -- Using the NAME of the cursor as the portal
name: Should do:  ref_cur := cur_this; -- Only works after OPEN
RETURN ref_cur;
END;
$$;

As noted in the comments.  If the code were:
  ref_cur := 'cur_this';  -- Now you can't just use ref_cur := cur_this;
  OPEN cur_this;
  RETURN ref_cur;
Then it would break now...  And even the CORRECT syntax would break, since
the cursor was not opened, so "cur_this" is null.

Now, I have NO IDEA if someone would actually do this.  It is almost
pathological.  The use case would be a complex cursor with parameters,
and they changed the code to return a refcursor!
This was the ONLY use case I could think of that wasn't HACKY!

HACKY use cases involve a child routine setting:  local_ref_cursor :=
'cur_this'; in order to access a cursor that was NOT passed to the child.
FWIW, I tested this, and it works, and I can FETCH in the child routine,
and it affects the parents' LOOP as it should... WOW.  I would be HAPPY
to break such horrible code, it has to be a security concern at some level.

Personally (and my 2 cents really shouldn't matter much), I think this
should still be fixed.
Because I believe this small use case is rare, it will break immediately,
and the fix is trivial (just initialize cur_this := 'cur_this'  in this
example),
and the fix removes the Orthogonal Behavior Tom pointed out, which led me
to reporting this.

I think I have exhausted examples of how this impacts a VALID
refcursor implementation.  I believe any other such versions are variations
of this!
And maybe we document that if a refcursor of a cursor is to be returned,
that the refcursor is ASSIGNED after the OPEN of the cursor, and it is done
without the quotes, as:
  ref_cursor := cur_this;  -- assign the name after opening.

Thanks!