Re: Set access strategy for parallel vacuum workers

[Bharath Rupireddy]

On Thu, Apr 8, 2021 at 11:22 AM Amit Kapila  wrote:
> Yeah, I will change that before commit unless there are more suggestions.

I have no further comments on the patch
fix_access_strategy_workers_11.patch, it LGTM.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

maximum columns for brin bloom indexes

[Jaime Casanova]

Hi everyone,

When testing brin bloom indexes I noted that we need to reduce the
PAGES_PER_RANGE parameter of the index to allow more columns on it.

Sadly, this could be a problem if you create the index before the table
grows, once it reaches some number of rows (i see the error as early as
1000 rows) it starts error out.

create table t1(i int, j int);

-- uses default PAGES_PER_RANGE=128
create index on t1 using brin(i int4_bloom_ops, j int4_bloom_ops ) ;

insert into t1 
select random()*1000, random()*1000 from generate_series(1, 
1000);
ERROR:  index row size 8968 exceeds maximum 8152 for index "t1_i_j_idx"

if instead you create the index with a minor PAGES_PER_RANGE it goes
fine, in this case it works once you reduce it to at least 116

create index on t1 using brin(i int4_bloom_ops, j int4_bloom_ops ) 
with (pages_per_range=116);


so, for having:
two int columns - PAGES_PER_RANGE should be max 116
three int columns - PAGES_PER_RANGE should be max 77
one int and one timestamp - PAGES_PER_RANGE should be max 121 

and so on

-- 
Jaime Casanova
Director de Servicios Profesionales
SystemGuards - Consultores de PostgreSQL

Re: Support tab completion for upper character inputs in psql

[Peter Eisentraut]

On 01.04.21 11:40, tanghy.f...@fujitsu.com wrote:

On Wednesday, March 31, 2021 4:05 AM, David Zhang  wrote


   8 postgres=# update tbl SET DATA =
   9
  10 postgres=# update TBL SET
  11
  12 postgres=#

So, as you can see the difference is between line 8 and 10 in case 2. It
looks like the lowercase can auto complete more than the uppercase;
secondly, if you can add some test cases, it would be great.


Thanks for your test. I fix the bug and add some tests for it.
Please find attached the latest patch V4.

Differences from v3 are:
* fix an issue reported by Zhang [1] where a scenario was found which still 
wasn't able to realize tap completion in query.
* add some tap tests.


Seeing the tests you provided, it's pretty obvious that the current 
behavior is insufficient.  I think we could probably think of a few more 
tests, for example exercising the "If case insensitive matching was 
requested initially, adjust the case according to setting." case, or 
something with quoted identifiers.  I'll push this to the next commit 
fest for now.  I encourage you to keep working on it.

Re: MultiXact\SLRU buffers configuration

[Andrey Borodin]

> 8 апр. 2021 г., в 03:30, Thomas Munro  написал(а):
> 
> Here's another approach that is a little less exciting than
> "tournament RR" (or whatever that should be called; I couldn't find an
> established name for it).  This version is just our traditional linear
> search, except that it stops at 128, and remembers where to start from
> next time (like a sort of Fisher-Price GCLOCK hand).  This feels more
> committable to me.  You can argue that all buffers above 128 are bonus
> buffers that PostgreSQL 13 didn't have, so the fact that we can no
> longer find the globally least recently used page when you set
> xact_buffers > 128 doesn't seem too bad to me, as an incremental step
> (but to be clear, of course we can do better than this with more work
> in later releases).
I agree that this version of eviction seems much more effective and less 
intrusive than RR. And it's still LRU, which is important for subsystem that is 
called SLRU.
shared->search_slotno is initialized implicitly with memset(). But this seems 
like a common practice.
Also comment above "max_search = Min(shared->num_slots, 
MAX_REPLACEMENT_SEARCH);" does not reflect changes.

Besides this patch looks good to me.

Thanks!

Best regards, Andrey Borodin.

Re: missing documentation for streaming in-progress transactions

[Ajin Cherian]

On Thu, Apr 8, 2021 at 8:19 AM Peter Smith  wrote:

>
> 1.
> I felt that this protocol documentation needs to include something
> like a "Since: 2" notation (e.g. see how the javadoc API [1] does it)
> otherwise with more message types and more protocol versions it is
> quickly going to become too complicated to know what message or
> message attribute belongs with what protocol.
>
>
> Updated.


> 2.
> There are inconsistent terms used for a transaction id.
> e.g.1 Sometimes called " Transaction id."
> e.g.2 Sometimes called "Xid of the transaction"
> I think there should be consistent terminology used on this page
>

Updated.

regards,
Ajin Cherian
Fujitsu Australia


v3-0001-doc-Update-information-of-new-messages-for-logica.patch
Description: Binary data

Add option --drop-cascade for pg_dump/restore

[Haotian Wu]

Hello,

I'd like to propose adding `--drop-cascade` option for pg_dump/restore


Usecase:

I'd like to be able to restore an old custom format database dump as a
single transaction ( so the current data won't lose if restore fails). The
database has added some new constraints after backup so a CASCADE DROP is
needed.


 This allows for restoring an old backup after adding new constraints,

 at the risk of losing new data.


There're already some requests for supporting cascade drop:

   -
   
https://dba.stackexchange.com/questions/281384/pg-restore-clean-not-working-because-cascade-drop
   -
   
https://www.postgresql.org/message-id/flat/Pine.LNX.4.33.0308281409440.6957-10%40dev2.int.journyx.com
   -
   
https://www.postgresql.org/message-id/flat/50EC9574.9060500%40encs.concordia.ca


Design & Implementation


Basically I'm following the changes in adding `--if-exists` patch:
https://github.com/postgres/postgres/commit/9067310cc5dd590e36c2c3219dbf3961d7c9f8cb
. pg_dump/restore will inject a CASCADE clause to each DROP command.


The attached patch has been tested on our old backup. I'm happy to get some
feedback.


0001-pg_dump-restore-add-drop-cascade-option.patch
Description: Binary data

Re: Can we remove extra memset in BloomInitPage, GinInitPage and SpGistInitPage when we have it in PageInit?

[Michael Paquier]

On Thu, Apr 08, 2021 at 07:45:25AM +0530, Bharath Rupireddy wrote:
> On Wed, Apr 7, 2021 at 11:47 AM Bharath Rupireddy
>  wrote:
>> I wanted to comment out p->pd_flags = 0; in PageInit similar to the
>> pd_prune_xid just for consistency.
>> /* p->pd_prune_xid = InvalidTransactionId;done by above MemSet */
> 
> As I said above, just for consistency, I would like to see if the
> attached one line patch can be taken, even though it doesn't have any
> impact.

FWIW, I tend to prefer the existing style to keep around this code
rather than commenting it out, as one could think to remove it, but I
think that it can be important in terms of code comprehension when
reading the area.  So I quite like what 96ef3b8 has undone for
pd_flags, but not much what cc59049 did back in 2007.  That's a matter
of taste, really.
--
Michael


signature.asc
Description: PGP signature

Re: SQL-standard function body

[Michael Paquier]

On Wed, Apr 07, 2021 at 11:35:14PM -0700, Andres Freund wrote:
> On 2021-04-08 01:41:40 -0400, Tom Lane wrote:
>> Andres Freund  writes:
>> FWIW, I think the long-term drift of things is definitely that
>> we want to have the querystring available everywhere.  Code like
>> executor_errposition is from an earlier era before we were trying
>> to enforce that.  In particular, if the querystring is available in
>> the leader and not the workers, then you will get different error
>> reporting behavior in parallel query than non-parallel query, which
>> is surely a bad thing.
> 
> Yea, I think it's a sensible direction - but I think we should put the
> line in the sand earlier on / higher up than ExecInitParallelPlan().

Indeed, I agree that enforcing the availability of querystring
everywhere sounds like a sensible thing to do in terms of consistency,
and that's my impression when I scanned the parallel execution code,
and I don't really get why SQL function bodies should not bind by this
rule.  Would people object if I add an open item to track that?
--
Michael


signature.asc
Description: PGP signature

Re: Can we remove extra memset in BloomInitPage, GinInitPage and SpGistInitPage when we have it in PageInit?

[Bharath Rupireddy]

On Thu, Apr 8, 2021 at 1:22 PM Michael Paquier  wrote:
>
> On Thu, Apr 08, 2021 at 07:45:25AM +0530, Bharath Rupireddy wrote:
> > On Wed, Apr 7, 2021 at 11:47 AM Bharath Rupireddy
> >  wrote:
> >> I wanted to comment out p->pd_flags = 0; in PageInit similar to the
> >> pd_prune_xid just for consistency.
> >> /* p->pd_prune_xid = InvalidTransactionId;done by above MemSet 
> >> */
> >
> > As I said above, just for consistency, I would like to see if the
> > attached one line patch can be taken, even though it doesn't have any
> > impact.
>
> FWIW, I tend to prefer the existing style to keep around this code
> rather than commenting it out, as one could think to remove it, but I
> think that it can be important in terms of code comprehension when
> reading the area.  So I quite like what 96ef3b8 has undone for
> pd_flags, but not much what cc59049 did back in 2007.  That's a matter
> of taste, really.

Thanks! Since the main patch is committed I will go ahead and close
the CF entry.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

Re: PATCH: Attempt to make dbsize a bit more consistent

[Michael Paquier]

On Wed, Mar 17, 2021 at 02:35:28PM +0900, Michael Paquier wrote:
> So we would likely want a separate function.  Another possibility,
> which I find tempting, would be to push down the calculation logic
> relying on physical files down to the table AM themselves with a new
> dedicated callback (relation_size_physical?), as it seems to me that
> the most important thing users want to know with this set of functions
> is how much physical space is being consumed at one given point in
> time.  Attached is a small prototype I was just playing with.

Thinking more about that, I'd be rather in favor of having a new table
AM callback to let the AM measure the size of physical files, and make
the business of dbsize.c fall under that.  It is clear that this needs
more work, so I have marked it as returned with feedback.
--
Michael


signature.asc
Description: PGP signature

Re: Support for NSS as a libpq TLS backend

[Michael Paquier]

On Mon, Apr 05, 2021 at 11:12:22AM +0900, Michael Paquier wrote:
> Please find an updated set, v35, attached, and my apologies for
> breaking again your patch set.  While testing this patch set and
> adjusting the SSL tests with HEAD, I have noticed what looks like a
> bug with the DN mapping that NSS does not run.  The connection strings
> are the same in v35 and in v34, with dbname only changing in-between.
> 
> Just to be sure, because I could have done something wrong with the
> rebase of v35, I have done the same test with v34 applied on top of
> dfc843d and things are failing.  So it seems to me that there is an
> issue with the DN mapping part.

For now, I have marked this patch set as returned with feedback as it
is still premature for integration, and there are still bugs in it.
FWIW, I think that there is a future for providing an alternative to
OpenSSL, so, even if it could not make it for this release, I'd like
to push forward with this area more seriously as of 15.  The recent
libcrypto-related refactorings were one step in this direction, as
well.
--
Michael


signature.asc
Description: PGP signature

Re: pg_stat_statements oddity with track = all

[Magnus Hagander]

On Tue, Mar 16, 2021 at 4:34 PM Julien Rouhaud  wrote:
>
> On Tue, Mar 16, 2021 at 12:55:45PM +0100, Magnus Hagander wrote:
> > On Tue, Mar 9, 2021 at 3:39 AM Julien Rouhaud  wrote:
> > >
> > > I think that we might be able to handle that without a flag.  The only 
> > > thing
> > > that would need to be done is when creating an entry, look for an existing
> > > entry with the opposite flag, and if there's simply use the same
> > > (query_offset, query_len) info.  This doesn't sound that expensive.
> >
> > That's basically what I was trying to say :)
>
> Oh ok sorry :)
>
> > > The real pain point will be that the garbage collection phase
> > > will become way more expensive as it will now have to somehow maintain 
> > > that
> > > knowledge, which will require additional lookups for each entry.  I'm a 
> > > bit
> > > concerned about that, especially with the current heuristic to schedule 
> > > garbage
> > > collection.  For now, need_qc_qtext says that we have to do it if the 
> > > extent is
> > > more than 512 (B) * pgss_max.  This probably doesn't work well for people 
> > > using
> > > ORM as they tend to generate gigantic SQL queries.
> >
> > Right, the cost would be mostly on the GC  side. I've never done any
> > profiling to see how big of a thing that is in systems today -- have
> > you?
>
> I didn't, but I don't see how it could be anything but ridiculously impacting.
> it's basically preventing any query from being planned or executed on the 
> whole
> instance the time needed to read the previous qtext file, and write all 
> entries
> still needed.
>
> > > I don't that think that anyone really had a strong argument, mostly gut
> > > feeling.  Note that pg_stat_kcache already implemented that toplevel 
> > > flags, so
> > > if people are using that extension in a recent version they might have 
> > > some
> > > figures to show.  I'll ping some people that I know are using it.
> >
> > Great -- data always wins over gut feelings :)
>
> So I asked some friends that have latest pg_stat_kcache installed on some
> preproduction environment configured to track nested queries.  There isn't a
> high throughput but the activity should still be representative of the
> production queries.  There are a lot of applications plugged there, around 20
> databases and quite a lot of PL code.
>
> After a few days, here are the statistics:
>
> - total of ~ 9500 entries
> - ~ 900 entries for nested statements
> - ~ 35 entries existing for both top level and nested statements
>
> So the duplicates account for less than 4% of the nested statements, and less
> than 0.5% of the whole entries.
>
> I wish I had more reports, but if this one is representative enough then it
> seems that trying to avoid storing duplicated queries wouldn't be worth it.

I agree. If those numbers are indeed representable, it seems like
better to pay that overhead than to pay the overhead of trying to
de-dupe it.

Let's hope they are :)


Looking through ti again my feeling said the toplevel column should go
after the queryid and not before, but I'm not going to open up a
bikeshed over that.

I've added in a comment to cover that one that you removed (if you did
send an updated patch as you said, then I missed it -- sorry), and
applied the rest.

Thanks!

--
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

Re: Typo in jsonfuncs.c

[Julien Rouhaud]

On Thu, Apr 08, 2021 at 10:06:56AM +0900, Tatsuro Yamada wrote:
> Hi,
> 
> I found a typo in jsonfuncs.c, probably.
>   s/an an/an/
> Please find attached patch.

For the archives' sake, this has been pushed as of 8ffb003591.

Re: CREATE SEQUENCE with RESTART option

[Bharath Rupireddy]

On Thu, Apr 8, 2021 at 10:09 AM Amul Sul  wrote:
>
> On Wed, Apr 7, 2021 at 6:52 PM Bharath Rupireddy
>  wrote:
> >
> > On Wed, Apr 7, 2021 at 6:04 PM Ashutosh Bapat
> >  wrote:
> > > At best CREATE SEQUENCE  START ... RESTART ... can be a shorthand
> > > for CREATE SEQUENCE ... START; ALTER SEQUENCE ... RESTART run back to
> > > back. So it looks useful but in rare cases.
> >
> > I personally feel that let's not mix up START and RESTART in CREATE
> > SEQUENCE. If required, users will run ALTER SEQUENCE RESTART
> > separately, that will be a clean way.
> >
> > > Said all that I agree that if we are supporting CREATE SEQUENCE ...
> > > RESTART then we should document it, correctly. If that's not the
> > > intention, we should disallow RESTART with CREATE SEQUENCE.
> >
> > As I mentioned upthread, it's better to disallow (throw error) if
> > RESTART is specified for CREATE SEQUENCE. Having said that, I would
> > like to hear from others.
> >
>
> FWIW, +1.
>
> The RESTART clause in the CREATE SEQUENCE doesn't make sense
> to me, it should be restricted, IMO.

Thanks! Attaching a patch that throws an error if the RESTART option
is specified with CREATE SEQUENCE. Please have a look and let me know
if the error message wording is fine or not. Is it better to include
the reason as to why we disallow something like "Because it may
override the START option." in err_detail along with the error
message?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
From 819d653cc2be54e822826f08905f6ce8ec2f8418 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy 
Date: Thu, 8 Apr 2021 13:35:38 +0530
Subject: [PATCH v1] Disallow RESTART option for CREATE SEQUENCE

Restarting a sequence while defining it doesn't make any sense
and it may override the START value specified. Hence, we throw
error for CREATE SEQUENCE if RESTART option is specified.
However, it can be used with ALTER SEQUENCE.

If users want their sequences to be starting from a value different
than START value, then they can specify RESTART value with ALTER
SEQUENCE.
---
 src/backend/commands/sequence.c| 13 +
 src/test/regress/expected/sequence.out |  4 
 src/test/regress/sql/sequence.sql  |  1 +
 3 files changed, 18 insertions(+)

diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 0415df9ccb..5b75229360 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1290,6 +1290,19 @@ init_params(ParseState *pstate, List *options, bool for_identity,
 		}
 		else if (strcmp(defel->defname, "restart") == 0)
 		{
+			/*
+			 * Restarting a sequence while defining it doesn't make any sense
+			 * and it may override the START value. Allowing both START and
+			 * RESTART option for CREATE SEQUENCE may cause confusion to user.
+			 * Hence, we throw error for CREATE SEQUENCE if RESTART option is
+			 * specified. However, it can be used with ALTER SEQUENCE.
+			 */
+			if (isInit)
+ereport(ERROR,
+		(errcode(ERRCODE_SYNTAX_ERROR),
+		 errmsg("RESTART option is not allowed while creating sequence"),
+		 parser_errposition(pstate, defel->location)));
+
 			if (restart_value)
 ereport(ERROR,
 		(errcode(ERRCODE_SYNTAX_ERROR),
diff --git a/src/test/regress/expected/sequence.out b/src/test/regress/expected/sequence.out
index 8b928b2888..b9593bc8e4 100644
--- a/src/test/regress/expected/sequence.out
+++ b/src/test/regress/expected/sequence.out
@@ -16,6 +16,10 @@ CREATE SEQUENCE sequence_testx INCREMENT BY 1 START -10;
 ERROR:  START value (-10) cannot be less than MINVALUE (1)
 CREATE SEQUENCE sequence_testx CACHE 0;
 ERROR:  CACHE (0) must be greater than zero
+CREATE SEQUENCE sequence_testx RESTART 5;
+ERROR:  RESTART option is not allowed while creating sequence
+LINE 1: CREATE SEQUENCE sequence_testx RESTART 5;
+   ^
 -- OWNED BY errors
 CREATE SEQUENCE sequence_testx OWNED BY nobody;  -- nonsense word
 ERROR:  invalid OWNED BY option
diff --git a/src/test/regress/sql/sequence.sql b/src/test/regress/sql/sequence.sql
index 7928ee23ee..367157fe2b 100644
--- a/src/test/regress/sql/sequence.sql
+++ b/src/test/regress/sql/sequence.sql
@@ -10,6 +10,7 @@ CREATE SEQUENCE sequence_testx INCREMENT BY 1 MAXVALUE -20;
 CREATE SEQUENCE sequence_testx INCREMENT BY -1 START 10;
 CREATE SEQUENCE sequence_testx INCREMENT BY 1 START -10;
 CREATE SEQUENCE sequence_testx CACHE 0;
+CREATE SEQUENCE sequence_testx RESTART 5;
 
 -- OWNED BY errors
 CREATE SEQUENCE sequence_testx OWNED BY nobody;  -- nonsense word
-- 
2.25.1

Re: Wired if-statement in gen_partprune_steps_internal

[David Rowley]

On Thu, 8 Apr 2021 at 00:49, Amit Langote  wrote:
>
> Thanks David.  Actually, I was busy updating the patch to revert to
> gen_partprune_steps_internal() returning a list and was almost done
> with it when I saw your message.
>
> I read through v3 and can say that it certainly looks better than v2.
> If you are happy with gen_partprune_steps_internal() no longer
> returning a list, I would not object if you wanted to go ahead and
> commit the v3.
>
> I've attached the patch I had ended up with and was about to post as
> v3, just in case you wanted to glance.

Thanks.  I've made a pass over that and just fixed up the places that
were mixing up NIL and NULL.

I applied most of my comments from my last version after adapting them
to account for the variation in the functions return value. I also did
a bit more explaining about op steps and combine steps in the header
comment for gen_partprune_steps_internal.

Patch attached.

David


v5_fixup_partprune_dgr.patch
Description: Binary data

Re: Change JOIN tutorial to focus more on explicit joins

[Peter Eisentraut]

On 15.03.21 09:06, Jürgen Purtz wrote:

+1. All proposed changes integrated.


committed

Re: Wired if-statement in gen_partprune_steps_internal

[Amit Langote]

On Thu, Apr 8, 2021 at 5:34 PM David Rowley  wrote:
> On Thu, 8 Apr 2021 at 00:49, Amit Langote  wrote:
> >
> > Thanks David.  Actually, I was busy updating the patch to revert to
> > gen_partprune_steps_internal() returning a list and was almost done
> > with it when I saw your message.
> >
> > I read through v3 and can say that it certainly looks better than v2.
> > If you are happy with gen_partprune_steps_internal() no longer
> > returning a list, I would not object if you wanted to go ahead and
> > commit the v3.
> >
> > I've attached the patch I had ended up with and was about to post as
> > v3, just in case you wanted to glance.
>
> Thanks.  I've made a pass over that and just fixed up the places that
> were mixing up NIL and NULL.
>
> I applied most of my comments from my last version after adapting them
> to account for the variation in the functions return value. I also did
> a bit more explaining about op steps and combine steps in the header
> comment for gen_partprune_steps_internal.

Thanks for updating the patch.

+ * These partition pruning steps come in 2 forms; operation steps and combine
+ * steps.

Maybe you meant "operator" steps?  IIRC, the reason why we named it
PartitionPruneStepOp is that an op step is built to prune based on the
semantics of the operators that were involved in the matched clause.
Although, they're abused for pruning based on nullness clauses too.
Maybe, we should also updated the description of node struct as
follows to consider that last point:

 * PartitionPruneStepOp - Information to prune using a set of mutually ANDed
 *  OpExpr and any IS [ NOT ] NULL clauses

+ * Combine steps (PartitionPruneStepCombine) instruct the partition pruning
+ * code how it should produce a single set of partitions from multiple input
+ * operation steps.

I think the last part should be: ...from multiple operation/operator
and [ other ] combine steps.

If that sounds fine, likewise adjust the following sentences in the
same paragraph.

--
Amit Langote
EDB: http://www.enterprisedb.com

Re: TRUNCATE on foreign table

[Fujii Masao]

On 2021/04/08 15:48, Kohei KaiGai wrote:

2021年4月8日(木) 15:04 Fujii Masao :


On 2021/04/08 13:43, Kohei KaiGai wrote:

In case when a local table (with no children) has same contents,
TRUNCATE command
witll remove the entire table contents.


But if there are local child tables that inherit the local parent table, and TRUNCATE 
ONLY  is executed, only the contents in the parent will be 
truncated. I was thinking that this behavior should be applied to the foreign table 
whose remote (parent) table have remote child tables.

So what we need to reach the consensus is; how far ONLY option affects. Please 
imagine the case where we have

(1) local parent table, also foreign table of remote parent table
(2) local child table, inherits local parent table
(3) remote parent table
(4) remote child table, inherits remote parent table

I think that we agree all (1), (2), (3) and (4) should be truncated if local 
parent table (1) is specified without ONLY in TRUNCATE command. OTOH, if ONLY 
is specified, we agree that at least local child table (2) should NOT be 
truncated.


My understanding of a foreign table is a representation of external
data, including remote RDBMS but not only RDBMS,
regardless of the parent-child relationship at the local side.
So, once a local foreign table wraps entire tables tree (a parent and
relevant children) at the remote side, at least, it shall
be considered as a unified data chunk from the standpoint of the local side.


At least for me it's not intuitive to truncate the remote table and its all 
dependent tables even though users explicitly specify ONLY for the foreign 
table. As far as I read the past discussion, some people was thinking the same.



Please assume if file_fdw could map 3 different CSV files, then
truncate on the foreign table may eliminate just 1 of 3 files.
Is it an expected / preferable behavior?


I think that's up to each FDW. That is, IMO the information about whether ONLY 
is specified or not for each table should be passed to FDW. Then FDW itself 
should determine how to handle that information.

Anyway, attached is the updated version of the patch. This is still based on 
the latest Kazutaka-san's patch. That is, extra list for ONLY is still passed 
to FDW. What about committing this version at first? Then we can continue the 
discussion and change the behavior later if necessary.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/contrib/postgres_fdw/connection.c 
b/contrib/postgres_fdw/connection.c
index 6a61d83862..82aa14a65d 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -92,7 +92,6 @@ static PGconn *connect_pg_server(ForeignServer *server, 
UserMapping *user);
 static void disconnect_pg_server(ConnCacheEntry *entry);
 static void check_conn_params(const char **keywords, const char **values, 
UserMapping *user);
 static void configure_remote_session(PGconn *conn);
-static void do_sql_command(PGconn *conn, const char *sql);
 static void begin_remote_xact(ConnCacheEntry *entry);
 static void pgfdw_xact_callback(XactEvent event, void *arg);
 static void pgfdw_subxact_callback(SubXactEvent event,
@@ -568,7 +567,7 @@ configure_remote_session(PGconn *conn)
 /*
  * Convenience subroutine to issue a non-data-returning SQL command to remote
  */
-static void
+void
 do_sql_command(PGconn *conn, const char *sql)
 {
PGresult   *res;
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 5aa3455e30..bdc4c3620d 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -56,6 +56,7 @@
 #include "utils/rel.h"
 #include "utils/syscache.h"
 #include "utils/typcache.h"
+#include "commands/tablecmds.h"
 
 /*
  * Global context for foreign_expr_walker's search of an expression tree.
@@ -2172,6 +2173,43 @@ deparseAnalyzeSql(StringInfo buf, Relation rel, List 
**retrieved_attrs)
deparseRelation(buf, rel);
 }
 
+/*
+ * Construct a simple "TRUNCATE rel" statement
+ */
+void
+deparseTruncateSql(StringInfo buf,
+  List *rels,
+  List *rels_extra,
+  DropBehavior behavior,
+  bool restart_seqs)
+{
+   ListCell   *lc1,
+  *lc2;
+
+   appendStringInfoString(buf, "TRUNCATE ");
+
+   forboth(lc1, rels, lc2, rels_extra)
+   {
+   Relationrel = lfirst(lc1);
+   int extra = lfirst_int(lc2);
+
+   if (lc1 != list_head(rels))
+   appendStringInfoString(buf, ", ");
+   if (extra & TRUNCATE_REL_CONTEXT_ONLY)
+   appendStringInfoString(buf, "ONLY ");
+
+   deparseRelation(buf, rel);
+   }
+
+   appendStringInfo(buf, " %s IDENTITY",
+restart_seqs ? "RESTART" : 

Re: A new function to wait for the backend exit after termination

[Magnus Hagander]

On Mon, Apr 5, 2021 at 5:21 AM Bharath Rupireddy
 wrote:
>
> On Fri, Mar 19, 2021 at 11:37 AM Bharath Rupireddy
>  wrote:
> > Attaching v11 patch that removed the wait boolean flag in the
> > pg_terminate_backend and timeout 0 indicates "no wait", negative value
> > "errors out", positive value "waits for those many milliseconds". Also
> > addressed other review comments that I received upthread. Please
> > review v11 further.
>
> Attaching v12 patch after rebasing onto the latest master.

I've applied this patch with some minor changes.

I rewrote some parts of the documentation to make it more focused on
the end user rather than the implementation. I also made a small
simplification in pg_terminate_backend() which removes the "wait"
variable (seems like a bit of a leftover since the time when it was a
separate argument). And picked a correct oid for the function (oids
8000- should be used for new patches, 16386 is in the user area of
oids)

Thanks!

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

Re: CREATE SEQUENCE with RESTART option

[Suraj Kharage]

On Thu, Apr 8, 2021 at 2:03 PM Bharath Rupireddy <
bharath.rupireddyforpostg...@gmail.com> wrote:

>
> >
> > The RESTART clause in the CREATE SEQUENCE doesn't make sense
> > to me, it should be restricted, IMO.
>

+1


>
> Thanks! Attaching a patch that throws an error if the RESTART option
> is specified with CREATE SEQUENCE. Please have a look and let me know
> if the error message wording is fine or not. Is it better to include
> the reason as to why we disallow something like "Because it may
> override the START option." in err_detail along with the error
> message?
>

Patch looks good to me. Current error message looks ok to me.
Do we need to add double quotes for RESTART word in the error message since
it is an option?

-- 
--

Thanks & Regards,
Suraj kharage,



edbpostgres.com

Re: Remove page-read callback from XLogReaderState.

[Thomas Munro]

I squashed the patch set into one because half of them were fixups,
and the two main patches were really parts of the same change and
should go in together.

I fixed a few compiler warnings (GCC 10.2 reported several
uninitialised variables, comparisons that are always true, etc) and
some comments.  You can see these in the fixup patch.

+static inline void
+XLogReaderNotifySize(XLogReaderState *state, int32 len)

I think maybe it it should really be XLogReaderSetInputData(state,
tli, data, size) in a later release.  In the meantime, I changed it to
XLogReaderSetInputData(state, size), hope that name make sense...
From aa5ed37bef02bdbee2e11ad54f028e6bac54816a Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Thu, 5 Sep 2019 20:21:55 +0900
Subject: [PATCH v20 1/5] Remove read_page callback from XLogReadRecord.

Previously, the XLogReader module would fetch new input data using a
callback function.  Redesign the interface so that it tells the caller
to insert more data with a return value instead.  This API suits later
patches for prefetching, encryption and probably other future features
that would otherwise require extending the callback interface for new
projects.

As incidental cleanup work, move global variables readOff, readLen and
readSegNo inside XlogReaderState.

Author: Kyotaro HORIGUCHI 
Author: Heikki Linnakangas  (earlier version)
Reviewed-by: Antonin Houska 
Reviewed-by: Alvaro Herrera 
Reviewed-by: Takashi Menjo 
Reviewed-by: Andres Freund 
Reviewed-by: Thomas Munro 
Discussion: https://postgr.es/m/20190418.210257.43726183.horiguchi.kyotaro%40lab.ntt.co.jp
---
 src/backend/access/transam/twophase.c |  14 +-
 src/backend/access/transam/xlog.c | 129 ++-
 src/backend/access/transam/xlogreader.c   | 932 +++---
 src/backend/access/transam/xlogutils.c|  25 +-
 src/backend/replication/logical/logical.c |  26 +-
 .../replication/logical/logicalfuncs.c|  13 +-
 src/backend/replication/slotfuncs.c   |  18 +-
 src/backend/replication/walsender.c   |  42 +-
 src/bin/pg_rewind/parsexlog.c | 107 +-
 src/bin/pg_waldump/pg_waldump.c   | 141 +--
 src/include/access/xlogreader.h   | 157 +--
 src/include/access/xlogutils.h|   4 +-
 src/include/pg_config_manual.h|   2 +-
 src/include/replication/logical.h |  11 +-
 14 files changed, 940 insertions(+), 681 deletions(-)

diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 89335b64a2..3137cb3ecc 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1330,11 +1330,8 @@ XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len)
 	char	   *errormsg;
 	TimeLineID	save_currtli = ThisTimeLineID;
 
-	xlogreader = XLogReaderAllocate(wal_segment_size, NULL,
-	XL_ROUTINE(.page_read = &read_local_xlog_page,
-			   .segment_open = &wal_segment_open,
-			   .segment_close = &wal_segment_close),
-	NULL);
+	xlogreader = XLogReaderAllocate(wal_segment_size, NULL, wal_segment_close);
+
 	if (!xlogreader)
 		ereport(ERROR,
 (errcode(ERRCODE_OUT_OF_MEMORY),
@@ -1342,7 +1339,12 @@ XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len)
  errdetail("Failed while allocating a WAL reading processor.")));
 
 	XLogBeginRead(xlogreader, lsn);
-	record = XLogReadRecord(xlogreader, &errormsg);
+	while (XLogReadRecord(xlogreader, &record, &errormsg) ==
+		   XLREAD_NEED_DATA)
+	{
+		if (!read_local_xlog_page(xlogreader))
+			break;
+	}
 
 	/*
 	 * Restore immediately the timeline where it was previously, as
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index c1d4415a43..d3d6fb4643 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -811,17 +811,13 @@ static XLogSegNo openLogSegNo = 0;
  * These variables are used similarly to the ones above, but for reading
  * the XLOG.  Note, however, that readOff generally represents the offset
  * of the page just read, not the seek position of the FD itself, which
- * will be just past that page. readLen indicates how much of the current
- * page has been read into readBuf, and readSource indicates where we got
- * the currently open file from.
+ * will be just past that page. readSource indicates where we got the
+ * currently open file from.
  * Note: we could use Reserve/ReleaseExternalFD to track consumption of
  * this FD too; but it doesn't currently seem worthwhile, since the XLOG is
  * not read by general-purpose sessions.
  */
 static int	readFile = -1;
-static XLogSegNo readSegNo = 0;
-static uint32 readOff = 0;
-static uint32 readLen = 0;
 static XLogSource readSource = XLOG_FROM_ANY;
 
 /*
@@ -838,13 +834,6 @@ static XLogSource currentSource = XLOG_FROM_ANY;
 static bool lastSourceFailed = false;
 static bool pendingWalRcvRestart = false;
 
-typedef struct XLogPageReadPrivate
-{
-	

Re: [PATCH] Add --create-only option to pg_dump/pg_dumpall

[Magnus Hagander]

On Tue, Mar 30, 2021 at 6:02 PM Michael Banck  wrote:
>
> Hi,
>
> Am Montag, den 29.03.2021, 17:59 + schrieb Cary Huang:
> > I have tried the patch and the new option is able to control the
> > contents of pg_dump outputs to include only create db related
> > commands.
>
> Thanks for testing!
>
> > I also agree that the option name is a little misleading to the user
> > so I would suggest instead of using "create-only", you can say
> > something maybe like "createdb-only" because this option only applies
> > to CREATE DATABASE related commands, not CREATE TABLE or other
> > objects. In the help menu, you can then elaborate more that this
> > option "dump only the commands related to create database like ALTER,
> > GRANT..etc"
>
> Well I have to say I agree with Peter that the option name I came up
> with is pretty confusing, not sure createdb-only is much better as it
> also includes GRANTs etc.
>
> I think from a technical POV it's useful as it closes a gap between
> pg_dumpall -g and pg_dump -Fc $DATABASE in my opinion, without having to
> potentially schema-dump and filter out a large number of database
> objects.
>
> Anybody else have some opinions on what to call this best? Maybe just a
> short option and some explanatory text in --help along with it?

Maybe --database-globals or something like that?

Other than the name (which might be influenced by this), shouldn't
this functionality be in pg_restore as well? That is, if I make a
pg_dump in custom format, I would want to be able to extract that
information from the dump as well?

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

Re: TRUNCATE on foreign table

[Fujii Masao]

On 2021/04/08 18:25, Fujii Masao wrote:

Anyway, attached is the updated version of the patch. This is still based on 
the latest Kazutaka-san's patch. That is, extra list for ONLY is still passed 
to FDW. What about committing this version at first? Then we can continue the 
discussion and change the behavior later if necessary.


The patch failed to be applied because of recent commit.
Attached is the updated version of the patch.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/contrib/postgres_fdw/connection.c 
b/contrib/postgres_fdw/connection.c
index 6a61d83862..82aa14a65d 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -92,7 +92,6 @@ static PGconn *connect_pg_server(ForeignServer *server, 
UserMapping *user);
 static void disconnect_pg_server(ConnCacheEntry *entry);
 static void check_conn_params(const char **keywords, const char **values, 
UserMapping *user);
 static void configure_remote_session(PGconn *conn);
-static void do_sql_command(PGconn *conn, const char *sql);
 static void begin_remote_xact(ConnCacheEntry *entry);
 static void pgfdw_xact_callback(XactEvent event, void *arg);
 static void pgfdw_subxact_callback(SubXactEvent event,
@@ -568,7 +567,7 @@ configure_remote_session(PGconn *conn)
 /*
  * Convenience subroutine to issue a non-data-returning SQL command to remote
  */
-static void
+void
 do_sql_command(PGconn *conn, const char *sql)
 {
PGresult   *res;
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 5aa3455e30..bdc4c3620d 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -56,6 +56,7 @@
 #include "utils/rel.h"
 #include "utils/syscache.h"
 #include "utils/typcache.h"
+#include "commands/tablecmds.h"
 
 /*
  * Global context for foreign_expr_walker's search of an expression tree.
@@ -2172,6 +2173,43 @@ deparseAnalyzeSql(StringInfo buf, Relation rel, List 
**retrieved_attrs)
deparseRelation(buf, rel);
 }
 
+/*
+ * Construct a simple "TRUNCATE rel" statement
+ */
+void
+deparseTruncateSql(StringInfo buf,
+  List *rels,
+  List *rels_extra,
+  DropBehavior behavior,
+  bool restart_seqs)
+{
+   ListCell   *lc1,
+  *lc2;
+
+   appendStringInfoString(buf, "TRUNCATE ");
+
+   forboth(lc1, rels, lc2, rels_extra)
+   {
+   Relationrel = lfirst(lc1);
+   int extra = lfirst_int(lc2);
+
+   if (lc1 != list_head(rels))
+   appendStringInfoString(buf, ", ");
+   if (extra & TRUNCATE_REL_CONTEXT_ONLY)
+   appendStringInfoString(buf, "ONLY ");
+
+   deparseRelation(buf, rel);
+   }
+
+   appendStringInfo(buf, " %s IDENTITY",
+restart_seqs ? "RESTART" : "CONTINUE");
+
+   if (behavior == DROP_RESTRICT)
+   appendStringInfoString(buf, " RESTRICT");
+   else if (behavior == DROP_CASCADE)
+   appendStringInfoString(buf, " CASCADE");
+}
+
 /*
  * Construct name to use for given column, and emit it into buf.
  * If it has a column_name FDW option, use that instead of attribute name.
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out 
b/contrib/postgres_fdw/expected/postgres_fdw.out
index eeb6ae79d0..7f69fa0054 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -8215,6 +8215,205 @@ select * from rem3;
 drop foreign table rem3;
 drop table loc3;
 -- ===
+-- test for TRUNCATE
+-- ===
+CREATE TABLE tru_rtable0 (id int primary key);
+CREATE TABLE tru_rtable1 (id int primary key);
+CREATE FOREIGN TABLE tru_ftable (id int)
+   SERVER loopback OPTIONS (table_name 'tru_rtable0');
+INSERT INTO tru_rtable0 (SELECT x FROM generate_series(1,10) x);
+CREATE TABLE tru_ptable (id int) PARTITION BY HASH(id);
+CREATE TABLE tru_ptable__p0 PARTITION OF tru_ptable
+FOR VALUES WITH (MODULUS 2, REMAINDER 0);
+CREATE FOREIGN TABLE tru_ftable__p1 PARTITION OF tru_ptable
+FOR VALUES WITH (MODULUS 2, REMAINDER 1)
+   SERVER loopback OPTIONS (table_name 'tru_rtable1');
+INSERT INTO tru_ptable (SELECT x FROM generate_series(11,20) x);
+CREATE TABLE tru_pk_table(id int primary key);
+CREATE TABLE tru_fk_table(fkey int references tru_pk_table(id));
+INSERT INTO tru_pk_table (SELECT x FROM generate_series(1,10) x);
+INSERT INTO tru_fk_table (SELECT x % 10 + 1 FROM generate_series(5,25) x);
+CREATE FOREIGN TABLE tru_pk_ftable (id int)
+   SERVER loopback OPTIONS (table_name 'tru_pk_ta

Re: Catalog version access

[Magnus Hagander]

On Wed, Mar 3, 2021 at 8:16 PM Tom Lane  wrote:
>
> Vik Fearing  writes:
> > On 3/3/21 6:35 PM, Peter Eisentraut wrote:
> >> If what you want to know is whether a given binary can run against a
> >> given data directory then CATALOG_VERSION_NO isn't the only thing you
> >> need to check.  The full truth of this is in ReadControlFile().  The
> >> best way to get that answer is to start a server and see if it
> >> complains.  You can even grep the log for "It looks like you need to
> >> initdb.".
>
> > In that case, what would everyone think about a  `pg_ctl check`  option?
>
> The trouble with Peter's recipe is that it doesn't work if there is
> already a server instance running there (or at least I think we'll
> bitch about the existing postmaster first, maybe I'm wrong).  Now,
> that's not such a big problem for the use-case you were describing.
> But I bet if we expose this method as an apparently-general-purpose
> pg_ctl option, there'll be complaints.

Another option could be to provide a switch to the postmaster binary.
Using pg_config as originally suggested is risky because you might
pick up the wrong postmaster, but if you put it on the actual
postmaster binary you certainly know which one you're on... As this is
something that's primarily of interest to developers, it's also a bit
lower weight than having a "heavy" solution like an entire mode for
pg_ctl.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

Order dependency in function test

[Magnus Hagander]

Looking at 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&dt=2021-04-08%2009%3A43%3A13
which broke with the patch to add pg_wait_backend_termination().

AFAICT the change is that the order of rows coming back from "SELECT
routine_name, sequence_name FROM
information_schema.routine_sequence_usage" has changed. This test was
added  in f40c6969d0e ("Routine usage information schema tables"),

It does not change consistently, as it works fine on my machine and
has also passed on other buildfarm animals (including other archs and
compilers).

My guess is that maybe the query plan is different, ending up with a
different order, since there is no explicit ORDER BY in the query.

Is there a particular thing we want to check on it that requires it to
run without ORDER BY, or should we add one to solve the problem? Or,
of course, am I completely misunderstanding it? :)

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

Re: Autovacuum on partitioned table (autoanalyze)

[Tomas Vondra]

On 4/8/21 5:22 AM, Alvaro Herrera wrote:
> OK, I bit the bullet and re-did the logic in the way I had proposed
> earlier in the thread: do the propagation on the collector's side, by
> sending only the list of ancestors: the collector can read the tuple
> change count by itself, to add it to each ancestor.  This seems less
> wasteful.  Attached is v16 which does it that way and seems to work
> nicely under my testing.
> 
> However, I just noticed there is a huge problem, which is that the new
> code in relation_needs_vacanalyze() is doing find_all_inheritors(), and
> we don't necessarily have a snapshot that lets us do that.  While adding
> a snapshot acquisition at that spot is a very easy fix, I hesitate to
> fix it that way, because the whole idea there seems quite wasteful: we
> have to look up, open and lock every single partition, on every single
> autovacuum iteration through the database.  That seems bad.  I'm
> inclined to think that a better idea may be to store reltuples for the
> partitioned table in pg_class.reltuples, instead of having to add up the
> reltuples of each partition.  I haven't checked if this is likely to
> break anything.
> 

How would that value get updated, for the parent?

> (Also, a minor buglet: if we do ANALYZE (col1), then ANALYZE (col2) a
> partition, then we repeatedly propagate the counts to the parent table,
> so we would cause the parent to be analyzed more times than it should.
> Sounds like we should not send the ancestor list when a column list is
> given to manual analyze.  I haven't verified this, however.)
> 

Are you sure? I haven't tried, but shouldn't this be prevented by only
sending the delta between the current and last reported value?

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: maximum columns for brin bloom indexes

[Tomas Vondra]

On 4/8/21 9:08 AM, Jaime Casanova wrote:
> Hi everyone,
> 
> When testing brin bloom indexes I noted that we need to reduce the
> PAGES_PER_RANGE parameter of the index to allow more columns on it.
> 
> Sadly, this could be a problem if you create the index before the table
> grows, once it reaches some number of rows (i see the error as early as
> 1000 rows) it starts error out.
> 
>   create table t1(i int, j int);
>   
>   -- uses default PAGES_PER_RANGE=128
>   create index on t1 using brin(i int4_bloom_ops, j int4_bloom_ops ) ;
>   
>   insert into t1 
>   select random()*1000, random()*1000 from generate_series(1, 
> 1000);
>   ERROR:  index row size 8968 exceeds maximum 8152 for index "t1_i_j_idx"
> 
> if instead you create the index with a minor PAGES_PER_RANGE it goes
> fine, in this case it works once you reduce it to at least 116
> 
>   create index on t1 using brin(i int4_bloom_ops, j int4_bloom_ops ) 
>   with (pages_per_range=116);
> 
> 
> so, for having:
> two int columns - PAGES_PER_RANGE should be max 116
> three int columns - PAGES_PER_RANGE should be max 77
> one int and one timestamp - PAGES_PER_RANGE should be max 121 
> 
> and so on
> 

No, because this very much depends on the number if distinct values in
the page page range, which determines how well the bloom filter
compresses. You used 1000, but that's just an arbitrary value and the
actual data might have any other value. And it's unlikely that all three
columns will have the same number of distinct values.

Of course, this also depends on the false positive rate.

FWIW I doubt people are using multi-column BRIN indexes very often.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Order dependency in function test

[Peter Eisentraut]

On 08.04.21 12:04, Magnus Hagander wrote:

Looking at 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&dt=2021-04-08%2009%3A43%3A13
which broke with the patch to add pg_wait_backend_termination().

AFAICT the change is that the order of rows coming back from "SELECT
routine_name, sequence_name FROM
information_schema.routine_sequence_usage" has changed. This test was
added  in f40c6969d0e ("Routine usage information schema tables"),

It does not change consistently, as it works fine on my machine and
has also passed on other buildfarm animals (including other archs and
compilers).

My guess is that maybe the query plan is different, ending up with a
different order, since there is no explicit ORDER BY in the query.

Is there a particular thing we want to check on it that requires it to
run without ORDER BY, or should we add one to solve the problem? Or,
of course, am I completely misunderstanding it? :)


I added some ORDER BY clauses to fix this.

Re: Order dependency in function test

[Magnus Hagander]

On Thu, Apr 8, 2021 at 12:22 PM Peter Eisentraut
 wrote:
>
> On 08.04.21 12:04, Magnus Hagander wrote:
> > Looking at 
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&dt=2021-04-08%2009%3A43%3A13
> > which broke with the patch to add pg_wait_backend_termination().
> >
> > AFAICT the change is that the order of rows coming back from "SELECT
> > routine_name, sequence_name FROM
> > information_schema.routine_sequence_usage" has changed. This test was
> > added  in f40c6969d0e ("Routine usage information schema tables"),
> >
> > It does not change consistently, as it works fine on my machine and
> > has also passed on other buildfarm animals (including other archs and
> > compilers).
> >
> > My guess is that maybe the query plan is different, ending up with a
> > different order, since there is no explicit ORDER BY in the query.
> >
> > Is there a particular thing we want to check on it that requires it to
> > run without ORDER BY, or should we add one to solve the problem? Or,
> > of course, am I completely misunderstanding it? :)
>
> I added some ORDER BY clauses to fix this.

Thanks!

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

Re: Order dependency in function test

[Bharath Rupireddy]

On Thu, Apr 8, 2021 at 3:34 PM Magnus Hagander  wrote:
>
> Looking at 
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&dt=2021-04-08%2009%3A43%3A13
> which broke with the patch to add pg_wait_backend_termination().
>
> AFAICT the change is that the order of rows coming back from "SELECT
> routine_name, sequence_name FROM
> information_schema.routine_sequence_usage" has changed. This test was
> added  in f40c6969d0e ("Routine usage information schema tables"),
>
> It does not change consistently, as it works fine on my machine and
> has also passed on other buildfarm animals (including other archs and
> compilers).
>
> My guess is that maybe the query plan is different, ending up with a
> different order, since there is no explicit ORDER BY in the query.
>
> Is there a particular thing we want to check on it that requires it to
> run without ORDER BY, or should we add one to solve the problem? Or,
> of course, am I completely misunderstanding it? :)

The buildfarm failure is due to lack of ORDER BY clause. Upon
searching in that file, I found below statements are returning more
than one row but doesn't have ORDER BY clause which can make output
quite unstable.

SELECT routine_name, sequence_name FROM
information_schema.routine_sequence_usage;
SELECT routine_name, table_name, column_name FROM
information_schema.routine_column_usage;
SELECT routine_name, table_name FROM information_schema.routine_table_usage;
SELECT * FROM functest_sri1();
SELECT * FROM functest_sri2();
TABLE sometable;

I added a ORDER BY 1 clause for each of the above statements and
replaced TABLE sometable; with SELECT * FROM sometable ORDER BY 1;

Here's the patch.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com


v1-0001-Stabilize-tests-in-create_function_3.sql-with-ORD.patch
Description: Binary data

Re: Order dependency in function test

[Bharath Rupireddy]

On Thu, Apr 8, 2021 at 3:53 PM Bharath Rupireddy
 wrote:
>
> On Thu, Apr 8, 2021 at 3:34 PM Magnus Hagander  wrote:
> >
> > Looking at 
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&dt=2021-04-08%2009%3A43%3A13
> > which broke with the patch to add pg_wait_backend_termination().
> >
> > AFAICT the change is that the order of rows coming back from "SELECT
> > routine_name, sequence_name FROM
> > information_schema.routine_sequence_usage" has changed. This test was
> > added  in f40c6969d0e ("Routine usage information schema tables"),
> >
> > It does not change consistently, as it works fine on my machine and
> > has also passed on other buildfarm animals (including other archs and
> > compilers).
> >
> > My guess is that maybe the query plan is different, ending up with a
> > different order, since there is no explicit ORDER BY in the query.
> >
> > Is there a particular thing we want to check on it that requires it to
> > run without ORDER BY, or should we add one to solve the problem? Or,
> > of course, am I completely misunderstanding it? :)
>
> The buildfarm failure is due to lack of ORDER BY clause. Upon
> searching in that file, I found below statements are returning more
> than one row but doesn't have ORDER BY clause which can make output
> quite unstable.
>
> SELECT routine_name, sequence_name FROM
> information_schema.routine_sequence_usage;
> SELECT routine_name, table_name, column_name FROM
> information_schema.routine_column_usage;
> SELECT routine_name, table_name FROM information_schema.routine_table_usage;
> SELECT * FROM functest_sri1();
> SELECT * FROM functest_sri2();
> TABLE sometable;
>
> I added a ORDER BY 1 clause for each of the above statements and
> replaced TABLE sometable; with SELECT * FROM sometable ORDER BY 1;
>
> Here's the patch.

I realized that the ORDER BY is added. Isn't it good if we add ORDER
BY for SELECT * FROM functest_sri2();, SELECT * FROM functest_sri1();
and replace TABLE sometable; with SELECT * FROM sometable ORDER BY 1;
? Otherwise they might become unstable at some other time?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

Re: Replication slot stats misgivings

[Amit Kapila]

On Wed, Apr 7, 2021 at 2:51 PM vignesh C  wrote:
>

@@ -4069,6 +4069,24 @@ pgstat_read_statsfiles(Oid onlydb, bool
permanent, bool deep)
  * slot follows.
  */
  case 'R':
+ /*
+ * There is a remote scenario where one of the replication slots
+ * is dropped and the drop slot statistics message is not
+ * received by the statistic collector process, now if the
+ * max_replication_slots is reduced to the actual number of
+ * replication slots that are in use and the server is
+ * re-started then the statistics process will not be aware of
+ * this. To avoid writing beyond the max_replication_slots
+ * this replication slot statistic information will be skipped.
+ */
+ if (max_replication_slots == nReplSlotStats)
+ {
+ ereport(pgStatRunningInCollector ? LOG : WARNING,
+ (errmsg("skipping \"%s\" replication slot statistics as
pg_stat_replication_slots does not have enough slots",
+ NameStr(replSlotStats[nReplSlotStats].slotname;
+ goto done;
+ }

I think we might truncate some valid slots here. I have another idea
to fix this case which is that while writing, we first write the
'nReplSlotStats' and then write each slot info. Then while reading we
can allocate memory based on the required number of slots. Later when
startup process sends the slots, we can remove the already dropped
slots from this array. What do you think?

-- 
With Regards,
Amit Kapila.

Re: CREATE SEQUENCE with RESTART option

[Bharath Rupireddy]

On Thu, Apr 8, 2021 at 3:16 PM Suraj Kharage
 wrote:
>> > The RESTART clause in the CREATE SEQUENCE doesn't make sense
>> > to me, it should be restricted, IMO.
>
> +1
>
>>
>> Thanks! Attaching a patch that throws an error if the RESTART option
>> is specified with CREATE SEQUENCE. Please have a look and let me know
>> if the error message wording is fine or not. Is it better to include
>> the reason as to why we disallow something like "Because it may
>> override the START option." in err_detail along with the error
>> message?
>
>
> Patch looks good to me. Current error message looks ok to me.
> Do we need to add double quotes for RESTART word in the error message since 
> it is an option?

Thanks for taking a look at the patch. Looks like the other options
are used in the error message without quotes, see
"MINVALUE (%s) is out of range for sequence data type
"START value (%s) cannot be less than MINVALUE
"RESTART value (%s) cannot be less than MINVALUE
"CACHE (%s) must be greater than zero

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

Re: Replication slot stats misgivings

[vignesh C]

On Thu, Apr 8, 2021 at 4:00 PM Amit Kapila  wrote:
>
> On Wed, Apr 7, 2021 at 2:51 PM vignesh C  wrote:
> >
>
> @@ -4069,6 +4069,24 @@ pgstat_read_statsfiles(Oid onlydb, bool
> permanent, bool deep)
>   * slot follows.
>   */
>   case 'R':
> + /*
> + * There is a remote scenario where one of the replication slots
> + * is dropped and the drop slot statistics message is not
> + * received by the statistic collector process, now if the
> + * max_replication_slots is reduced to the actual number of
> + * replication slots that are in use and the server is
> + * re-started then the statistics process will not be aware of
> + * this. To avoid writing beyond the max_replication_slots
> + * this replication slot statistic information will be skipped.
> + */
> + if (max_replication_slots == nReplSlotStats)
> + {
> + ereport(pgStatRunningInCollector ? LOG : WARNING,
> + (errmsg("skipping \"%s\" replication slot statistics as
> pg_stat_replication_slots does not have enough slots",
> + NameStr(replSlotStats[nReplSlotStats].slotname;
> + goto done;
> + }
>
> I think we might truncate some valid slots here. I have another idea
> to fix this case which is that while writing, we first write the
> 'nReplSlotStats' and then write each slot info. Then while reading we
> can allocate memory based on the required number of slots. Later when
> startup process sends the slots, we can remove the already dropped
> slots from this array. What do you think?

I felt this idea is better, the reason being in the earlier idea we
might end up deleting some valid replication slot statistics and that
slot's statistics will never be available to the user.

Regards,
Vignesh

Re: Wired if-statement in gen_partprune_steps_internal

[David Rowley]

On Thu, 8 Apr 2021 at 21:04, Amit Langote  wrote:
> + * These partition pruning steps come in 2 forms; operation steps and combine
> + * steps.
>
> Maybe you meant "operator" steps?  IIRC, the reason why we named it
> PartitionPruneStepOp is that an op step is built to prune based on the
> semantics of the operators that were involved in the matched clause.
> Although, they're abused for pruning based on nullness clauses too.
> Maybe, we should also updated the description of node struct as
> follows to consider that last point:

Oh right. Thanks. I fixed that.

>  * PartitionPruneStepOp - Information to prune using a set of mutually ANDed
>  *  OpExpr and any IS [ NOT ] NULL clauses
>
> + * Combine steps (PartitionPruneStepCombine) instruct the partition pruning
> + * code how it should produce a single set of partitions from multiple input
> + * operation steps.

I didn't add that. I wasn't really sure if I understood why we'd talk
about PartitionPruneStepCombine in the PartitionPruneStepOp. I thought
the overview in gen_partprune_steps_internal was ok to link the two
together and explain why they're both needed.

> I think the last part should be: ...from multiple operation/operator
> and [ other ] combine steps.

Change that and pushed.

David

Re: Binary search in ScalarArrayOpExpr for OR'd constant arrays

[David Rowley]

On Thu, 8 Apr 2021 at 18:50, David Rowley  wrote:
> I've not done any further performance tests yet but will start those now.

I ran a set of tests on this:

select * from a where a in( < 1 to 10 > );
and
select * from a where a in( < 1 to 100 > );

the table "a" is just an empty table with a single int column.

I ran "pgbench -T 15 -c 16 -t 16" ten times each and the resulting tps
is averaged over the 10 runs.

With 10 items in the IN clause:
master: 99887.9098314 tps
patched: 103235.7616416 tps (3.35% faster)

With 100 items:
master: 62442.4838792 tps
patched:62275.4955754 tps (0.27% slower)

These tests are just designed to test the overhead of the additional
planning and expression initialisation.  Testing the actual
performance of the patch vs master with large IN lists shows the
expected significant speedups.

These results show that there's not much in the way of a measurable
slowdown in planning or executor startup from the additional code
which decides if we should hash the ScalarArrayOpExpr.

I think the changes in the patch are fairly isolated and the test
coverage is now pretty good.  I'm planning on looking at the patch
again now and will consider pushing it for PG14.

David

Re: DETAIL for wrong scram password

[Michael Paquier]

On Fri, Mar 26, 2021 at 09:49:00AM +0900, Michael Paquier wrote:
> Yes, you are right here.  I missed the parts before
> mock_scram_secret() gets called and there are comments in the whole
> area.  Hmm, at the end of the day, I think that would just have
> verify_client_proof() fill in logdetail when the client proof does not
> match, and use a wording different than what's proposed upthread to
> outline that this is a client proof mismatch.

Seeing no updates, this has been marked as RwF.
--
Michael


signature.asc
Description: PGP signature

Re: SQL-standard function body

[Julien Rouhaud]

On Thu, Apr 08, 2021 at 04:54:56PM +0900, Michael Paquier wrote:
> On Wed, Apr 07, 2021 at 11:35:14PM -0700, Andres Freund wrote:
> > On 2021-04-08 01:41:40 -0400, Tom Lane wrote:
> >> Andres Freund  writes:
> >> FWIW, I think the long-term drift of things is definitely that
> >> we want to have the querystring available everywhere.  Code like
> >> executor_errposition is from an earlier era before we were trying
> >> to enforce that.  In particular, if the querystring is available in
> >> the leader and not the workers, then you will get different error
> >> reporting behavior in parallel query than non-parallel query, which
> >> is surely a bad thing.
> > 
> > Yea, I think it's a sensible direction - but I think we should put the
> > line in the sand earlier on / higher up than ExecInitParallelPlan().
> 
> Indeed, I agree that enforcing the availability of querystring
> everywhere sounds like a sensible thing to do in terms of consistency,
> and that's my impression when I scanned the parallel execution code,
> and I don't really get why SQL function bodies should not bind by this
> rule.  Would people object if I add an open item to track that?

It makes sense, +1 for an open item.

Re: SQL-standard function body

[Julien Rouhaud]

On Thu, Apr 08, 2021 at 02:58:02AM -0400, Tom Lane wrote:
> Julien Rouhaud  writes:
> > On Wed, Apr 07, 2021 at 11:33:20PM -0700, Andres Freund wrote:
> >> Nothing special, really. Surprised the BF doesn't see it:
> 
> > Is think this is because the buildfarm client is running installcheck for 
> > the
> > contribs rather than check, and pg_stat_statements/Makefile has:
> > # Disabled because these tests require 
> > "shared_preload_libraries=pg_stat_statements",
> > # which typical installcheck users do not have (e.g. buildfarm clients).
> > NO_INSTALLCHECK = 1
> 
> No, because if that were the explanation then we'd be getting no
> buildfarm coverage at all for for pg_stat_statements.  Which aside
> from being awful contradicts the results at coverage.postgresql.org.

Is there any chance that coverage.postgresql.org isn't backed by the buildfarm
client but a plain make check-world or something like that?

> I think Andres has the right idea that there's some more-subtle
> variation in the test conditions, but (yawn) too tired to look
> into it right now.

I tried to look at some force-parallel-mode animal, like mantid, and I don't
see any evidence of pg_stat_statements being run by a "make check", and only a
few contrib modules seem to have an explicit check phase.  However, looking at
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mantid&dt=2021-04-08%2007%3A07%3A05
I see
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=mantid&dt=2021-04-08%2007%3A07%3A05&stg=contrib-install-check-C:

[...]
make -C pg_stat_statements installcheck
make[1]: Entering directory 
`/u1/tac/build-farm-11/buildroot/HEAD/pgsql.build/contrib/pg_stat_statements'
make[1]: Nothing to be done for `installcheck'.

Re: Truncate in synchronous logical replication failed

[Japin Li]

On Wed, 07 Apr 2021 at 16:34, tanghy.f...@fujitsu.com  
wrote:
> On Wednesday, April 7, 2021 5:28 PM Amit Kapila  
> wrote
>
>>Can you please check if the behavior is the same for PG-13? This is
>>just to ensure that we have not introduced any bug in PG-14.
>
> Yes, same failure happens at PG-13, too.
>

I found that when we truncate a table in synchronous logical replication,
LockAcquireExtended() [1] will try to take a lock via fast path and it
failed (FastPathStrongRelationLocks->count[fasthashcode] = 1).
However, it can acquire the lock when in asynchronous logical replication.
I'm not familiar with the locks, any suggestions? What the difference
between sync and async logical replication for locks?

[1]
if (EligibleForRelationFastPath(locktag, lockmode) &&
FastPathLocalUseCount < FP_LOCK_SLOTS_PER_BACKEND)
{
uint32  fasthashcode = FastPathStrongLockHashPartition(hashcode);
boolacquired;

/*
 * LWLockAcquire acts as a memory sequencing point, so it's safe to
 * assume that any strong locker whose increment to
 * FastPathStrongRelationLocks->counts becomes visible after we test
 * it has yet to begin to transfer fast-path locks.
 */
LWLockAcquire(&MyProc->fpInfoLock, LW_EXCLUSIVE);
if (FastPathStrongRelationLocks->count[fasthashcode] != 0)
acquired = false;
else
acquired = FastPathGrantRelationLock(locktag->locktag_field2,
 lockmode);
LWLockRelease(&MyProc->fpInfoLock);
if (acquired)
{
/*
 * The locallock might contain stale pointers to some old shared
 * objects; we MUST reset these to null before considering the
 * lock to be acquired via fast-path.
 */
locallock->lock = NULL;
locallock->proclock = NULL;
GrantLockLocal(locallock, owner);
return LOCKACQUIRE_OK;
}
}

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Simplify backend terminate and wait logic in postgres_fdw test

[Bharath Rupireddy]

Hi,

With the recent commit aaf0432572 which introduced a waiting/timeout
capability for pg_teriminate_backend function, I would like to do
$subject. Attaching a patch, please have a look.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com


v1-0001-Simplify-backend-terminate-and-wait-logic-in-post.patch
Description: Binary data

Re: Race condition in InvalidateObsoleteReplicationSlots()

[Amit Kapila]

On Thu, Apr 8, 2021 at 7:39 AM Andres Freund  wrote:
>
> On 2021-04-07 17:10:37 -0700, Andres Freund wrote:
> > I think this can be solved in two different ways:
> >
> > 1) Hold ReplicationSlotAllocationLock with LW_SHARED across most of
> >InvalidateObsoleteReplicationSlots(). That way nobody could re-create a 
> > new
> >slot in the to-be-obsoleted-slot's place.
> >
> > 2) Atomically check whether the slot needs to be invalidated and try to
> >acquire if needed. Don't release ReplicationSlotControlLock between those
> >two steps. Signal the owner to release the slot iff we couldn't acquire 
> > the
> >slot. In the latter case wait and then recheck if the slot still needs to
> >be dropped.
> >
> > To me 2) seems better, because we then can also be sure that the slot still
> > needs to be obsoleted, rather than potentially doing so unnecessarily.
> >

+1.

> >
> > It looks to me like several of the problems here stem from trying to reuse
> > code from ReplicationSlotAcquireInternal() (which before this was just named
> > ReplicationSlotAcquire()).  I don't think that makes sense, because cases 
> > like
> > this want to check if a condition is true, and acquire it only if so.
> >
> > IOW, I think this basically needs to look like 
> > ReplicationSlotsDropDBSlots(),
> > except that a different condition is checked, and the if (active_pid) case
> > needs to prepare a condition variable, signal the owner and then wait on the
> > condition variable, to restart after.
>
> I'm also confused by the use of ConditionVariableTimedSleep(timeout =
> 10). Why do we need a timed sleep here in the first place? And why with
> such a short sleep?
>
> I also noticed that the code is careful to use CHECK_FOR_INTERRUPTS(); -
> but is aware it's running in checkpointer. I don't think CFI does much
> there? If we are worried about needing to check for interrupts, more
> work is needed.
>
>
> Sketch for a fix attached. I did leave the odd
> ConditionVariableTimedSleep(10ms) in, because I wasn't sure why it's
> there...
>

I haven't tested the patch but I couldn't spot any problems while
reading it. A minor point, don't we need to use
ConditionVariableCancelSleep() at some point after doing
ConditionVariableTimedSleep?

-- 
With Regards,
Amit Kapila.

Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?

[Thomas Munro]

Hi Julien, Bruce,

A warning appears on 32 bit systems:

In file included from pgstatfuncs.c:15:
pgstatfuncs.c: In function 'pg_stat_get_activity':
../../../../src/include/postgres.h:593:29: warning: cast to pointer
from integer of different size [-Wint-to-pointer-cast]
  593 | #define DatumGetPointer(X) ((Pointer) (X))
  | ^
../../../../src/include/postgres.h:678:42: note: in expansion of macro
'DatumGetPointer'
  678 | #define DatumGetUInt64(X) (* ((uint64 *) DatumGetPointer(X)))
  |  ^~~
pgstatfuncs.c:920:18: note: in expansion of macro 'DatumGetUInt64'
  920 | values[29] = DatumGetUInt64(beentry->st_queryid);
  |  ^~

Hmm, maybe this should be UInt64GetDatum()?

Re: WIP: WAL prefetch (another approach)

[Thomas Munro]

On Thu, Apr 8, 2021 at 3:27 AM Tomas Vondra
 wrote:
> On 4/7/21 1:24 PM, Thomas Munro wrote:
> > I made one other simplifying change: previously, the prefetch module
> > would read the WAL up to the "written" LSN (so, allowing itself to
> > read data that had been written but not yet flushed to disk by the
> > walreceiver), though it still waited until a record's LSN was
> > "flushed" before replaying.  That allowed prefetching to happen
> > concurrently with the WAL flush, which was nice, but it felt a little
> > too "special".  I decided to remove that part for now, and I plan to
> > look into making standbys work more like primary servers, using WAL
> > buffers, the WAL writer and optionally the standard log-before-data
> > rule.
>
> Not sure, but the removal seems unnecessary. I'm worried that this will
> significantly reduce the amount of data that we'll be able to prefetch.
> How likely it is that we have data that is written but not flushed?
> Let's assume the replica is lagging and network bandwidth is not the
> bottleneck - how likely is this "has to be flushed" a limit for the
> prefetching?

Yeah, it would have been nice to include that but it'll have to be for
v15 due to lack of time to convince myself that it was correct.  I do
intend to look into more concurrency of that kind for v15.  I have
pushed these patches, updated to be disabled by default.  I will look
into how I can run a BF animal that has it enabled during the recovery
tests for coverage.  Thanks very much to everyone on this thread for
all the discussion and testing so far.

Re: Remove page-read callback from XLogReaderState.

[Thomas Munro]

On Thu, Apr 8, 2021 at 9:46 PM Thomas Munro  wrote:
> I squashed the patch set into one because half of them were fixups,
> and the two main patches were really parts of the same change and
> should go in together.
>
> I fixed a few compiler warnings (GCC 10.2 reported several
> uninitialised variables, comparisons that are always true, etc) and
> some comments.  You can see these in the fixup patch.

Pushed.  Luckily there are plenty more improvements possible for
XLogReader/XLogDecoder in the next cycle.

Re: Simplify backend terminate and wait logic in postgres_fdw test

[Michael Paquier]

On Thu, Apr 08, 2021 at 04:55:22PM +0530, Bharath Rupireddy wrote:
> With the recent commit aaf0432572 which introduced a waiting/timeout
> capability for pg_teriminate_backend function, I would like to do
> $subject. Attaching a patch, please have a look.

+-- Terminate the remote backend having the specified application_name and wait
+-- for the termination to complete. 10 seconds timeout here is chosen randomly,
+-- we will see a warning if the process doesn't go away within that time.
+SELECT pg_terminate_backend(pid, 1) FROM pg_stat_activity
+WHERE application_name = 'fdw_retry_check';

I think that you are making the tests less stable by doing that.  A
couple of buildfarm machines are very slow, and 10 seconds would not
be enough.  So it seems to me that this patch is trading what is a
stable solution for a solution that may finish by randomly bite.
--
Michael


signature.asc
Description: PGP signature

Re: Wired if-statement in gen_partprune_steps_internal

[Amit Langote]

On Thu, Apr 8, 2021 at 7:41 PM David Rowley  wrote:
> On Thu, 8 Apr 2021 at 21:04, Amit Langote  wrote:
> > Maybe, we should also updated the description of node struct as
> > follows to consider that last point:
>>
> >  * PartitionPruneStepOp - Information to prune using a set of mutually ANDed
> >  *  OpExpr and any IS [ NOT ] NULL clauses
>
> I didn't add that. I wasn't really sure if I understood why we'd talk
> about PartitionPruneStepCombine in the PartitionPruneStepOp. I thought
> the overview in gen_partprune_steps_internal was ok to link the two
> together and explain why they're both needed.

Sorry, maybe the way I wrote it was a bit confusing, but I meant to
suggest that we do what I have quoted above from my last email.  That
is, we should clarify in the description of PartitionPruneStepOp that
it contains information derived from OpExprs and in some cases also IS
[ NOT ] NULL clauses.

Thanks for the commit.

-- 
Amit Langote
EDB: http://www.enterprisedb.com

Re: Binary search in ScalarArrayOpExpr for OR'd constant arrays

[David Rowley]

On Thu, 8 Apr 2021 at 22:54, David Rowley  wrote:
>
> I think the changes in the patch are fairly isolated and the test
> coverage is now pretty good.  I'm planning on looking at the patch
> again now and will consider pushing it for PG14.

I push this with some minor cleanup from the v6 patch I posted earlier.

David

Re: pg_stat_statements oddity with track = all

[Julien Rouhaud]

On Thu, Apr 08, 2021 at 10:30:53AM +0200, Magnus Hagander wrote:
> 
> I agree. If those numbers are indeed representable, it seems like
> better to pay that overhead than to pay the overhead of trying to
> de-dupe it.
> 
> Let's hope they are :)

:)

> Looking through ti again my feeling said the toplevel column should go
> after the queryid and not before, but I'm not going to open up a
> bikeshed over that.
> 
> I've added in a comment to cover that one that you removed (if you did
> send an updated patch as you said, then I missed it -- sorry), and
> applied the rest.

Oops, somehow I totally forgot to send the new patch, sorry :(

While looking at the patch, I unfortunately just realize that I unnecessarily
bumped the version to 1.10, as 1.9 was already new as of pg14.  Honestly I have
no idea why I used 1.10 at that time.  Version numbers are not a scarce
resource but maybe it would be better to keep 1.10 for a future major postgres
version?

If yes, PFA a patch to merge 1.10 in 1.9.
>From b2b3102fa16d2b02d1838cf7853d0869dbb966cc Mon Sep 17 00:00:00 2001
From: Julien Rouhaud 
Date: Thu, 8 Apr 2021 19:52:33 +0800
Subject: [PATCH v1] Don't bump pg_stat_statements to 1.10 in REL_14_STABLE.

---
 contrib/pg_stat_statements/Makefile   |  3 +-
 .../pg_stat_statements--1.8--1.9.sql  | 53 +
 .../pg_stat_statements--1.9--1.10.sql | 57 ---
 .../pg_stat_statements/pg_stat_statements.c   | 18 +++---
 .../pg_stat_statements.control|  2 +-
 5 files changed, 64 insertions(+), 69 deletions(-)
 delete mode 100644 contrib/pg_stat_statements/pg_stat_statements--1.9--1.10.sql

diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index cab4f626ad..3ec627b956 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -6,8 +6,7 @@ OBJS = \
 	pg_stat_statements.o
 
 EXTENSION = pg_stat_statements
-DATA = pg_stat_statements--1.4.sql \
-pg_stat_statements--1.9--1.10.sql pg_stat_statements--1.8--1.9.sql \
+DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.8--1.9.sql \
 	pg_stat_statements--1.7--1.8.sql pg_stat_statements--1.6--1.7.sql \
 	pg_stat_statements--1.5--1.6.sql pg_stat_statements--1.4--1.5.sql \
 	pg_stat_statements--1.3--1.4.sql pg_stat_statements--1.2--1.3.sql \
diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.8--1.9.sql b/contrib/pg_stat_statements/pg_stat_statements--1.8--1.9.sql
index 3504ca7eb1..c45223f888 100644
--- a/contrib/pg_stat_statements/pg_stat_statements--1.8--1.9.sql
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.8--1.9.sql
@@ -16,3 +16,56 @@ CREATE VIEW pg_stat_statements_info AS
   SELECT * FROM pg_stat_statements_info();
 
 GRANT SELECT ON pg_stat_statements_info TO PUBLIC;
+
+/* First we have to remove them from the extension */
+ALTER EXTENSION pg_stat_statements DROP VIEW pg_stat_statements;
+ALTER EXTENSION pg_stat_statements DROP FUNCTION pg_stat_statements(boolean);
+
+/* Then we can drop them */
+DROP VIEW pg_stat_statements;
+DROP FUNCTION pg_stat_statements(boolean);
+
+/* Now redefine */
+CREATE FUNCTION pg_stat_statements(IN showtext boolean,
+OUT userid oid,
+OUT dbid oid,
+OUT toplevel bool,
+OUT queryid bigint,
+OUT query text,
+OUT plans int8,
+OUT total_plan_time float8,
+OUT min_plan_time float8,
+OUT max_plan_time float8,
+OUT mean_plan_time float8,
+OUT stddev_plan_time float8,
+OUT calls int8,
+OUT total_exec_time float8,
+OUT min_exec_time float8,
+OUT max_exec_time float8,
+OUT mean_exec_time float8,
+OUT stddev_exec_time float8,
+OUT rows int8,
+OUT shared_blks_hit int8,
+OUT shared_blks_read int8,
+OUT shared_blks_dirtied int8,
+OUT shared_blks_written int8,
+OUT local_blks_hit int8,
+OUT local_blks_read int8,
+OUT local_blks_dirtied int8,
+OUT local_blks_written int8,
+OUT temp_blks_read int8,
+OUT temp_blks_written int8,
+OUT blk_read_time float8,
+OUT blk_write_time float8,
+OUT wal_records int8,
+OUT wal_fpi int8,
+OUT wal_bytes numeric
+)
+RETURNS SETOF record
+AS 'MODULE_PATHNAME', 'pg_stat_statements_1_9'
+LANGUAGE C STRICT VOLATILE PARALLEL SAFE;
+
+CREATE VIEW pg_stat_statements AS
+  SELECT * FROM pg_stat_statements(true);
+
+GRANT SELECT ON pg_stat_statements TO PUBLIC;
diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.9--1.10.sql b/contrib/pg_stat_statements/pg_stat_statements--1.9--1.10.sql
deleted file mode 100644
index f97d16497d..00
--- a/contrib/pg_stat_statements/pg_stat_statements--1.9--1.10.sql
+++ /dev/null
@@ -1,57 +0,0 @@
-/* contrib/pg_stat_statements/pg_stat_statements--1.9--1.10.sql */
-
--- complain if script is sourced in psql, rather than via ALTER EXTENSION
-\echo Use "ALTER EXTENSION pg_stat_statements UPDATE TO '1.10'" to load this file. \quit
-
-/* First we have to remove them from the extension */
-ALTER EXTENSION pg_s

Re: PostgreSQL 14 Feature Freeze + Release Management Team (RMT)

[Michael Paquier]

On Mon, Mar 22, 2021 at 10:17:56AM +0900, Michael Paquier wrote:
> The Release Management Team (RMT) for the PostgreSQL 14 is assembled
> and has determined that the feature freeze date for the PostgreSQL 11
> release will be April 7, 2021.  This means that any feature for the
> PostgreSQL 14 release *must be committed by April 7, 2021 AoE*
> ("anywhere on earth", see [1]).  In other words, by April 8, it is too
> late.

And so, here we are.
--
Michael


signature.asc
Description: PGP signature

Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?

[Julien Rouhaud]

On Thu, Apr 08, 2021 at 11:36:48PM +1200, Thomas Munro wrote:
> Hi Julien, Bruce,
> 
> A warning appears on 32 bit systems:
> 
> In file included from pgstatfuncs.c:15:
> pgstatfuncs.c: In function 'pg_stat_get_activity':
> ../../../../src/include/postgres.h:593:29: warning: cast to pointer
> from integer of different size [-Wint-to-pointer-cast]
>   593 | #define DatumGetPointer(X) ((Pointer) (X))
>   | ^
> ../../../../src/include/postgres.h:678:42: note: in expansion of macro
> 'DatumGetPointer'
>   678 | #define DatumGetUInt64(X) (* ((uint64 *) DatumGetPointer(X)))
>   |  ^~~
> pgstatfuncs.c:920:18: note: in expansion of macro 'DatumGetUInt64'
>   920 | values[29] = DatumGetUInt64(beentry->st_queryid);
>   |  ^~

Wow, that's really embarrassing :(

> Hmm, maybe this should be UInt64GetDatum()?

Yes definitely.  I'm attaching the previous patch for force_parallel_mode to
not forget it + a new one for this issue.
>From d74523dfb76e7583c27166ec10d72670654c3b7b Mon Sep 17 00:00:00 2001
From: Julien Rouhaud 
Date: Thu, 8 Apr 2021 13:59:43 +0800
Subject: [PATCH v2 1/2] Ignore parallel workers in pg_stat_statements.

Oversight in 4f0b0966c8 which exposed queryid in parallel workers.  Counters
are aggregated by the main backend process so parallel workers would report
duplicated activity, and could also report activity for the wrong entry as they
are only aware of the top level queryid.

Author: Julien Rouhaud
Reported-by: Andres Freund
Discussion: https://postgr.es/m/20210408051735.lfbdzun5zdlax...@alap3.anarazel.de
---
 contrib/pg_stat_statements/pg_stat_statements.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index fc2677643b..dbd0d41d88 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -47,6 +47,7 @@
 #include 
 #include 
 
+#include "access/parallel.h"
 #include "catalog/pg_authid.h"
 #include "common/hashfn.h"
 #include "executor/instrument.h"
@@ -278,8 +279,9 @@ static bool pgss_save;			/* whether to save stats across shutdown */
 
 
 #define pgss_enabled(level) \
+	(!IsParallelWorker() && \
 	(pgss_track == PGSS_TRACK_ALL || \
-	(pgss_track == PGSS_TRACK_TOP && (level) == 0))
+	(pgss_track == PGSS_TRACK_TOP && (level) == 0)))
 
 #define record_gc_qtexts() \
 	do { \
-- 
2.30.1

>From 61ff6d226761fcc8f2a28fe8e313382d1d46f098 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud 
Date: Thu, 8 Apr 2021 20:05:14 +0800
Subject: [PATCH v2 2/2] Fix thinko in pg_stat_get_activity when retrieving the
 queryid.

---
 src/backend/utils/adt/pgstatfuncs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 9fa4a93162..182b15e3f2 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -917,7 +917,7 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
 			if (beentry->st_queryid == 0)
 nulls[29] = true;
 			else
-values[29] = DatumGetUInt64(beentry->st_queryid);
+values[29] = UInt64GetDatum(beentry->st_queryid);
 		}
 		else
 		{
-- 
2.30.1

Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?

[Amit Kapila]

On Thu, Apr 8, 2021 at 9:47 AM Julien Rouhaud  wrote:
>
> On Wed, Apr 07, 2021 at 02:12:11PM -0400, Bruce Momjian wrote:
> >
> > Patch applied.  I am ready to adjust this with any improvements people
> > might have.  Thank you for all the good feedback we got on this, and I
> > know many users have waited a long time for this feature.
>
> Thanks a lot Bruce and everyone!  I hope that the users who waited a long time
> for this will find everything they need.
>

@@ -1421,8 +1421,9 @@ ParallelQueryMain(dsm_segment *seg, shm_toc *toc)
/* Setting debug_query_string for individual workers */
debug_query_string = queryDesc->sourceText;

-   /* Report workers' query for monitoring purposes */
+   /* Report workers' query and queryId for monitoring purposes */
pgstat_report_activity(STATE_RUNNING, debug_query_string);
+   pgstat_report_queryid(queryDesc->plannedstmt->queryId, false);


Below lines down in ParallelQueryMain, we call ExecutorStart which
will report queryid, so do we need it here?

-- 
With Regards,
Amit Kapila.

Re: MultiXact\SLRU buffers configuration

[Thomas Munro]

On Thu, Apr 8, 2021 at 7:24 PM Andrey Borodin  wrote:
> I agree that this version of eviction seems much more effective and less 
> intrusive than RR. And it's still LRU, which is important for subsystem that 
> is called SLRU.
> shared->search_slotno is initialized implicitly with memset(). But this seems 
> like a common practice.
> Also comment above "max_search = Min(shared->num_slots, 
> MAX_REPLACEMENT_SEARCH);" does not reflect changes.
>
> Besides this patch looks good to me.

Thanks!  I chickened out of committing a buffer replacement algorithm
patch written 11 hours before the feature freeze, but I also didn't
really want to commit the GUC patch without that.  Ahh, if only we'd
latched onto the real problems here just a little sooner, but there is
always PostgreSQL 15, I heard it's going to be amazing.  Moved to next
CF.

Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?

[Julien Rouhaud]

On Thu, Apr 08, 2021 at 05:46:07PM +0530, Amit Kapila wrote:
> 
> @@ -1421,8 +1421,9 @@ ParallelQueryMain(dsm_segment *seg, shm_toc *toc)
> /* Setting debug_query_string for individual workers */
> debug_query_string = queryDesc->sourceText;
> 
> -   /* Report workers' query for monitoring purposes */
> +   /* Report workers' query and queryId for monitoring purposes */
> pgstat_report_activity(STATE_RUNNING, debug_query_string);
> +   pgstat_report_queryid(queryDesc->plannedstmt->queryId, false);
> 
> 
> Below lines down in ParallelQueryMain, we call ExecutorStart which
> will report queryid, so do we need it here?

Correct, it's not actually needed.  The overhead should be negligible but let's
get rid of it.  Updated fix patchset attached.
>From d74523dfb76e7583c27166ec10d72670654c3b7b Mon Sep 17 00:00:00 2001
From: Julien Rouhaud 
Date: Thu, 8 Apr 2021 13:59:43 +0800
Subject: [PATCH v3 1/3] Ignore parallel workers in pg_stat_statements.

Oversight in 4f0b0966c8 which exposed queryid in parallel workers.  Counters
are aggregated by the main backend process so parallel workers would report
duplicated activity, and could also report activity for the wrong entry as they
are only aware of the top level queryid.

Author: Julien Rouhaud
Reported-by: Andres Freund
Discussion: https://postgr.es/m/20210408051735.lfbdzun5zdlax...@alap3.anarazel.de
---
 contrib/pg_stat_statements/pg_stat_statements.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index fc2677643b..dbd0d41d88 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -47,6 +47,7 @@
 #include 
 #include 
 
+#include "access/parallel.h"
 #include "catalog/pg_authid.h"
 #include "common/hashfn.h"
 #include "executor/instrument.h"
@@ -278,8 +279,9 @@ static bool pgss_save;			/* whether to save stats across shutdown */
 
 
 #define pgss_enabled(level) \
+	(!IsParallelWorker() && \
 	(pgss_track == PGSS_TRACK_ALL || \
-	(pgss_track == PGSS_TRACK_TOP && (level) == 0))
+	(pgss_track == PGSS_TRACK_TOP && (level) == 0)))
 
 #define record_gc_qtexts() \
 	do { \
-- 
2.30.1

>From 61ff6d226761fcc8f2a28fe8e313382d1d46f098 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud 
Date: Thu, 8 Apr 2021 20:05:14 +0800
Subject: [PATCH v3 2/3] Fix thinko in pg_stat_get_activity when retrieving the
 queryid.

---
 src/backend/utils/adt/pgstatfuncs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 9fa4a93162..182b15e3f2 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -917,7 +917,7 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
 			if (beentry->st_queryid == 0)
 nulls[29] = true;
 			else
-values[29] = DatumGetUInt64(beentry->st_queryid);
+values[29] = UInt64GetDatum(beentry->st_queryid);
 		}
 		else
 		{
-- 
2.30.1

>From bc3b5470d12aab282e1b6f6b0b2f4afcc65b7dcf Mon Sep 17 00:00:00 2001
From: Julien Rouhaud 
Date: Thu, 8 Apr 2021 20:25:19 +0800
Subject: [PATCH v3 3/3] Remove unnecessary call to pgstat_report_queryid().

Reported-by: Amit Kapila
---
 src/backend/executor/execParallel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/executor/execParallel.c b/src/backend/executor/execParallel.c
index d104a19767..bfd6155509 100644
--- a/src/backend/executor/execParallel.c
+++ b/src/backend/executor/execParallel.c
@@ -1426,7 +1426,7 @@ ParallelQueryMain(dsm_segment *seg, shm_toc *toc)
 
 	/* Report workers' query and queryId for monitoring purposes */
 	pgstat_report_activity(STATE_RUNNING, debug_query_string);
-	pgstat_report_queryid(queryDesc->plannedstmt->queryId, false);
+	//pgstat_report_queryid(queryDesc->plannedstmt->queryId, false);
 
 	/* Attach to the dynamic shared memory area. */
 	area_space = shm_toc_lookup(toc, PARALLEL_KEY_DSA, false);
-- 
2.30.1

Re: WIP: WAL prefetch (another approach)

[Tomas Vondra]

On 4/8/21 1:46 PM, Thomas Munro wrote:
> On Thu, Apr 8, 2021 at 3:27 AM Tomas Vondra
>  wrote:
>> On 4/7/21 1:24 PM, Thomas Munro wrote:
>>> I made one other simplifying change: previously, the prefetch module
>>> would read the WAL up to the "written" LSN (so, allowing itself to
>>> read data that had been written but not yet flushed to disk by the
>>> walreceiver), though it still waited until a record's LSN was
>>> "flushed" before replaying.  That allowed prefetching to happen
>>> concurrently with the WAL flush, which was nice, but it felt a little
>>> too "special".  I decided to remove that part for now, and I plan to
>>> look into making standbys work more like primary servers, using WAL
>>> buffers, the WAL writer and optionally the standard log-before-data
>>> rule.
>>
>> Not sure, but the removal seems unnecessary. I'm worried that this will
>> significantly reduce the amount of data that we'll be able to prefetch.
>> How likely it is that we have data that is written but not flushed?
>> Let's assume the replica is lagging and network bandwidth is not the
>> bottleneck - how likely is this "has to be flushed" a limit for the
>> prefetching?
> 
> Yeah, it would have been nice to include that but it'll have to be for
> v15 due to lack of time to convince myself that it was correct.  I do
> intend to look into more concurrency of that kind for v15.  I have
> pushed these patches, updated to be disabled by default.  I will look
> into how I can run a BF animal that has it enabled during the recovery
> tests for coverage.  Thanks very much to everyone on this thread for
> all the discussion and testing so far.
> 

OK, understood. I'll rerun the benchmarks on this version, and if
there's a significant negative impact we can look into that during the
stabilization phase.

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Simplify backend terminate and wait logic in postgres_fdw test

[Bharath Rupireddy]

On Thu, Apr 8, 2021 at 5:28 PM Michael Paquier  wrote:
>
> On Thu, Apr 08, 2021 at 04:55:22PM +0530, Bharath Rupireddy wrote:
> > With the recent commit aaf0432572 which introduced a waiting/timeout
> > capability for pg_teriminate_backend function, I would like to do
> > $subject. Attaching a patch, please have a look.
>
> +-- Terminate the remote backend having the specified application_name and 
> wait
> +-- for the termination to complete. 10 seconds timeout here is chosen 
> randomly,
> +-- we will see a warning if the process doesn't go away within that time.
> +SELECT pg_terminate_backend(pid, 1) FROM pg_stat_activity
> +WHERE application_name = 'fdw_retry_check';
>
> I think that you are making the tests less stable by doing that.  A
> couple of buildfarm machines are very slow, and 10 seconds would not
> be enough.  So it seems to me that this patch is trading what is a
> stable solution for a solution that may finish by randomly bite.

Agree. Please see the attached patch, I removed a fixed waiting time.
Instead of relying on pg_stat_activity, pg_sleep and
pg_stat_clear_snapshot, now it depends on pg_terminate_backend and
pg_wait_for_backend_termination. This way we could reduce the
functions that the procedure terminate_backend_and_wait uses and also
the new functions pg_terminate_backend and
pg_wait_for_backend_termination gets a test coverage.

Thoughts?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com


v2-0001-Simplify-backend-terminate-and-wait-logic-in-post.patch
Description: Binary data

Re: TRUNCATE on foreign table

[Kohei KaiGai]

2021年4月8日(木) 18:25 Fujii Masao :
>
> On 2021/04/08 15:48, Kohei KaiGai wrote:
> > 2021年4月8日(木) 15:04 Fujii Masao :
> >>
> >> On 2021/04/08 13:43, Kohei KaiGai wrote:
> >>> In case when a local table (with no children) has same contents,
> >>> TRUNCATE command
> >>> witll remove the entire table contents.
> >>
> >> But if there are local child tables that inherit the local parent table, 
> >> and TRUNCATE ONLY  is executed, only the contents in the 
> >> parent will be truncated. I was thinking that this behavior should be 
> >> applied to the foreign table whose remote (parent) table have remote child 
> >> tables.
> >>
> >> So what we need to reach the consensus is; how far ONLY option affects. 
> >> Please imagine the case where we have
> >>
> >> (1) local parent table, also foreign table of remote parent table
> >> (2) local child table, inherits local parent table
> >> (3) remote parent table
> >> (4) remote child table, inherits remote parent table
> >>
> >> I think that we agree all (1), (2), (3) and (4) should be truncated if 
> >> local parent table (1) is specified without ONLY in TRUNCATE command. 
> >> OTOH, if ONLY is specified, we agree that at least local child table (2) 
> >> should NOT be truncated.
> >>
> > My understanding of a foreign table is a representation of external
> > data, including remote RDBMS but not only RDBMS,
> > regardless of the parent-child relationship at the local side.
> > So, once a local foreign table wraps entire tables tree (a parent and
> > relevant children) at the remote side, at least, it shall
> > be considered as a unified data chunk from the standpoint of the local side.
>
> At least for me it's not intuitive to truncate the remote table and its all 
> dependent tables even though users explicitly specify ONLY for the foreign 
> table. As far as I read the past discussion, some people was thinking the 
> same.
>
> >
> > Please assume if file_fdw could map 3 different CSV files, then
> > truncate on the foreign table may eliminate just 1 of 3 files.
> > Is it an expected / preferable behavior?
>
> I think that's up to each FDW. That is, IMO the information about whether 
> ONLY is specified or not for each table should be passed to FDW. Then FDW 
> itself should determine how to handle that information.
>
> Anyway, attached is the updated version of the patch. This is still based on 
> the latest Kazutaka-san's patch. That is, extra list for ONLY is still passed 
> to FDW. What about committing this version at first? Then we can continue the 
> discussion and change the behavior later if necessary.
>
Ok, it's fair enought for me.

I'll try to sort out my thought, then raise a follow-up discussion if necessary.

Best regards,
-- 
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei 

Re: TRUNCATE on foreign table

[Fujii Masao]

On 2021/04/08 22:02, Kohei KaiGai wrote:

Anyway, attached is the updated version of the patch. This is still based on 
the latest Kazutaka-san's patch. That is, extra list for ONLY is still passed 
to FDW. What about committing this version at first? Then we can continue the 
discussion and change the behavior later if necessary.


Pushed! Thank all involved in this development!!
For record, I attached the final patch I committed.



Ok, it's fair enought for me.

I'll try to sort out my thought, then raise a follow-up discussion if necessary.


Thanks!

The followings are the open items and discussion points that I'm thinking of.

1. Currently the extra information (TRUNCATE_REL_CONTEXT_NORMAL, 
TRUNCATE_REL_CONTEXT_ONLY or TRUNCATE_REL_CONTEXT_CASCADING) about how a 
foreign table was specified as the target to truncate in TRUNCATE command is 
collected and passed to FDW. Does this really need to be passed to FDW? Seems 
Stephen, Michael and I think that's necessary. But Kaigai-san does not. I also 
think that TRUNCATE_REL_CONTEXT_CASCADING can be removed because there seems no 
use case for that maybe.

2. Currently when the same foreign table is specified multiple times in the command, the extra 
information only for the foreign table found first is collected. For example, when "TRUNCATE 
ft, ONLY ft" is executed, TRUNCATE_REL_CONTEXT_NORMAL is collected and _ONLY is ignored 
because "ft" is found first. Is this OK? Or we should collect all, e.g., both _NORMAL and 
_ONLY should be collected in that example? I think that the current approach (i.e., collect the 
extra info about table found first if the same table is specified multiple times) is good because 
even local tables are also treated the same way. But Kaigai-san does not.

3. Currently postgres_fdw specifies ONLY clause in TRUNCATE command that it 
constructs. That is, if the foreign table is specified with ONLY, postgres_fdw 
also issues the TRUNCATE command for the corresponding remote table with ONLY 
to the remote server. Then only root table is truncated in remote server side, 
and the tables inheriting that are not truncated. Is this behavior desirable? 
Seems Michael and I think this behavior is OK. But Kaigai-san does not.

4. Tab-completion for TRUNCATE should be updated so that also foreign tables 
are displayed.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/contrib/postgres_fdw/connection.c 
b/contrib/postgres_fdw/connection.c
index 6a61d83862..82aa14a65d 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -92,7 +92,6 @@ static PGconn *connect_pg_server(ForeignServer *server, 
UserMapping *user);
 static void disconnect_pg_server(ConnCacheEntry *entry);
 static void check_conn_params(const char **keywords, const char **values, 
UserMapping *user);
 static void configure_remote_session(PGconn *conn);
-static void do_sql_command(PGconn *conn, const char *sql);
 static void begin_remote_xact(ConnCacheEntry *entry);
 static void pgfdw_xact_callback(XactEvent event, void *arg);
 static void pgfdw_subxact_callback(SubXactEvent event,
@@ -568,7 +567,7 @@ configure_remote_session(PGconn *conn)
 /*
  * Convenience subroutine to issue a non-data-returning SQL command to remote
  */
-static void
+void
 do_sql_command(PGconn *conn, const char *sql)
 {
PGresult   *res;
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 5aa3455e30..bdc4c3620d 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -56,6 +56,7 @@
 #include "utils/rel.h"
 #include "utils/syscache.h"
 #include "utils/typcache.h"
+#include "commands/tablecmds.h"
 
 /*
  * Global context for foreign_expr_walker's search of an expression tree.
@@ -2172,6 +2173,43 @@ deparseAnalyzeSql(StringInfo buf, Relation rel, List 
**retrieved_attrs)
deparseRelation(buf, rel);
 }
 
+/*
+ * Construct a simple "TRUNCATE rel" statement
+ */
+void
+deparseTruncateSql(StringInfo buf,
+  List *rels,
+  List *rels_extra,
+  DropBehavior behavior,
+  bool restart_seqs)
+{
+   ListCell   *lc1,
+  *lc2;
+
+   appendStringInfoString(buf, "TRUNCATE ");
+
+   forboth(lc1, rels, lc2, rels_extra)
+   {
+   Relationrel = lfirst(lc1);
+   int extra = lfirst_int(lc2);
+
+   if (lc1 != list_head(rels))
+   appendStringInfoString(buf, ", ");
+   if (extra & TRUNCATE_REL_CONTEXT_ONLY)
+   appendStringInfoString(buf, "ONLY ");
+
+   deparseRelation(buf, rel);
+   }
+
+   appendStringInfo(buf, " %s IDENTITY",
+restart_seqs ? "RESTART" : "CONTINUE");
+
+   if (behavior == DROP_R

Re: pg_stat_statements oddity with track = all

[Magnus Hagander]

On Thu, Apr 8, 2021 at 2:04 PM Julien Rouhaud  wrote:
>
> On Thu, Apr 08, 2021 at 10:30:53AM +0200, Magnus Hagander wrote:
> >
> > I agree. If those numbers are indeed representable, it seems like
> > better to pay that overhead than to pay the overhead of trying to
> > de-dupe it.
> >
> > Let's hope they are :)
>
> :)
>
> > Looking through ti again my feeling said the toplevel column should go
> > after the queryid and not before, but I'm not going to open up a
> > bikeshed over that.
> >
> > I've added in a comment to cover that one that you removed (if you did
> > send an updated patch as you said, then I missed it -- sorry), and
> > applied the rest.
>
> Oops, somehow I totally forgot to send the new patch, sorry :(
>
> While looking at the patch, I unfortunately just realize that I unnecessarily
> bumped the version to 1.10, as 1.9 was already new as of pg14.  Honestly I 
> have
> no idea why I used 1.10 at that time.  Version numbers are not a scarce
> resource but maybe it would be better to keep 1.10 for a future major postgres
> version?
>
> If yes, PFA a patch to merge 1.10 in 1.9.

I actually thought I looked at that, but clearly I was confused one
way or another.

I think you're right, it's cleaner to merge it into 1.9, so applied and pushed.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?

[Julien Rouhaud]

On Thu, Apr 08, 2021 at 08:27:20PM +0800, Julien Rouhaud wrote:
> On Thu, Apr 08, 2021 at 05:46:07PM +0530, Amit Kapila wrote:
> > 
> > @@ -1421,8 +1421,9 @@ ParallelQueryMain(dsm_segment *seg, shm_toc *toc)
> > /* Setting debug_query_string for individual workers */
> > debug_query_string = queryDesc->sourceText;
> > 
> > -   /* Report workers' query for monitoring purposes */
> > +   /* Report workers' query and queryId for monitoring purposes */
> > pgstat_report_activity(STATE_RUNNING, debug_query_string);
> > +   pgstat_report_queryid(queryDesc->plannedstmt->queryId, false);
> > 
> > 
> > Below lines down in ParallelQueryMain, we call ExecutorStart which
> > will report queryid, so do we need it here?
> 
> Correct, it's not actually needed.  The overhead should be negligible but 
> let's
> get rid of it.  Updated fix patchset attached.

Sorry I messed up the last commit, v4 is ok.
>From d74523dfb76e7583c27166ec10d72670654c3b7b Mon Sep 17 00:00:00 2001
From: Julien Rouhaud 
Date: Thu, 8 Apr 2021 13:59:43 +0800
Subject: [PATCH v4 1/3] Ignore parallel workers in pg_stat_statements.

Oversight in 4f0b0966c8 which exposed queryid in parallel workers.  Counters
are aggregated by the main backend process so parallel workers would report
duplicated activity, and could also report activity for the wrong entry as they
are only aware of the top level queryid.

Author: Julien Rouhaud
Reported-by: Andres Freund
Discussion: https://postgr.es/m/20210408051735.lfbdzun5zdlax...@alap3.anarazel.de
---
 contrib/pg_stat_statements/pg_stat_statements.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index fc2677643b..dbd0d41d88 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -47,6 +47,7 @@
 #include 
 #include 
 
+#include "access/parallel.h"
 #include "catalog/pg_authid.h"
 #include "common/hashfn.h"
 #include "executor/instrument.h"
@@ -278,8 +279,9 @@ static bool pgss_save;			/* whether to save stats across shutdown */
 
 
 #define pgss_enabled(level) \
+	(!IsParallelWorker() && \
 	(pgss_track == PGSS_TRACK_ALL || \
-	(pgss_track == PGSS_TRACK_TOP && (level) == 0))
+	(pgss_track == PGSS_TRACK_TOP && (level) == 0)))
 
 #define record_gc_qtexts() \
 	do { \
-- 
2.30.1

>From 61ff6d226761fcc8f2a28fe8e313382d1d46f098 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud 
Date: Thu, 8 Apr 2021 20:05:14 +0800
Subject: [PATCH v4 2/3] Fix thinko in pg_stat_get_activity when retrieving the
 queryid.

---
 src/backend/utils/adt/pgstatfuncs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 9fa4a93162..182b15e3f2 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -917,7 +917,7 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
 			if (beentry->st_queryid == 0)
 nulls[29] = true;
 			else
-values[29] = DatumGetUInt64(beentry->st_queryid);
+values[29] = UInt64GetDatum(beentry->st_queryid);
 		}
 		else
 		{
-- 
2.30.1

>From c3ab7472f1f75c9a26f1b77d748aa267e3483d1e Mon Sep 17 00:00:00 2001
From: Julien Rouhaud 
Date: Thu, 8 Apr 2021 20:25:19 +0800
Subject: [PATCH v4 3/3] Remove unnecessary call to pgstat_report_queryid().

Reported-by: Amit Kapila
---
 src/backend/executor/execParallel.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/backend/executor/execParallel.c b/src/backend/executor/execParallel.c
index d104a19767..4fca8782b2 100644
--- a/src/backend/executor/execParallel.c
+++ b/src/backend/executor/execParallel.c
@@ -1426,7 +1426,6 @@ ParallelQueryMain(dsm_segment *seg, shm_toc *toc)
 
 	/* Report workers' query and queryId for monitoring purposes */
 	pgstat_report_activity(STATE_RUNNING, debug_query_string);
-	pgstat_report_queryid(queryDesc->plannedstmt->queryId, false);
 
 	/* Attach to the dynamic shared memory area. */
 	area_space = shm_toc_lookup(toc, PARALLEL_KEY_DSA, false);
-- 
2.30.1

Re: 2019-03 CF now in progress

[David Steele]

On 3/15/21 10:24 AM, David Steele wrote:

On 3/1/21 10:30 AM, David Steele wrote:

Hackers,

The 2019-03 commitfest is now in progress. It's a big one as usual.

Needs review: 213.
Waiting on Author: 21.
Ready for Committer: 28.
Committed: 29.
Withdrawn: 3.
Total: 294.


We are now halfway through the 2021-03 commitfest, though historically 
this CF goes a bit long.


Here is the updated summary:

Needs review: 152. (-61)
Waiting on Author: 42. (+21)
Ready for Committer: 26. (-2)
Committed: 55. (+26)
Moved to next CF: 5. (+5)
Returned with Feedback: 4. (+4)
Rejected: 1. (+1)
Withdrawn: 9. (+6)
Total: 294.

In short, 42 of 262 entries open at the beginning of the CF have been 
closed (16%).


The 2021-03 CF is now over.

Here is the updated summary:

Needs review: 80 (-72)
Waiting on Author: 34 (-8)
Ready for Committer: 14 (-12)
Committed: 120 (+65)
Moved to next CF: 16 (+11)
Withdrawn: 14 (+5)
Rejected: 1
Returned with Feedback: 15 (+11)
Total: 294

Overall, 118 of 262 entries were closed during this commitfest (45%). 
That includes 91 patches committed since March 1, which is pretty 
fantastic. Thank you to everyone, especially the committers, for your 
hard work!


Today and tomorrow I'll be checking the Waiting on Author patches to 
determine which should be moved to the next CF and which should be 
returned with feedback. The rest will be moved to the next CF when this 
one is closed.


Regards,
--
-David
da...@pgmasters.net

Re: 2019-03 CF now in progress

[Matthias van de Meent]

On Thu, 8 Apr 2021 at 15:49, David Steele  wrote:
>
> On 3/15/21 10:24 AM, David Steele wrote:
> > On 3/1/21 10:30 AM, David Steele wrote:
> >> Hackers,
> >>
> >> The 2019-03 commitfest is now in progress. It's a big one as usual.
> >>
> >> Needs review: 213.
> >> Waiting on Author: 21.
> >> Ready for Committer: 28.
> >> Committed: 29.
> >> Withdrawn: 3.
> >> Total: 294.
> >
> > We are now halfway through the 2021-03 commitfest, though historically
> > this CF goes a bit long.
> >
> > Here is the updated summary:
> >
> > Needs review: 152. (-61)
> > Waiting on Author: 42. (+21)
> > Ready for Committer: 26. (-2)
> > Committed: 55. (+26)
> > Moved to next CF: 5. (+5)
> > Returned with Feedback: 4. (+4)
> > Rejected: 1. (+1)
> > Withdrawn: 9. (+6)
> > Total: 294.
> >
> > In short, 42 of 262 entries open at the beginning of the CF have been
> > closed (16%).
>
> The 2021-03 CF is now over.
>
> Here is the updated summary:
>
> Needs review: 80 (-72)
> Waiting on Author: 34 (-8)
> Ready for Committer: 14 (-12)
> Committed: 120 (+65)
> Moved to next CF: 16 (+11)
> Withdrawn: 14 (+5)
> Rejected: 1
> Returned with Feedback: 15 (+11)
> Total: 294
>
> Overall, 118 of 262 entries were closed during this commitfest (45%).
> That includes 91 patches committed since March 1, which is pretty
> fantastic. Thank you to everyone, especially the committers, for your
> hard work!

Thanks for all of your great work!

If my memory serves correctly from a statistics thread from 2020, I
believe that this a new record, at least with regards to number of
committed patches registered to one CF.

> Today and tomorrow I'll be checking the Waiting on Author patches to
> determine which should be moved to the next CF and which should be
> returned with feedback. The rest will be moved to the next CF when this
> one is closed.

I noticed that there is an old patch stuck at 'Needs review' in CF
2020-09. Could you also update the state of that patch, because I
don't think I am the right person to do that.

With regards,

Matthias van de Meent

Re: [PATCH] PREPARE TRANSACTION unexpected behavior with TEMP TABLE

[Robert Haas]

On Wed, Apr 7, 2021 at 12:19 PM Himanshu Upadhyaya
 wrote:
> Please find attached the V2 patch.

Hi,

This patch is essentially taking the position that calling
load_typcache_tupdesc before using that tupdesc for anything is
required for correctness. I'm not sure whether that's a good
architectural decision: to me, it looks like whoever wrote this code
originally - I think it was Tom - had the idea that it would be OK to
skip calling that function whenever we already have the value.
Changing that has some small performance cost, and it also just looks
kind of weird, because you don't expect a function called
load_typcache_tupdesc() to have the side effect of preventing some
kind of bad thing from happening. You just expect it to be loading
stuff. The comments in this code are not exactly stellar as things
stand, but the patch also doesn't update them in a meaningful way.
Sure, it corrects a few comments that would be flat-out wrong
otherwise, but it doesn't add any kind of explanation that would help
the next person who looks at this code understand why they shouldn't
just put back the exact same performance optimization you're proposing
to rip out.

An alternative design would be to find some new place to set
XACT_FLAGS_ACCESSEDTEMPNAMESPACE. For example, we could set a flag in
the TypeCacheEntry indicating whether this flag ought to be set when
somebody looks up the entry.

But, before we get too deeply into what the design should be, I think
we need to be clear about what problem we're trying to fix. I agree
that the behavior you demonstrate in your example looks inconsistent,
but that doesn't necessarily mean that the code is wrong. What exactly
is the code trying to prohibit here, and does this test case really
show that principle being violated? The comments in
PrepareTransaction() justify this prohibition by saying that "Having
the prepared xact hold locks on another backend's temp table seems a
bad idea --- for instance it would prevent the backend from exiting.
There are other problems too, such as how to clean up the source
backend's local buffers and ON COMMIT state if the prepared xact
includes a DROP of a temp table." But, in this case, none of that
stuff actually happens. If I run your test case without the patch, the
backend has no problem exiting, and the prepared xact holds no lock on
the temp table, and the prepared xact does not include a DROP of a
temp table. That's not to say that everything is great, because after
starting a new session and committing mytran, this happens:

rhaas=# \df longname
ERROR:  cache lookup failed for type 16432

But the patch doesn't actually prevent that from happening, because
even with the patch applied I can still recreate the same failure
using a different sequence of steps:

[ session 1 ]
rhaas=# create temp table fullname (first text, last text);
CREATE TABLE

[ session 2 ]
rhaas=# select oid::regclass from pg_class where relname = 'fullname';
oid

 pg_temp_3.fullname
(1 row)

rhaas=# begin;
BEGIN
rhaas=*# create function longname(pg_temp_3.fullname) returns text
language sql as $$select $1.first || ' ' || $1.last$$;
CREATE FUNCTION

[ session 1 ]
rhaas=# \q

[ session 2 ]
rhaas=*# commit;
COMMIT
rhaas=# \df longname
ERROR:  cache lookup failed for type 16448

To really fix this, you'd need CREATE FUNCTION to take a lock on the
containing namespace, whether permanent or temporary. You'd also need
every other CREATE statement that creates a schema-qualified object to
do the same thing. Maybe that's a good idea, but we've been reluctant
to go that far in the past due to performance consequences, and it's
not clear whether any of those problems are related to the issue that
prompted you to submit the patch. So, I'm kind of left wondering what
exactly you're trying to solve here. Can you clarify?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: SQL:2011 PERIODS vs Postgres Ranges?

[David Steele]

On 10/27/20 12:34 PM, Paul Jungwirth wrote:

On 10/27/20 7:11 AM, Ibrar Ahmed wrote:
I have spent some more time on the patch and did a lot of 
cleanup along with some fixes, compilation errors, and warnings.


Thank you for taking a look at this! I've been swamped with ordinary 
work and haven't had a chance to focus on it for a while, but I'm hoping 
to make some improvements over the coming holidays, especially based on 
feedback from my talk at PgCon. There are a handful of small specific 
things I'd like to do, and then one big thing: add support for PERIODs. 
Vik said I could include his old patch for PERIODs, so I'd like to get 
that working on the latest master, and then rebase my own work on top of 
it. Then we can accept either ranges or PERIODs in various places 
(marked by TODOs in the code).


Vik also pointed out a way to check foreign keys without using 
range_agg. He thinks it may even be more efficient. On the other hand 
it's a much more complicated SQL statement. I'd like to do a performance 
comparison to get concrete numbers, but if we did use his query, then 
this patch wouldn't depend on multiranges anymore---which seems like a 
big aid to moving it forward. Assuming multiranges gets committed, we 
can always swap in the range_agg query depending on the performance 
comparison results.


I apologize for the slow progress here, and thank you for your help!


Looks like Ibrar reopened this patch in the 2020-09 CF rather than 
moving it to a new one. Given that Paul has not had a chance to look at 
it since then I'm setting it back to RwF.


Paul, you can submit to the next CF when you are ready with a new patch.

Regards,
--
-David
da...@pgmasters.net

Re: 2019-03 CF now in progress

[David Steele]

On 4/8/21 10:12 AM, Matthias van de Meent wrote:


I noticed that there is an old patch stuck at 'Needs review' in CF
2020-09. Could you also update the state of that patch, because I
don't think I am the right person to do that.


Good catch, thanks. I have reset it to RwF.

Regards,
--
-David
da...@pgmasters.net

Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.

[David Steele]

On 3/15/21 11:11 AM, Mark Dilger wrote:



On Mar 2, 2021, at 6:08 AM, Pavel Borisov  wrote:

I completely agree that checking uniqueness requires looking at the heap, but I 
don't agree that every caller of bt_index_check on an index wants that 
particular check to be performed.  There are multiple ways in which an index 
might be corrupt, and Peter wrote the code to only check some of them by 
default, with options to expand the checks to other things.  This is why 
heapallindexed is optional.  If you don't want to pay the price of checking all 
entries in the heap against the btree, you don't have to.

I've got the idea and revised the patch accordingly. Thanks!
Pfa v4 of a patch. I've added an optional argument to allow uniqueness checks 
for the unique indexes.
Also, I added a test variant to make them work on 32-bit systems. 
Unfortunately, converting the regression test to TAP would be a pain for me. 
Hope it can be used now as a 2-variant regression test for 32 and 64 bit 
systems.

Thank you for your consideration!

--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com



Looking over v4, here are my review comments...


This patch appears to need some work and has not been updated in several 
weeks, so marking Returned with Feedback.


Please submit to the next CF when you have a new patch.

Regards,
--
-David
da...@pgmasters.net

Re: allow partial union-all and improve parallel subquery costing

[David Steele]

On 3/15/21 9:09 AM, David Steele wrote:


On 12/30/20 8:54 AM, Luc Vlaming wrote:


Created a commitfest entry assuming this is the right thing to do so 
that someone can potentially pick it up during the commitfest.


Providing an updated patch based on latest master.


Looks like you need another rebase: 
http://cfbot.cputube.org/patch_32_2787.log. Marked as Waiting for Author.


You may also want to give a more detailed description of what you have 
done here and why it improves execution plans. This may help draw some 
reviewers.


Since no new patch has been provided, marking this Returned with Feedback.

Please resubmit to the next CF when you have a new patch.

Regards,
--
-David
da...@pgmasters.net

Re: maximum columns for brin bloom indexes

[Jaime Casanova]

On Thu, Apr 08, 2021 at 12:18:36PM +0200, Tomas Vondra wrote:
> On 4/8/21 9:08 AM, Jaime Casanova wrote:
> > Hi everyone,
> > 
> > When testing brin bloom indexes I noted that we need to reduce the
> > PAGES_PER_RANGE parameter of the index to allow more columns on it.
> > 
> > Sadly, this could be a problem if you create the index before the table
> > grows, once it reaches some number of rows (i see the error as early as
> > 1000 rows) it starts error out.
> > 
> > create table t1(i int, j int);
> > 
> > -- uses default PAGES_PER_RANGE=128
> > create index on t1 using brin(i int4_bloom_ops, j int4_bloom_ops ) ;
> > 
> > insert into t1 
> > select random()*1000, random()*1000 from generate_series(1, 
> > 1000);
> > ERROR:  index row size 8968 exceeds maximum 8152 for index "t1_i_j_idx"
> > 
> > if instead you create the index with a minor PAGES_PER_RANGE it goes
> > fine, in this case it works once you reduce it to at least 116
> > 
> > create index on t1 using brin(i int4_bloom_ops, j int4_bloom_ops ) 
> > with (pages_per_range=116);
> > 
> > 
> > so, for having:
> > two int columns - PAGES_PER_RANGE should be max 116
> > three int columns - PAGES_PER_RANGE should be max 77
> > one int and one timestamp - PAGES_PER_RANGE should be max 121 
> > 
> > and so on
> > 
> 
> No, because this very much depends on the number if distinct values in
> the page page range, which determines how well the bloom filter
> compresses. You used 1000, but that's just an arbitrary value and the
> actual data might have any other value. And it's unlikely that all three
> columns will have the same number of distinct values.
>

Ok, that makes sense. Still I see a few odd things: 

"""
drop table if exists t1;
create table t1(i int, j int);
create index on t1 using brin(i int4_bloom_ops, j int4_bloom_ops ) ;

-- This one will succeed, I guess because it has less different
-- values
insert into t1
select random()*20, random()*100 from generate_series(1, 1000);

-- succeed
insert into t1
select random()*20, random()*100 from generate_series(1, 10);

-- succeed
insert into t1
select random()*200, random()*1000 from generate_series(1, 1000);

-- succeed
insert into t1
select random()*200, random()*1000 from generate_series(1, 1000);

-- succeed? This is the case it has been causing problems before
insert into t1
select random()*1000, random()*1000 from generate_series(1, 1000);
"""

Maybe this makes sense, but it looks random to me. If it makes sense
this is something we should document better. 

Let's try another combination:

"""
drop table if exists t1;
create table t1(i int, j int);
create index on t1 using brin(i int4_bloom_ops, j int4_bloom_ops ) ;

-- this fails again
insert into t1
select random()*1000, random()*1000 from generate_series(1, 1000);

-- and this starts to fail now, but this worked before
insert into t1
select random()*20, random()*100 from generate_series(1, 1000);
"""

> Of course, this also depends on the false positive rate.
> 

How the false positive rate work?

> FWIW I doubt people are using multi-column BRIN indexes very often.
> 

true. 

Another question, should we allow to create a brin multi column index
that uses different opclasses?

CREATE INDEX ON t1 USING brin (i int4_bloom_ops, j int4_minmax_ops);

-- 
Jaime Casanova
Director de Servicios Profesionales
SystemGuards - Consultores de PostgreSQL

Re: pg_stat_statements oddity with track = all

[Julien Rouhaud]

On Thu, Apr 08, 2021 at 03:18:09PM +0200, Magnus Hagander wrote:
> On Thu, Apr 8, 2021 at 2:04 PM Julien Rouhaud  wrote:
> >
> > If yes, PFA a patch to merge 1.10 in 1.9.
> 
> I actually thought I looked at that, but clearly I was confused one
> way or another.
> 
> I think you're right, it's cleaner to merge it into 1.9, so applied and 
> pushed.

Thanks Magnus!

Re: [BUG] orphaned function

[David Steele]

On 3/29/21 7:20 AM, Drouvot, Bertrand wrote:


This item is classified as a bug-fix in the commitfest, but it doesn't
sound like something we can actually back-patch.


Since it looks like a new patch is required and this probably should not 
be classified as a bug fix, marking Returned with Feedback.


Please resubmit to the next CF when you have a new patch.

Regards,
--
-David
da...@pgmasters.net

Re: maximum columns for brin bloom indexes

[Tomas Vondra]

On 4/8/21 4:49 PM, Jaime Casanova wrote:
> On Thu, Apr 08, 2021 at 12:18:36PM +0200, Tomas Vondra wrote:
>> On 4/8/21 9:08 AM, Jaime Casanova wrote:
>>> Hi everyone,
>>>
>>> When testing brin bloom indexes I noted that we need to reduce the
>>> PAGES_PER_RANGE parameter of the index to allow more columns on it.
>>>
>>> Sadly, this could be a problem if you create the index before the table
>>> grows, once it reaches some number of rows (i see the error as early as
>>> 1000 rows) it starts error out.
>>>
>>> create table t1(i int, j int);
>>> 
>>> -- uses default PAGES_PER_RANGE=128
>>> create index on t1 using brin(i int4_bloom_ops, j int4_bloom_ops ) ;
>>> 
>>> insert into t1 
>>> select random()*1000, random()*1000 from generate_series(1, 
>>> 1000);
>>> ERROR:  index row size 8968 exceeds maximum 8152 for index "t1_i_j_idx"
>>>
>>> if instead you create the index with a minor PAGES_PER_RANGE it goes
>>> fine, in this case it works once you reduce it to at least 116
>>>
>>> create index on t1 using brin(i int4_bloom_ops, j int4_bloom_ops ) 
>>> with (pages_per_range=116);
>>>
>>>
>>> so, for having:
>>> two int columns - PAGES_PER_RANGE should be max 116
>>> three int columns - PAGES_PER_RANGE should be max 77
>>> one int and one timestamp - PAGES_PER_RANGE should be max 121 
>>>
>>> and so on
>>>
>>
>> No, because this very much depends on the number if distinct values in
>> the page page range, which determines how well the bloom filter
>> compresses. You used 1000, but that's just an arbitrary value and the
>> actual data might have any other value. And it's unlikely that all three
>> columns will have the same number of distinct values.
>>
> 
> Ok, that makes sense. Still I see a few odd things: 
> 
>   """
>   drop table if exists t1;
>   create table t1(i int, j int);
>   create index on t1 using brin(i int4_bloom_ops, j int4_bloom_ops ) ;
> 
>   -- This one will succeed, I guess because it has less different
>   -- values
>   insert into t1
>   select random()*20, random()*100 from generate_series(1, 1000);
> 
>   -- succeed
>   insert into t1
>   select random()*20, random()*100 from generate_series(1, 10);
> 
>   -- succeed
>   insert into t1
>   select random()*200, random()*1000 from generate_series(1, 1000);
> 
>   -- succeed
>   insert into t1
>   select random()*200, random()*1000 from generate_series(1, 1000);
> 
>   -- succeed? This is the case it has been causing problems before
>   insert into t1
>   select random()*1000, random()*1000 from generate_series(1, 1000);
>   """
> 
> Maybe this makes sense, but it looks random to me. If it makes sense
> this is something we should document better. 
> 

Presumably it's about where exactly are the new rows added, and when we
summarize the page range.

> Let's try another combination:
> 
>   """
>   drop table if exists t1;
>   create table t1(i int, j int);
>   create index on t1 using brin(i int4_bloom_ops, j int4_bloom_ops ) ;
> 
>   -- this fails again
>   insert into t1
>   select random()*1000, random()*1000 from generate_series(1, 1000);
> 
>   -- and this starts to fail now, but this worked before
>   insert into t1
>   select random()*20, random()*100 from generate_series(1, 1000);
>   """
> 
>> Of course, this also depends on the false positive rate.
>>
> 
> How the false positive rate work?
> 

The lower the false positive rate, the more "accurate" the index is,
because fewer page ranges not containing the value will be added to the
bitmap. The bloom filter however has to be larger.

>> FWIW I doubt people are using multi-column BRIN indexes very often.
>>
> 
> true. 
> 
> Another question, should we allow to create a brin multi column index
> that uses different opclasses?
> 
> CREATE INDEX ON t1 USING brin (i int4_bloom_ops, j int4_minmax_ops);
> 

Why not? Without that you couldn't create index on (int, bigint) because
those are in principle different opclasses too. I don't see what would
this restriction give us.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Performing partition pruning using row value

[David Steele]

On 2/16/21 9:07 AM, Anastasia Lubennikova wrote:

On 21.07.2020 11:24, kato-...@fujitsu.com wrote:
So, after looking at these functions and modifying this patch, I 
would like to add this patch to the next

I updated this patch and registered for the next CF .

https://commitfest.postgresql.org/29/2654/

regards,
sho kato


Thank you for working on this improvement. I took a look at the code.

1) This piece of code is unneeded:

             switch (get_op_opfamily_strategy(opno, partopfamily))
             {
                 case BTLessStrategyNumber:
                 case BTLessEqualStrategyNumber:
                 case BTGreaterEqualStrategyNumber:
                 case BTGreaterStrategyNumber:

See the comment for RowCompareExpr, which states that "A RowCompareExpr 
node is only generated for the < <= > >= cases".


2) It's worth to add a regression test for this feature.

Other than that, the patch looks good to me.


This patch has been Waiting on Author for several months, so marking 
Returned with Feedback.


Please resubmit to the next CF when you have a new patch.

Regards,
--
-David
da...@pgmasters.net

Re: [PATCH] Add support for ON UPDATE/DELETE actions on ALTER CONSTRAINT

[David Steele]

On 3/4/21 6:28 AM, Ibrar Ahmed wrote:


This patch set no longer applies
http://cfbot.cputube.org/patch_32_1533.log 



It has been over a year since the last update to this patch and it no 
longer applies, so marking Returned with Feedback.


Please resubmit to the next CF when you have a new patch.

Regards,
--
-David
da...@pgmasters.net

Re: Extending range type operators to cope with elements

[David Steele]

On 3/4/21 6:11 AM, Ibrar Ahmed wrote:


This patch set no longer applies.

http://cfbot.cputube.org/patch_32_2747.log 



Can we get a rebase?

I am marking the patch "Waiting on Author"


This patch needs updates and a rebase and there has been no new patch 
six months, so marking Returned with Feedback.


Please resubmit to the next CF when you have a new patch.

Regards,
--
-David
da...@pgmasters.net

Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?

[Bruce Momjian]

On Thu, Apr  8, 2021 at 09:31:27PM +0800, Julien Rouhaud wrote:
> On Thu, Apr 08, 2021 at 08:27:20PM +0800, Julien Rouhaud wrote:
> > On Thu, Apr 08, 2021 at 05:46:07PM +0530, Amit Kapila wrote:
> > > 
> > > @@ -1421,8 +1421,9 @@ ParallelQueryMain(dsm_segment *seg, shm_toc *toc)
> > > /* Setting debug_query_string for individual workers */
> > > debug_query_string = queryDesc->sourceText;
> > > 
> > > -   /* Report workers' query for monitoring purposes */
> > > +   /* Report workers' query and queryId for monitoring purposes */
> > > pgstat_report_activity(STATE_RUNNING, debug_query_string);
> > > +   pgstat_report_queryid(queryDesc->plannedstmt->queryId, false);
> > > 
> > > 
> > > Below lines down in ParallelQueryMain, we call ExecutorStart which
> > > will report queryid, so do we need it here?
> > 
> > Correct, it's not actually needed.  The overhead should be negligible but 
> > let's
> > get rid of it.  Updated fix patchset attached.
> 
> Sorry I messed up the last commit, v4 is ok.

Patch applied, thanks.

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

Re: zstd compression for pg_dump

[David Steele]

On 1/3/21 9:53 PM, Justin Pryzby wrote:


I rebased so the "typedef struct compression" patch is first and zstd on top of
that (say, in case someone wants to bikeshed about which compression algorithm
to support).  And made a central struct with all the compression-specific info
to further isolate the compress-specific changes.


It has been a few months since there was a new patch and the current one 
no longer applies, so marking Returned with Feedback.


Please resubmit to the next CF when you have a new patch.

Regards,
--
-David
da...@pgmasters.net

Re: pgsql: autovacuum: handle analyze for partitioned tables

[Alvaro Herrera]

On 2021-Apr-08, Tom Lane wrote:

> Alvaro Herrera  writes:
> > autovacuum: handle analyze for partitioned tables
> 
> Looks like this has issues under EXEC_BACKEND:
> 
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2021-04-08%2005%3A50%3A08

Hmm, I couldn't reproduce this under EXEC_BACKEND or otherwise, but I
think this is unrelated to that, but rather a race condition.

The backtrace saved by buildfarm is:

Program terminated with signal SIGSEGV, Segmentation fault.
#0  relation_needs_vacanalyze (relid=relid@entry=43057, 
relopts=relopts@entry=0x0, classForm=classForm@entry=0x7e000501eef0, 
tabentry=0x5611ec71b030, 
effective_multixact_freeze_max_age=effective_multixact_freeze_max_age@entry=4,
 dovacuum=dovacuum@entry=0x7ffd78cc4ee0, doanalyze=0x7ffd78cc4ee1, 
wraparound=0x7ffd78cc4ee2) at 
/mnt/resource/andres/bf/culicidae/HEAD/pgsql.build/../pgsql/src/backend/postmaster/autovacuum.c:3237
3237childclass = (Form_pg_class) 
GETSTRUCT(childtuple);
#0  relation_needs_vacanalyze (relid=relid@entry=43057, 
relopts=relopts@entry=0x0, classForm=classForm@entry=0x7e000501eef0, 
tabentry=0x5611ec71b030, 
effective_multixact_freeze_max_age=effective_multixact_freeze_max_age@entry=4,
 dovacuum=dovacuum@entry=0x7ffd78cc4ee0, doanalyze=0x7ffd78cc4ee1, 
wraparound=0x7ffd78cc4ee2) at 
/mnt/resource/andres/bf/culicidae/HEAD/pgsql.build/../pgsql/src/backend/postmaster/autovacuum.c:3237
#1  0x5611eb09fc91 in do_autovacuum () at 
/mnt/resource/andres/bf/culicidae/HEAD/pgsql.build/../pgsql/src/backend/postmaster/autovacuum.c:2168
#2  0x5611eb0a0f8b in AutoVacWorkerMain (argc=argc@entry=1, 
argv=argv@entry=0x5611ec61f1e0) at 
/mnt/resource/andres/bf/culicidae/HEAD/pgsql.build/../pgsql/src/backend/postmaster/autovacuum.c:1715

the code in question is:

children = find_all_inheritors(relid, AccessShareLock, 
NULL);

foreach(lc, children)
{
Oid childOID = 
lfirst_oid(lc);
HeapTuple   childtuple;
Form_pg_class childclass;

childtuple = SearchSysCache1(RELOID, 
ObjectIdGetDatum(childOID));
childclass = (Form_pg_class) 
GETSTRUCT(childtuple);

Evidently SearchSysCache must be returning NULL, but how come that
happens, when we have acquired lock on the rel during
find_all_inheritors?

I would suggest that we do not take lock here at all, and just skip the
rel if SearchSysCache returns empty, as in the attached.  Still, I am
baffled about this crash.

-- 
Álvaro Herrera   Valdivia, Chile
"Oh, great altar of passive entertainment, bestow upon me thy discordant images
at such speed as to render linear thought impossible" (Calvin a la TV)
>From 2bb3e54862c37ee2a20fed21513a3df309381919 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Thu, 8 Apr 2021 11:10:44 -0400
Subject: [PATCH] Fix race condition in relation_needs_vacanalyze

---
 src/backend/postmaster/autovacuum.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index aef9ac4dd2..96073d4597 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -3223,18 +3223,23 @@ relation_needs_vacanalyze(Oid relid,
 			ListCell   *lc;
 
 			reltuples = 0;
 
-			/* Find all members of inheritance set taking AccessShareLock */
-			children = find_all_inheritors(relid, AccessShareLock, NULL);
+			/*
+			 * Find all members of inheritance set.  Beware that they may
+			 * disappear from under us, since we don't acquire any locks.
+			 */
+			children = find_all_inheritors(relid, NoLock, NULL);
 
 			foreach(lc, children)
 			{
 Oid			childOID = lfirst_oid(lc);
 HeapTuple	childtuple;
 Form_pg_class childclass;
 
 childtuple = SearchSysCache1(RELOID, ObjectIdGetDatum(childOID));
+if (childtuple == NULL)
+	continue;
 childclass = (Form_pg_class) GETSTRUCT(childtuple);
 
 /* Skip a partitioned table and foreign partitions */
 if (RELKIND_HAS_STORAGE(childclass->relkind))
-- 
2.20.1

Re: VACUUM (DISABLE_PAGE_SKIPPING on)

[David Steele]

On 3/17/21 4:50 PM, Simon Riggs wrote:

On Fri, 12 Mar 2021 at 22:16, Tomas Vondra
 wrote:


On 1/28/21 2:33 PM, Simon Riggs wrote:

On Thu, 28 Jan 2021 at 12:53, Masahiko Sawada  wrote:


This entry has been "Waiting on Author" status and the patch has not
been updated since Nov 30. Are you still planning to work on this?


Yes, new patch version tomorrow. Thanks for the nudge and the review.



So, is it tomorrow already? ;-)


Been a long day. ;-)


It has been five months since this patch was updated, so marking 
Returned with Feedback.


Please resubmit to the next CF when you have a new patch.

Regards,
--
-David
da...@pgmasters.net

Re: TRUNCATE on foreign table

[Kazutaka Onishi]

Fujii-san,

> >> Anyway, attached is the updated version of the patch. This is still based 
> >> on the latest Kazutaka-san's patch. That is, extra list for ONLY is still 
> >> passed to FDW. What about committing this version at first? Then we can 
> >> continue the discussion and change the behavior later if necessary.
> Pushed! Thank all involved in this development!!
> For record, I attached the final patch I committed.

Thank you for revising the v16 patch to v18 and pushing it.
Cool!

2021年4月8日(木) 22:14 Fujii Masao :
>
>
>
> On 2021/04/08 22:02, Kohei KaiGai wrote:
> >> Anyway, attached is the updated version of the patch. This is still based 
> >> on the latest Kazutaka-san's patch. That is, extra list for ONLY is still 
> >> passed to FDW. What about committing this version at first? Then we can 
> >> continue the discussion and change the behavior later if necessary.
>
> Pushed! Thank all involved in this development!!
> For record, I attached the final patch I committed.
>
>
> > Ok, it's fair enought for me.
> >
> > I'll try to sort out my thought, then raise a follow-up discussion if 
> > necessary.
>
> Thanks!
>
> The followings are the open items and discussion points that I'm thinking of.
>
> 1. Currently the extra information (TRUNCATE_REL_CONTEXT_NORMAL, 
> TRUNCATE_REL_CONTEXT_ONLY or TRUNCATE_REL_CONTEXT_CASCADING) about how a 
> foreign table was specified as the target to truncate in TRUNCATE command is 
> collected and passed to FDW. Does this really need to be passed to FDW? Seems 
> Stephen, Michael and I think that's necessary. But Kaigai-san does not. I 
> also think that TRUNCATE_REL_CONTEXT_CASCADING can be removed because there 
> seems no use case for that maybe.
>
> 2. Currently when the same foreign table is specified multiple times in the 
> command, the extra information only for the foreign table found first is 
> collected. For example, when "TRUNCATE ft, ONLY ft" is executed, 
> TRUNCATE_REL_CONTEXT_NORMAL is collected and _ONLY is ignored because "ft" is 
> found first. Is this OK? Or we should collect all, e.g., both _NORMAL and 
> _ONLY should be collected in that example? I think that the current approach 
> (i.e., collect the extra info about table found first if the same table is 
> specified multiple times) is good because even local tables are also treated 
> the same way. But Kaigai-san does not.
>
> 3. Currently postgres_fdw specifies ONLY clause in TRUNCATE command that it 
> constructs. That is, if the foreign table is specified with ONLY, 
> postgres_fdw also issues the TRUNCATE command for the corresponding remote 
> table with ONLY to the remote server. Then only root table is truncated in 
> remote server side, and the tables inheriting that are not truncated. Is this 
> behavior desirable? Seems Michael and I think this behavior is OK. But 
> Kaigai-san does not.
>
> 4. Tab-completion for TRUNCATE should be updated so that also foreign tables 
> are displayed.
>
> Regards,
>
> --
> Fujii Masao
> Advanced Computing Technology Center
> Research and Development Headquarters
> NTT DATA CORPORATION

Re: Autovacuum on partitioned table (autoanalyze)

[Alvaro Herrera]

On 2021-Apr-08, Tomas Vondra wrote:

> On 4/8/21 5:22 AM, Alvaro Herrera wrote:

> > However, I just noticed there is a huge problem, which is that the new
> > code in relation_needs_vacanalyze() is doing find_all_inheritors(), and
> > we don't necessarily have a snapshot that lets us do that.  While adding
> > a snapshot acquisition at that spot is a very easy fix, I hesitate to
> > fix it that way, because the whole idea there seems quite wasteful: we
> > have to look up, open and lock every single partition, on every single
> > autovacuum iteration through the database.  That seems bad.  I'm
> > inclined to think that a better idea may be to store reltuples for the
> > partitioned table in pg_class.reltuples, instead of having to add up the
> > reltuples of each partition.  I haven't checked if this is likely to
> > break anything.
> 
> How would that value get updated, for the parent?

Same as for any other relation: ANALYZE would set it, after it's done
scanning the table.  We would to make sure that nothing resets it to
empty, though, and that it doesn't cause issues elsewhere.  (The patch I
sent contains the minimal change to make it work, but of course that's
missing having other pieces of code maintain it.)

> > (Also, a minor buglet: if we do ANALYZE (col1), then ANALYZE (col2) a
> > partition, then we repeatedly propagate the counts to the parent table,
> > so we would cause the parent to be analyzed more times than it should.
> > Sounds like we should not send the ancestor list when a column list is
> > given to manual analyze.  I haven't verified this, however.)
> 
> Are you sure? I haven't tried, but shouldn't this be prevented by only
> sending the delta between the current and last reported value?

I did try, and yes it behaves as you say.

-- 
Álvaro Herrera   Valdivia, Chile
Bob [Floyd] used to say that he was planning to get a Ph.D. by the "green
stamp method," namely by saving envelopes addressed to him as 'Dr. Floyd'.
After collecting 500 such letters, he mused, a university somewhere in
Arizona would probably grant him a degree.  (Don Knuth)

Re: [HACKERS] Custom compression methods

[David Steele]

On 3/30/21 10:30 AM, Tom Lane wrote:

Robert Haas  writes:

On Wed, Mar 24, 2021 at 2:15 PM Tom Lane  wrote:

But let's ignore the case of pg_upgrade and just consider a dump/restore.
I'd still say that unless you give --no-toast-compression then I would
expect the dump/restore to preserve the tables' old compression behavior.
Robert's argument that the pre-v14 database had no particular compression
behavior seems nonsensical to me.  We know exactly which compression
behavior it has.



I said that it didn't have a state, not that it didn't have a
behavior. That's not exactly the same thing. But I don't want to argue
about it, either. It's a judgement call what's best here, and I don't
pretend to have all the answers. If you're sure you've got it right
... great!


I've not heard any other comments about this, but I'm pretty sure that
preserving a table's old toast behavior is in line with what we'd normally
expect pg_dump to do --- especially in light of the fact that we did not
provide any --preserve-toast-compression switch to tell it to do so.
So I'm going to go change it.


It looks like this CF entry should have been marked as committed so I 
did that.


Regards,
--
-David
da...@pgmasters.net

Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?

[Bruce Momjian]

On Wed, Apr  7, 2021 at 11:27:04PM -0400, Álvaro Herrera wrote:
> On 2021-Apr-07, Bruce Momjian wrote:
> 
> > On Thu, Apr  8, 2021 at 10:38:08AM +0800, Julien Rouhaud wrote:
> 
> > > Thanks!  And I agree with using query_id in the new field names while 
> > > keeping
> > > queryid for pg_stat_statements to avoid unnecessary query breakage.
> > 
> > I think we need more feedback from the group.  Do people agree with the
> > idea above?  The question is what to call:
> > 
> > GUC compute_queryid
> > pg_stat_activity.queryid
> > pg_stat_statements.queryid
> > 
> > using "queryid" or "query_id", and do they have to match?
> 
> Seems a matter of personal preference.  Mine is to have the underscore
> everywhere in backend code (where this is new), and let it without the
> underscore in pg_stat_statements to avoid breaking existing code.  Seems
> to match what Julien is saying.

OK, let's get some details.  First, pg_stat_statements.queryid already
exists (no underscore), and I don't think anyone wants to change that. 

pg_stat_activity.queryid is new, but I can imagine cases where you would
join pg_stat_activity to pg_stat_statements to get an estimate of how
long the query will take --- having one using an underscore and another
one not seems odd.  Also, looking at the existing pg_stat_activity
columns, those don't use underscores before the "id" unless there is a
modifier before the "id", e.g. "pid", "xid":

SELECT  attname
FROMpg_namespace JOIN pg_class ON (pg_namespace.oid = relnamespace)
JOIN pg_attribute ON (pg_class.oid = pg_attribute.attrelid)
WHERE   nspname = 'pg_catalog' AND
relname = 'pg_stat_activity' AND
attname ~ 'id$';
   attname
-
 backend_xid
 datid
 leader_pid
 pid
 queryid
 usesysid

We don't have a modifier before queryid.

If people like query_id, and I do too, I am thinking we just keep
query_id as the GUC (compute_query_id), and just accept that the GUC and
SQL levels will not match.  This is exactly what we have now.  I brought
it up to be sure this is what we want,

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

Re: VACUUM (DISABLE_PAGE_SKIPPING on)

[Simon Riggs]

On Thu, 8 Apr 2021 at 16:23, David Steele  wrote:
>
> On 3/17/21 4:50 PM, Simon Riggs wrote:
> > On Fri, 12 Mar 2021 at 22:16, Tomas Vondra
> >  wrote:
> >>
> >> On 1/28/21 2:33 PM, Simon Riggs wrote:
> >>> On Thu, 28 Jan 2021 at 12:53, Masahiko Sawada  
> >>> wrote:
> >>>
>  This entry has been "Waiting on Author" status and the patch has not
>  been updated since Nov 30. Are you still planning to work on this?
> >>>
> >>> Yes, new patch version tomorrow. Thanks for the nudge and the review.
> >>>
> >>
> >> So, is it tomorrow already? ;-)
> >
> > Been a long day. ;-)
>
> It has been five months since this patch was updated, so marking
> Returned with Feedback.
>
> Please resubmit to the next CF when you have a new patch.

There are 2 separate patch-sets on this thread, with separate CF entries.

One has requested changes that have not been made. I presume this one
has been RwF'd.

What is happening about the other patch?

-- 
Simon Riggshttp://www.EnterpriseDB.com/

Re: Autovacuum on partitioned table (autoanalyze)

[Tomas Vondra]

On 4/8/21 5:27 PM, Alvaro Herrera wrote:
> On 2021-Apr-08, Tomas Vondra wrote:
> 
>> On 4/8/21 5:22 AM, Alvaro Herrera wrote:
> 
>>> However, I just noticed there is a huge problem, which is that the new
>>> code in relation_needs_vacanalyze() is doing find_all_inheritors(), and
>>> we don't necessarily have a snapshot that lets us do that.  While adding
>>> a snapshot acquisition at that spot is a very easy fix, I hesitate to
>>> fix it that way, because the whole idea there seems quite wasteful: we
>>> have to look up, open and lock every single partition, on every single
>>> autovacuum iteration through the database.  That seems bad.  I'm
>>> inclined to think that a better idea may be to store reltuples for the
>>> partitioned table in pg_class.reltuples, instead of having to add up the
>>> reltuples of each partition.  I haven't checked if this is likely to
>>> break anything.
>>
>> How would that value get updated, for the parent?
> 
> Same as for any other relation: ANALYZE would set it, after it's done
> scanning the table.  We would to make sure that nothing resets it to
> empty, though, and that it doesn't cause issues elsewhere.  (The patch I
> sent contains the minimal change to make it work, but of course that's
> missing having other pieces of code maintain it.)
> 

So ANALYZE would inspect the child relations, sum the reltuples and set
it for the parent? IMO that'd be problematic because it'd mean we're
comparing the current number of changes with reltuples value which may
be arbitrarily stale (if we haven't analyzed the parent for a while).

That's essentially the issue I described when explaining why I think the
code needs to propagate the changes, reread the stats and then evaluate
which relations need vacuuming. It's similar to the issue of comparing
old changes_since_analyze vs. current reltuples, which is why the code
is rereading the stats before checking the thresholds. This time it's
the opposite direction - the reltuples might be stale.

FWIW I think the current refresh logic is not quite correct, because
autovac_refresh_stats does some throttling (STATS_READ_DELAY). It
probably needs a "force" parameter to ensure it actually reads the
current stats in this one case.

>>> (Also, a minor buglet: if we do ANALYZE (col1), then ANALYZE (col2) a
>>> partition, then we repeatedly propagate the counts to the parent table,
>>> so we would cause the parent to be analyzed more times than it should.
>>> Sounds like we should not send the ancestor list when a column list is
>>> given to manual analyze.  I haven't verified this, however.)
>>
>> Are you sure? I haven't tried, but shouldn't this be prevented by only
>> sending the delta between the current and last reported value?
> 
> I did try, and yes it behaves as you say.
> 

OK, good.

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: VACUUM (DISABLE_PAGE_SKIPPING on)

[David Steele]

On 4/8/21 11:41 AM, Simon Riggs wrote:

On Thu, 8 Apr 2021 at 16:23, David Steele  wrote:


On 3/17/21 4:50 PM, Simon Riggs wrote:

On Fri, 12 Mar 2021 at 22:16, Tomas Vondra
 wrote:


On 1/28/21 2:33 PM, Simon Riggs wrote:

On Thu, 28 Jan 2021 at 12:53, Masahiko Sawada  wrote:


This entry has been "Waiting on Author" status and the patch has not
been updated since Nov 30. Are you still planning to work on this?


Yes, new patch version tomorrow. Thanks for the nudge and the review.



So, is it tomorrow already? ;-)


Been a long day. ;-)


It has been five months since this patch was updated, so marking
Returned with Feedback.

Please resubmit to the next CF when you have a new patch.


There are 2 separate patch-sets on this thread, with separate CF entries.

One has requested changes that have not been made. I presume this one
has been RwF'd.

What is happening about the other patch?


Hmmm, well https://commitfest.postgresql.org/32/2908 and 
https://commitfest.postgresql.org/32/2909 are both linked to the same 
thread with the same patch, so I thought it was a duplicate.


It's not clear to me which patch is which, so perhaps move one CF entry 
to next CF and clarify which patch is current?


Regards,
--
-David
da...@pgmasters.net

Re: Autovacuum on partitioned table (autoanalyze)

[Alvaro Herrera]

On 2021-Apr-08, Tomas Vondra wrote:

> On 4/8/21 5:27 PM, Alvaro Herrera wrote:
>
> > Same as for any other relation: ANALYZE would set it, after it's done
> > scanning the table.  We would to make sure that nothing resets it to
> > empty, though, and that it doesn't cause issues elsewhere.  (The patch I
> > sent contains the minimal change to make it work, but of course that's
> > missing having other pieces of code maintain it.)
> 
> So ANALYZE would inspect the child relations, sum the reltuples and set
> it for the parent? IMO that'd be problematic because it'd mean we're
> comparing the current number of changes with reltuples value which may
> be arbitrarily stale (if we haven't analyzed the parent for a while).

What?  Not at all.  reltuples would be set by ANALYZE on one run, and
then the value is available for the future autovacuum run.  That's how
it works for regular tables too, so I'm not sure what you problem have
with that.  The (possibly stale) reltuples value is multiplied by the
scale factor, and added to the analyze_threshold value, and that's
compared with the current changes_since_analyze to determine whether to
analyze or not.

> That's essentially the issue I described when explaining why I think the
> code needs to propagate the changes, reread the stats and then evaluate
> which relations need vacuuming. It's similar to the issue of comparing
> old changes_since_analyze vs. current reltuples, which is why the code
> is rereading the stats before checking the thresholds. This time it's
> the opposite direction - the reltuples might be stale.

Well, I don't think the issue is the same.  reltuples is always stale,
even for regular tables, because that's just how it works.
changes_since_analyze is not stale for regular tables, and that's why it
makes sense to propagate it from partitions to ancestors prior to
checking the analyze condition.

> FWIW I think the current refresh logic is not quite correct, because
> autovac_refresh_stats does some throttling (STATS_READ_DELAY). It
> probably needs a "force" parameter to ensure it actually reads the
> current stats in this one case.

Hmm ... good catch, but actually that throttling only applies to the
launcher.  do_autovacuum runs in the worker, so there's no throttling.

-- 
Álvaro Herrera39°49'30"S 73°17'W

Re: SQL-standard function body

[Tom Lane]

Julien Rouhaud  writes:
> On Thu, Apr 08, 2021 at 02:58:02AM -0400, Tom Lane wrote:
>> No, because if that were the explanation then we'd be getting no
>> buildfarm coverage at all for for pg_stat_statements.  Which aside
>> from being awful contradicts the results at coverage.postgresql.org.

> Is there any chance that coverage.postgresql.org isn't backed by the buildfarm
> client but a plain make check-world or something like that?

Hmm, I think you're right.  Poking around in the log files from one
of my own buildfarm animals, there's no evidence that pg_stat_statements
is being tested at all.  Needless to say, that's just horrid :-(

I see that contrib/test_decoding also sets NO_INSTALLCHECK = 1,
and the reason it gets tested is that the buildfarm script has
a special module for that.  I guess we need to clone that module,
or maybe better, find a way to generalize it.

There are also some src/test/modules modules that set NO_INSTALLCHECK,
but apparently those do have coverage (modules-check is the step that
runs their SQL tests, and then the TAP tests if any get broken out
as separate buildfarm steps).

regards, tom lane

Re: VACUUM (DISABLE_PAGE_SKIPPING on)

[Simon Riggs]

On Thu, 8 Apr 2021 at 16:58, David Steele  wrote:

> >> It has been five months since this patch was updated, so marking
> >> Returned with Feedback.
> >>
> >> Please resubmit to the next CF when you have a new patch.
> >
> > There are 2 separate patch-sets on this thread, with separate CF entries.
> >
> > One has requested changes that have not been made. I presume this one
> > has been RwF'd.
> >
> > What is happening about the other patch?
>
> Hmmm, well https://commitfest.postgresql.org/32/2908 and
> https://commitfest.postgresql.org/32/2909 are both linked to the same
> thread with the same patch, so I thought it was a duplicate.

Nobody had mentioned any confusion. Multiple patches on one thread is common.

> It's not clear to me which patch is which, so perhaps move one CF entry
> to next CF and clarify which patch is current?

Entry: Maximize page freezing
has this patch, perfectly fine, awaiting review from Alvaro since 29 Nov.
one_freeze_then_max_freeze.v7.patch

Other patch is awaiting changes.

-- 
Simon Riggshttp://www.EnterpriseDB.com/

Re: psql - add SHOW_ALL_RESULTS option

[Julien Rouhaud]

On Thu, Apr 08, 2021 at 01:32:12AM +, shiy.f...@fujitsu.com wrote:
> > Attached a patch which attempts to fix this by moving the cancellation
> > cancelling request after processing results.
> 
> Thank you for your fixing. I tested and the problem has been solved after 
> applying your patch.

Thanks for the patch Fabien.  I've hit this issue multiple time and this is
indeed unwelcome.  Should we add it as an open item?

Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?

[Julien Rouhaud]

On Thu, Apr 08, 2021 at 11:34:25AM -0400, Bruce Momjian wrote:
> 
> OK, let's get some details.  First, pg_stat_statements.queryid already
> exists (no underscore), and I don't think anyone wants to change that. 
> 
> pg_stat_activity.queryid is new, but I can imagine cases where you would
> join pg_stat_activity to pg_stat_statements to get an estimate of how
> long the query will take --- having one using an underscore and another
> one not seems odd.

Indeed, and also being able to join with a USING clause rather than an ON could
also save some keystrokes.  But unfortunately, we already have (userid, dbid)
on pg_stat_statements side vs (usesysid, datid) on pg_stat_activity side, so
this unfortunately won't fix all the oddities.

Re: doc review for v14

[Justin Pryzby]

Another round of doc review, not yet including all of yesterday's commits.

29c8d614c3 duplicate words
diff --git a/src/include/lib/sort_template.h b/src/include/lib/sort_template.h
index 771c789ced..24d6d0006c 100644
--- a/src/include/lib/sort_template.h
+++ b/src/include/lib/sort_template.h
@@ -241,7 +241,7 @@ ST_SCOPE void ST_SORT(ST_ELEMENT_TYPE *first, size_t n
 
 /*
  * Find the median of three values.  Currently, performance seems to be best
- * if the the comparator is inlined here, but the med3 function is not inlined
+ * if the comparator is inlined here, but the med3 function is not inlined
  * in the qsort function.
  */
 static pg_noinline ST_ELEMENT_TYPE *
e7c370c7c5 pg_amcheck: remove Double semi-colon
diff --git a/src/bin/pg_amcheck/t/004_verify_heapam.pl 
b/src/bin/pg_amcheck/t/004_verify_heapam.pl
index 36607596b1..2171d236a7 100644
--- a/src/bin/pg_amcheck/t/004_verify_heapam.pl
+++ b/src/bin/pg_amcheck/t/004_verify_heapam.pl
@@ -175,7 +175,7 @@ sub write_tuple
seek($fh, $offset, 0)
or BAIL_OUT("seek failed: $!");
defined(syswrite($fh, $buffer, HEAPTUPLE_PACK_LENGTH))
-   or BAIL_OUT("syswrite failed: $!");;
+   or BAIL_OUT("syswrite failed: $!");
return;
 }
 
b745e9e60e a statistics objects
diff --git a/src/backend/statistics/extended_stats.c 
b/src/backend/statistics/extended_stats.c
index 463d44a68a..4674168ff8 100644
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -254,7 +254,7 @@ BuildRelationExtStatistics(Relation onerel, double 
totalrows,
  * that would require additional columns.
  *
  * See statext_compute_stattarget for details about how we compute statistics
- * target for a statistics objects (from the object target, attribute targets
+ * target for a statistics object (from the object target, attribute targets
  * and default statistics target).
  */
 int
e7d5c5d9dc guc.h: remove mention of "doit"
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 1892c7927b..1126b34798 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -90,8 +90,7 @@ typedef enum
  * dividing line between "interactive" and "non-interactive" sources for
  * error reporting purposes.
  *
- * PGC_S_TEST is used when testing values to be used later ("doit" will always
- * be false, so this never gets stored as the actual source of any value).
+ * PGC_S_TEST is used when testing values to be used later.
  * For example, ALTER DATABASE/ROLE tests proposed per-database or per-user
  * defaults this way, and CREATE FUNCTION tests proposed function SET clauses
  * this way.  This is an interactive case, but it needs its own source value
ad5f9a2023 Caller
diff --git a/src/backend/utils/adt/jsonfuncs.c 
b/src/backend/utils/adt/jsonfuncs.c
index 9961d27df4..09fcff6729 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -1651,7 +1651,7 @@ push_null_elements(JsonbParseState **ps, int num)
  * this path. E.g. the path [a][0][b] with the new value 1 will produce the
  * structure {a: [{b: 1}]}.
  *
- * Called is responsible to make sure such path does not exist yet.
+ * Caller is responsible to make sure such path does not exist yet.
  */
 static void
 push_path(JsonbParseState **st, int level, Datum *path_elems,
@@ -4887,7 +4887,7 @@ IteratorConcat(JsonbIterator **it1, JsonbIterator **it2,
  * than just one last element, this flag will instruct to create the whole
  * chain of corresponding objects and insert the value.
  *
- * JB_PATH_CONSISTENT_POSITION for an array indicates that the called wants to
+ * JB_PATH_CONSISTENT_POSITION for an array indicates that the caller wants to
  * keep values with fixed indices. Indices for existing elements could be
  * changed (shifted forward) in case if the array is prepended with a new value
  * and a negative index out of the range, so this behavior will be prevented
9acedbd4af as
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 20e7d57d41..40a54ad0bd 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -410,7 +410,7 @@ CopyMultiInsertBufferCleanup(CopyMultiInsertInfo *miinfo,
  * Once flushed we also trim the tracked buffers list down to size by removing
  * the buffers created earliest first.
  *
- * Callers should pass 'curr_rri' is the ResultRelInfo that's currently being
+ * Callers should pass 'curr_rri' as the ResultRelInfo that's currently being
  * used.  When cleaning up old buffers we'll never remove the one for
  * 'curr_rri'.
  */
9f78de5042 exist
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 5bdaceefd5..182a133033 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -617,7 +617,7 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
 *
 * We assume that VACUUM hasn't set pg_class.reltuples already, even
 * during a VACUU

Re: VACUUM (DISABLE_PAGE_SKIPPING on)

[David Steele]

On 4/8/21 12:29 PM, Simon Riggs wrote:

On Thu, 8 Apr 2021 at 16:58, David Steele  wrote:


It has been five months since this patch was updated, so marking
Returned with Feedback.

Please resubmit to the next CF when you have a new patch.


There are 2 separate patch-sets on this thread, with separate CF entries.

One has requested changes that have not been made. I presume this one
has been RwF'd.

What is happening about the other patch?


Hmmm, well https://commitfest.postgresql.org/32/2908 and
https://commitfest.postgresql.org/32/2909 are both linked to the same
thread with the same patch, so I thought it was a duplicate.


Nobody had mentioned any confusion. Multiple patches on one thread is common.


Yes, but these days they generally get updated in the same reply so the 
cfbot can test them all. In your case only the latest patch was being 
tested and it was not applying cleanly.



It's not clear to me which patch is which, so perhaps move one CF entry
to next CF and clarify which patch is current?


Entry: Maximize page freezing
has this patch, perfectly fine, awaiting review from Alvaro since 29 Nov.
one_freeze_then_max_freeze.v7.patch


That CF entry was marked Waiting for Author on 3/11, so I thought it was 
for this patch. In fact, both CF entries were Waiting for Author for 
some time.


Moved to the next CF in Waiting for Author state.

Regards,
--
-David
da...@pgmasters.net

Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?

[Bruce Momjian]

On Fri, Apr  9, 2021 at 12:38:29AM +0800, Julien Rouhaud wrote:
> On Thu, Apr 08, 2021 at 11:34:25AM -0400, Bruce Momjian wrote:
> > 
> > OK, let's get some details.  First, pg_stat_statements.queryid already
> > exists (no underscore), and I don't think anyone wants to change that. 
> > 
> > pg_stat_activity.queryid is new, but I can imagine cases where you would
> > join pg_stat_activity to pg_stat_statements to get an estimate of how
> > long the query will take --- having one using an underscore and another
> > one not seems odd.
> 
> Indeed, and also being able to join with a USING clause rather than an ON 
> could
> also save some keystrokes.  But unfortunately, we already have (userid, dbid)
> on pg_stat_statements side vs (usesysid, datid) on pg_stat_activity side, so
> this unfortunately won't fix all the oddities.

Wow, good point.  Shame they don't match.

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

Re: VACUUM (DISABLE_PAGE_SKIPPING on)

[Simon Riggs]

On Thu, 8 Apr 2021 at 17:44, David Steele  wrote:
>
> On 4/8/21 12:29 PM, Simon Riggs wrote:
> > On Thu, 8 Apr 2021 at 16:58, David Steele  wrote:
> >
>  It has been five months since this patch was updated, so marking
>  Returned with Feedback.
> 
>  Please resubmit to the next CF when you have a new patch.
> >>>
> >>> There are 2 separate patch-sets on this thread, with separate CF entries.
> >>>
> >>> One has requested changes that have not been made. I presume this one
> >>> has been RwF'd.
> >>>
> >>> What is happening about the other patch?
> >>
> >> Hmmm, well https://commitfest.postgresql.org/32/2908 and
> >> https://commitfest.postgresql.org/32/2909 are both linked to the same
> >> thread with the same patch, so I thought it was a duplicate.
> >
> > Nobody had mentioned any confusion. Multiple patches on one thread is 
> > common.
>
> Yes, but these days they generally get updated in the same reply so the
> cfbot can test them all. In your case only the latest patch was being
> tested and it was not applying cleanly.
>
> >> It's not clear to me which patch is which, so perhaps move one CF entry
> >> to next CF and clarify which patch is current?
> >
> > Entry: Maximize page freezing
> > has this patch, perfectly fine, awaiting review from Alvaro since 29 Nov.
> > one_freeze_then_max_freeze.v7.patch
>
> That CF entry was marked Waiting for Author on 3/11, so I thought it was
> for this patch. In fact, both CF entries were Waiting for Author for
> some time.

That was done by someone that hadn't mentioned it to me, or the list.

It is not Waiting on Author.

> Moved to the next CF in Waiting for Author state.

That is clearly an incorrect state.

-- 
Simon Riggshttp://www.EnterpriseDB.com/

Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?

[Alvaro Herrera]

On 2021-Apr-08, Bruce Momjian wrote:

> pg_stat_activity.queryid is new, but I can imagine cases where you would
> join pg_stat_activity to pg_stat_statements to get an estimate of how
> long the query will take --- having one using an underscore and another
> one not seems odd.

OK.  So far, you have one vote for queryid (your own) and two votes for
query_id (mine and Julien's).  And even yourself were hesitating about
it earlier in the thread.

-- 
Álvaro Herrera   Valdivia, Chile

Re: Binary search in ScalarArrayOpExpr for OR'd constant arrays

[James Coleman]

On Thu, Apr 8, 2021 at 8:01 AM David Rowley  wrote:
>
> On Thu, 8 Apr 2021 at 22:54, David Rowley  wrote:
> >
> > I think the changes in the patch are fairly isolated and the test
> > coverage is now pretty good.  I'm planning on looking at the patch
> > again now and will consider pushing it for PG14.
>
> I push this with some minor cleanup from the v6 patch I posted earlier.
>
> David

Thank you!

I assume proper procedure for the CF entry is to move it into the
current CF and then mark it as committed, however I don't know how (or
don't have permissions?) to move it into the current CF. How does one
go about doing that?

Here's the entry: https://commitfest.postgresql.org/29/2542/

Thanks,
James

Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?

[Bruce Momjian]

On Thu, Apr  8, 2021 at 12:51:06PM -0400, Álvaro Herrera wrote:
> On 2021-Apr-08, Bruce Momjian wrote:
> 
> > pg_stat_activity.queryid is new, but I can imagine cases where you would
> > join pg_stat_activity to pg_stat_statements to get an estimate of how
> > long the query will take --- having one using an underscore and another
> > one not seems odd.
> 
> OK.  So far, you have one vote for queryid (your own) and two votes for
> query_id (mine and Julien's).  And even yourself were hesitating about
> it earlier in the thread.

OK, if people are fine with pg_stat_activity.query_id not matching
pg_stat_statements.queryid, I am fine with that.  I just don't want
someone to say it was a big mistake later.  ;-)

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

Re: SQL-standard function body

[Andrew Dunstan]

On 4/8/21 2:40 AM, Julien Rouhaud wrote:
> On Wed, Apr 07, 2021 at 11:33:20PM -0700, Andres Freund wrote:
>> Hi,
>>
>> On 2021-04-08 02:05:25 -0400, Tom Lane wrote:
>>> So far the buildfarm seems to be turning green after b3ee4c503 ...
>>> so I wonder what extra condition is needed to cause the failure
>>> Andres is seeing.
>> Nothing special, really. Surprised the BF doesn't see it:
>>
>> andres@awork3:~/build/postgres/dev-assert/vpath$ cat /tmp/test.conf
>> force_parallel_mode=regress
>> andres@awork3:~/build/postgres/dev-assert/vpath$ make -j48 -s && 
>> EXTRA_REGRESS_OPTS='--temp-config /tmp/test.conf' make -s -C 
>> contrib/pg_stat_statements/ check
>> All of PostgreSQL successfully made. Ready to install.
>> ...
>> The differences that caused some tests to fail can be viewed in the
>> file 
>> "/home/andres/build/postgres/dev-assert/vpath/contrib/pg_stat_statements/regression.diffs".
>>   A copy of the test summary that you see
>> above is saved in the file 
>> "/home/andres/build/postgres/dev-assert/vpath/contrib/pg_stat_statements/regression.out".
>> ...
> Is think this is because the buildfarm client is running installcheck for the
> contribs rather than check, and pg_stat_statements/Makefile has:
>
> # Disabled because these tests require 
> "shared_preload_libraries=pg_stat_statements",
> # which typical installcheck users do not have (e.g. buildfarm clients).
> NO_INSTALLCHECK = 1
>
>
>


Yeah, Julien is right, we run "make check" for these in src/test/modules
but I missed contrib. I have fixed this on crake so we get some
immediate coverage and a fix will be pushed to github shortly.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: psql - add SHOW_ALL_RESULTS option

[Fabien COELHO]

Bonjour Julien,


Attached a patch which attempts to fix this by moving the cancellation
cancelling request after processing results.


Thank you for your fixing. I tested and the problem has been solved 
after applying your patch.


Thanks for the patch Fabien.  I've hit this issue multiple time and this is
indeed unwelcome.  Should we add it as an open item?


It is definitely a open item. I'm not sure where you want to add it… 
possibly the "Pg 14 Open Items" wiki page? I tried but I do not have 
enough privileges, if you can do it please proceed. I added an entry in 
the next CF in the bugfix section.


--
Fabien.

Re: Binary search in ScalarArrayOpExpr for OR'd constant arrays

[Alvaro Herrera]

On 2021-Apr-08, James Coleman wrote:

> I assume proper procedure for the CF entry is to move it into the
> current CF and then mark it as committed, however I don't know how (or
> don't have permissions?) to move it into the current CF. How does one
> go about doing that?
> 
> Here's the entry: https://commitfest.postgresql.org/29/2542/

Done, thanks.

-- 
Álvaro Herrera39°49'30"S 73°17'W