Re: Commit/abort WAL records with dropped rels missing XLR_SPECIAL_REL_UPDATE

[Michael Paquier]

On Sat, Aug 15, 2020 at 11:05:43AM +0530, Amit Kapila wrote:
> On Fri, Aug 14, 2020 at 2:17 PM Heikki Linnakangas  wrote:
>> It's always been like that, but I am not going backport, for fear of
>> breaking existing applications. If a program reads the WAL, and would
>> actually need to do something with commit records dropping relations,
>> that seems like such a common scenario that the author should've thought
>> about it and handled it even without the flag reminding about it. Fixing
>> it in master ought to be enough.
> 
> +1 for doing it in master only. Even if someone comes up with such a
> scenario for back-branches, we can revisit our decision to backport
> this but like you, I also don't see any pressing need to do it now.

+1.
--
Michael


signature.asc
Description: PGP signature

Re: Newline after --progress report

[Heikki Linnakangas]

On 14/08/2020 16:51, Tom Lane wrote:

Heikki Linnakangas  writes:

Attached is a patch to fix this, as well as a similar issue in
pg_checksums. pg_basebackup and pgbench also print progres reports like
this, but they seem correct to me.


I wonder whether it'd be better to push the responsibility for this
into progress_report(), by adding an additional parameter "bool last"
or the like.  Then the callers would not need such an unseemly amount
of knowledge about what progress_report() is doing.


Good point. Pushed a patch along those lines.

- Heikki

Re: Commit/abort WAL records with dropped rels missing XLR_SPECIAL_REL_UPDATE

[Heikki Linnakangas]

On 17/08/2020 10:00, Michael Paquier wrote:

On Sat, Aug 15, 2020 at 11:05:43AM +0530, Amit Kapila wrote:

On Fri, Aug 14, 2020 at 2:17 PM Heikki Linnakangas  wrote:

It's always been like that, but I am not going backport, for fear of
breaking existing applications. If a program reads the WAL, and would
actually need to do something with commit records dropping relations,
that seems like such a common scenario that the author should've thought
about it and handled it even without the flag reminding about it. Fixing
it in master ought to be enough.


+1 for doing it in master only. Even if someone comes up with such a
scenario for back-branches, we can revisit our decision to backport
this but like you, I also don't see any pressing need to do it now.


+1.


Pushed, thanks!

- Heikki

Making the function range_union_internal available to other extensions

[Esteban Zimanyi]

Dear all

In MobilityDB
https://github.com/MobilityDB/MobilityDB
we use extensively the range types.

Is there any possibility to make the function range_union_internal available to
use by other extensions ? Otherwise we need to copy/paste it verbatim. For
example lines 114-153 in
https://github.com/MobilityDB/MobilityDB/blob/develop/src/rangetypes_ext.c

Regards

Esteban

Re: proposal - reference to plpgsql_check from plpgsql doc

[Magnus Hagander]

On Mon, Aug 17, 2020 at 8:47 AM Pavel Stehule 
wrote:

> Hi
>
> plpgsql_check extension is almost complete now. This extension is
> available on all environments and for all supported Postgres releases. It
> is probably too big to be part of contrib, but I think so it can be
> referenced in
> https://www.postgresql.org/docs/current/plpgsql-development-tips.html
> chapter.
>
> What do you think about it?
>
>
Without making any valuation on this particular tool, I think we should be
very very careful and restrictive about putting such links in the main
documentation.

The appropriate location for such references are in the product catalog on
the website and on the wiki. (I'd be happy to have a link from the docs to
a generic "pl/pgsql tips" page on the wiki, though, if people would think
that helpful -- because that would be linking to a destination that we can
easily update/fix should it go stale)

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

Re: proposal - reference to plpgsql_check from plpgsql doc

[Pavel Stehule]

po 17. 8. 2020 v 10:37 odesílatel Magnus Hagander 
napsal:

>
>
> On Mon, Aug 17, 2020 at 8:47 AM Pavel Stehule 
> wrote:
>
>> Hi
>>
>> plpgsql_check extension is almost complete now. This extension is
>> available on all environments and for all supported Postgres releases. It
>> is probably too big to be part of contrib, but I think so it can be
>> referenced in
>> https://www.postgresql.org/docs/current/plpgsql-development-tips.html
>> chapter.
>>
>> What do you think about it?
>>
>>
> Without making any valuation on this particular tool, I think we should be
> very very careful and restrictive about putting such links in the main
> documentation.
>
> The appropriate location for such references are in the product catalog on
> the website and on the wiki. (I'd be happy to have a link from the docs to
> a generic "pl/pgsql tips" page on the wiki, though, if people would think
> that helpful -- because that would be linking to a destination that we can
> easily update/fix should it go stale)
>

good idea

Pavel


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

Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits

[Hamid Akhtar]

Unfortunately the latest patch doesn't apply cleanly on the master branch. Can 
you please share an updated one.

Re: track_planning causing performance regression

[Fujii Masao]

On 2020/07/31 21:40, Hamid Akhtar wrote:



On Mon, Jul 6, 2020 at 10:29 AM Fujii Masao mailto:masao.fu...@oss.nttdata.com>> wrote:



On 2020/07/04 12:22, Pavel Stehule wrote:
 >
 >
 > pá 3. 7. 2020 v 13:02 odesílatel Fujii Masao mailto:masao.fu...@oss.nttdata.com> >> napsal:
 >
 >
 >
 >     On 2020/07/03 16:02, Pavel Stehule wrote:
 >      >
 >      >
 >      > pá 3. 7. 2020 v 8:57 odesílatel Fujii Masao mailto:masao.fu...@oss.nttdata.com> > 
       >
 >      >
 >      >
 >      >     On 2020/07/03 13:05, Pavel Stehule wrote:
 >      >      > Hi
 >      >      >
 >      >      > pá 3. 7. 2020 v 4:39 odesílatel Fujii Masao mailto:masao.fu...@oss.nttdata.com> 
>  
>>  
>  
 napsal:
 >      >      >
 >      >      >
 >      >      >
 >      >      >     On 2020/07/01 7:37, Peter Geoghegan wrote:
 >      >      >      > On Tue, Jun 30, 2020 at 6:40 AM Fujii Masao mailto:masao.fu...@oss.nttdata.com> 
>  
>>  
>  
 wrote:
 >      >      >      >> Ants and Andres suggested to replace the spinlock 
used in pgss_store() with
 >      >      >      >> LWLock. I agreed with them and posted the POC 
patch doing that. But I think
 >      >      >      >> the patch is an item for v14. The patch may 
address the reported performance
 >      >      >      >> issue, but may cause other performance issues in 
other workloads. We would
 >      >      >      >> need to measure how the patch affects the 
performance in various workloads.
 >      >      >      >> It seems too late to do that at this stage of v13. 
Thought?
 >      >      >      >
 >      >      >      > I agree that it's too late for v13.
 >      >      >
 >      >      >     Thanks for the comment!
 >      >      >
 >      >      >     So I pushed the patch and changed default of 
track_planning to off.
 >      >      >
 >      >      >
 >      >      > Maybe there can be documented so enabling this option can 
have a negative impact on performance.
 >      >
 >      >     Yes. What about adding either of the followings into the doc?
 >      >
 >      >           Enabling this parameter may incur a noticeable 
performance penalty.
 >      >
 >      >     or
 >      >
 >      >           Enabling this parameter may incur a noticeable 
performance penalty,
 >      >           especially when a fewer kinds of queries are executed 
on many
 >      >           concurrent connections.
 >      >
 >      >
 >      > This second variant looks perfect for this case.
 >
 >     Ok, so patch attached.
 >
 >
 > +1

Thanks for the review! Pushed.

Regards,

-- 
Fujii Masao

Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION



You might also want to update this patch's status in the commitfest:
https://commitfest.postgresql.org/29/2634/


The patch added into this CF entry has not been committed yet.
So I was thinking that there is no need to update the status yet. No?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Re: track_planning causing performance regression

[Hamid Akhtar]

On Mon, Aug 17, 2020 at 2:21 PM Fujii Masao 
wrote:

>
>
> On 2020/07/31 21:40, Hamid Akhtar wrote:
> > 
> >
> > On Mon, Jul 6, 2020 at 10:29 AM Fujii Masao  > wrote:
> >
> >
> >
> > On 2020/07/04 12:22, Pavel Stehule wrote:
> >  >
> >  >
> >  > pá 3. 7. 2020 v 13:02 odesílatel Fujii Masao <
> masao.fu...@oss.nttdata.com   masao.fu...@oss.nttdata.com >> napsal:
> >  >
> >  >
> >  >
> >  > On 2020/07/03 16:02, Pavel Stehule wrote:
> >  >  >
> >  >  >
> >  >  > pá 3. 7. 2020 v 8:57 odesílatel Fujii Masao <
> masao.fu...@oss.nttdata.com   masao.fu...@oss.nttdata.com >  masao.fu...@oss.nttdata.com   masao.fu...@oss.nttdata.com  napsal:
> >  >  >
> >  >  >
> >  >  >
> >  >  > On 2020/07/03 13:05, Pavel Stehule wrote:
> >  >  >  > Hi
> >  >  >  >
> >  >  >  > pá 3. 7. 2020 v 4:39 odesílatel Fujii Masao <
> masao.fu...@oss.nttdata.com   masao.fu...@oss.nttdata.com >  masao.fu...@oss.nttdata.com   masao.fu...@oss.nttdata.com >>
> 
> >
> 
> 
> napsal:
> >  >  >  >
> >  >  >  >
> >  >  >  >
> >  >  >  > On 2020/07/01 7:37, Peter Geoghegan wrote:
> >  >  >  >  > On Tue, Jun 30, 2020 at 6:40 AM Fujii Masao <
> masao.fu...@oss.nttdata.com   masao.fu...@oss.nttdata.com >  masao.fu...@oss.nttdata.com   masao.fu...@oss.nttdata.com >>
> 
> >
> 
> 
> wrote:
> >  >  >  >  >> Ants and Andres suggested to replace the
> spinlock used in pgss_store() with
> >  >  >  >  >> LWLock. I agreed with them and posted the
> POC patch doing that. But I think
> >  >  >  >  >> the patch is an item for v14. The patch may
> address the reported performance
> >  >  >  >  >> issue, but may cause other performance
> issues in other workloads. We would
> >  >  >  >  >> need to measure how the patch affects the
> performance in various workloads.
> >  >  >  >  >> It seems too late to do that at this stage
> of v13. Thought?
> >  >  >  >  >
> >  >  >  >  > I agree that it's too late for v13.
> >  >  >  >
> >  >  >  > Thanks for the comment!
> >  >  >  >
> >  >  >  > So I pushed the patch and changed default of
> track_planning to off.
> >  >  >  >
> >  >  >  >
> >  >  >  > Maybe there can be documented so enabling this
> option can have a negative impact on performance.
> >  >  >
> >  >  > Yes. What about adding either of the followings into
> the doc?
> >  >  >
> >  >  >   Enabling this parameter may incur a noticeable
> performance penalty.
> >  >  >
> >  >  > or
> >  >  >
> >  >  >   Enabling this parameter may incur a noticeable
> performance penalty,
> >  >  >   especially when a fewer kinds of queries are
> executed on many
> >  >  >   concurrent connections.
> >  >  >
> >  >  >
> >  >  > This second variant looks perfect for this case.
> >  >
> >  > Ok, so patch attached.
> >  >
> >  >
> >  > +1
> >
> > Thanks for the review! Pushed.
> >
> > Regards,
> >
> > --
> > Fujii Masao
> > Advanced Computing Technology Center
> > Research and Development Headquarters
> > NTT DATA CORPORATION
> >
> >
> >
> > You might also want to update this patch's status in the commitfest:
> > https://commitfest.postgresql.org/29/2634/
>
> The patch added into this CF entry has not been committed yet.
> So I was thinking that there is no need to update the status yet. No?
>

Your previous email suggested that i

Re: EDB builds Postgres 13 with an obsolete ICU version

[Magnus Hagander]

On Fri, Aug 14, 2020 at 3:00 PM Bruce Momjian  wrote:

> On Tue, Aug 11, 2020 at 02:58:30PM +0200, Magnus Hagander wrote:
> > On Tue, Aug 4, 2020 at 11:42 AM Dave Page  wrote:
> > That would require fairly large changes to the installer to allow it
> to
> > login to the database server (whether that would work would
> be dependent on
> > how pg_hba.conf is configured), and also assumes that the ICU ABI
> hasn't
> > changed between releases. It would also require some hacky renaming
> of
> > DLLs, as they have the version number in them.
> >
> > I assumed it had code for that stuff already. Mainly because I assumed it
> > supported doing pg_upgrade, which requires similar things no?
>
> While pg_upgrade requires having the old and new cluster software in
> place, I don't think it helps allowing different ICU versions for each
> cluster.


Depends on where they are installed (and disclaimer, I don't know how the
windows installers do that). But as long as the ICU libraries are installed
in separate locations for the two versions, which I *think* they are or at
least used to be, it shouldn't have an effect on this in either direction.

That argument really only holds for different versions, not for different
clusters of the same version. But I don't think the installers (natively)
supports multiple clusters of the same version anyway.

The tricky thing is if you want to allow the user to *choose* which ICU
version should be used with postgres version .  Because then the user
might also expect an upgrade-path wherein they only upgrade the icu library
on an existing install...


> I guess you can argue that if you know the user is _not_ going
> to be using pg_upgrade, then a new ICU version should be used for the
> new cluster.
>

Yes, that's exactly the argument I meant :) If the user got the option to
"pick version of ICU: , ", with a comment saying "pick old only
if you plan to do a pg_upgrade based upgrade of a different cluster, or if
this instance should participate in replication with a node using ",
that would probably help for the vast majority of cases. And of course, if
the installer through other options can determine with certainty that it's
going to be running pg_upgrade for the user, then it can reword the dialog
based on that (that is, it should still allow the user to pick the new
version, as long as they know that their indexes are going to need
reindexing)


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

Optimising compactify_tuples()

[Thomas Munro]

Hi,

With [1] applied so that you can get crash recovery to be CPU bound
with a pgbench workload, we spend an awful lot of time in qsort(),
called from compactify_tuples().  I tried replacing that with a
specialised sort, and I got my test crash recovery time from ~55.5s
down to ~49.5s quite consistently.

I've attached a draft patch.  The sort_utils.h thing (which I've
proposed before in another context where it didn't turn out to be
needed) probably needs better naming, and a few more parameterisations
so that it could entirely replace the existing copies of the algorithm
rather than adding yet one more.  The header also contains some more
related algorithms that don't have a user right now; maybe I should
remove them.

While writing this email, I checked the archives and discovered that a
couple of other people have complained about this hot spot before and
proposed faster sorts already[2][3], and then there was a wide ranging
discussion of various options which ultimately seemed to conclude that
we should do what I'm now proposing ... and then it stalled.  The
present work is independent; I wrote this for some other sorting
problem, and then tried it out here when perf told me that it was the
next thing to fix to make recovery go faster.  So I guess what I'm
really providing here is the compelling workload and numbers that were
perhaps missing from that earlier thread, but I'm open to other
solutions too.

[1] https://commitfest.postgresql.org/29/2669/
[2] 
https://www.postgresql.org/message-id/flat/3c6ff1d3a2ff429ee80d0031e6c69cb7%40postgrespro.ru
[3] https://www.postgresql.org/message-id/flat/546B89DE.7030906%40vmware.com
From 4f3e6ccab72b3241da9afebac311eb32400eaa61 Mon Sep 17 00:00:00 2001
From: Thomas Munro 
Date: Mon, 17 Aug 2020 21:31:56 +1200
Subject: [PATCH 1/2] Add parameterized sorting/searching support.

Provide some simple sorting and searching algorithms for working with
sorted arrays, allowing for inlining of comparisons and object sizes.
---
 src/include/lib/sort_utils.h | 322 +++
 1 file changed, 322 insertions(+)
 create mode 100644 src/include/lib/sort_utils.h

diff --git a/src/include/lib/sort_utils.h b/src/include/lib/sort_utils.h
new file mode 100644
index 00..8b92e00273
--- /dev/null
+++ b/src/include/lib/sort_utils.h
@@ -0,0 +1,322 @@
+/*-
+ *
+ * sort_utils.h
+ *
+ *	  Simple sorting-related algorithms specialized for arrays of
+ *	  paramaterized type, using inlined comparators.
+ *
+ * Copyright (c) 2020, PostgreSQL Global Development Group
+ *
+ * Usage notes:
+ *
+ *	  To generate functions specialized for a type, the following parameter
+ *	  macros should be #define'd before this file is included.
+ *
+ *	  - SA_PREFIX - prefix for all symbol names generated.
+ *	  - SA_ELEMENT_TYPE - type of the referenced elements
+ *	  - SA_DECLARE - if defined the functions and types are declared
+ *	  - SA_DEFINE - if defined the functions and types are defined
+ *	  - SA_SCOPE - scope (e.g. extern, static inline) for functions
+ *
+ *	  The following macros should be #define'd to enable functions:
+ *
+ *	  - SA_ENABLE_SORT - a PREFIX_sort function
+ *	  - SA_ENABLE_UNIQUE - a PREFIX_unique function
+ *	  - SA_ENABLE_BINARY_SEARCH - a PREFIX_binary_search function
+ *	  - SA_ENABLE_LOWER_BOUND - a PREFIX_lower_bound function
+ *
+ *	  The following are relevant only when SA_DEFINE is defined:
+ *
+ *	  - SA_COMPARE(a, b) - an expression to compare pointers to two values
+ *
+ * IDENTIFICATION
+ *		src/include/lib/sort_utils.h
+ *
+ *-
+ */
+
+#define SA_MAKE_PREFIX(a) CppConcat(a,_)
+#define SA_MAKE_NAME(name) SA_MAKE_NAME_(SA_MAKE_PREFIX(SA_PREFIX),name)
+#define SA_MAKE_NAME_(a,b) CppConcat(a,b)
+
+/* function declarations */
+#define SA_SORT SA_MAKE_NAME(sort)
+#define SA_UNIQUE SA_MAKE_NAME(unique)
+#define SA_BINARY_SEARCH SA_MAKE_NAME(binary_search)
+#define SA_LOWER_BOUND SA_MAKE_NAME(lower_bound)
+
+#ifdef SA_DECLARE
+
+SA_SCOPE void SA_SORT(SA_ELEMENT_TYPE *first, size_t n);
+SA_SCOPE SA_ELEMENT_TYPE *SA_UNIQUE(SA_ELEMENT_TYPE *first,
+	SA_ELEMENT_TYPE *last);
+SA_SCOPE SA_ELEMENT_TYPE *SA_BINARY_SEARCH(SA_ELEMENT_TYPE *first,
+		   SA_ELEMENT_TYPE *last,
+		   SA_ELEMENT_TYPE *value);
+SA_SCOPE SA_ELEMENT_TYPE *SA_LOWER_BOUND(SA_ELEMENT_TYPE *first,
+		 SA_ELEMENT_TYPE *last,
+		 SA_ELEMENT_TYPE *value);
+
+#endif
+
+#ifdef SA_DEFINE
+
+#ifdef SA_ENABLE_SORT
+
+/* helper functions */
+#define SA_MED3 SA_MAKE_NAME(med3)
+#define SA_SWAP SA_MAKE_NAME(swap)
+#define SA_SWAPN SA_MAKE_NAME(swapn)
+
+static inline SA_ELEMENT_TYPE *
+SA_MED3(SA_ELEMENT_TYPE *a,
+		SA_ELEMENT_TYPE *b,
+		SA_ELEMENT_TYPE *c)
+{
+	return SA_COMPARE(a, b) < 0 ?
+		(SA_COMPARE(b, c) < 0 ? b : (SA_COMPARE(a, c) < 0 ? c : a))
+		: (SA_COMPARE(b, c) > 0 ? b : (SA_COMPARE(a, c) < 0 ? a

Re: Corruption during WAL replay

[Heikki Linnakangas]

On 14/04/2020 22:04, Teja Mupparti wrote:
Thanks Kyotaro and Masahiko for the feedback. I think there is a 
consensus on the critical-section around truncate,


+1


but I just want to emphasize the need for reversing the order of the
dropping the buffers and the truncation.

  Repro details (when full page write = off)

          1) Page on disk has empty LP 1, Insert into page LP 1
          2) checkpoint START (Recovery REDO eventually starts here)
          3) Delete all rows on the page (page is empty now)
          4) Autovacuum kicks in and truncates the pages
                  DropRelFileNodeBuffers - Dirty page NOT written, LP 1 
on disk still empty

          5) Checkpoint completes
          6) Crash
          7) smgrtruncate - Not reached (this is where we do the 
physical truncate)


  Now the crash-recovery starts

          Delete-log-replay (above step-3) reads page with empty LP 1 
and the delete fails with PANIC (old page on disk with no insert)


Doing recovery, truncate is even not reached, a WAL replay of the 
truncation will happen in the future but the recovery fails (repeatedly) 
even before reaching that point.


Hmm. I think simply reversing the order of DropRelFileNodeBuffers() and 
truncating the file would open a different issue:


  1) Page on disk has empty LP 1, Insert into page LP 1
  2) checkpoint START (Recovery REDO eventually starts here)
  3) Delete all rows on the page (page is empty now)
  4) Autovacuum kicks in and starts truncating
  5) smgrtruncate() truncates the file
  6) checkpoint writes out buffers for pages that were just truncated 
away, expanding the file again.


Your patch had a mechanism to mark the buffers as io-in-progress before 
truncating the file to fix that, but I'm wary of that approach. Firstly, 
it requires scanning the buffers that are dropped twice, which can take 
a long time. I remember that people have already complained that 
DropRelFileNodeBuffers() is slow, when it has to scan all the buffers 
once. More importantly, abusing the BM_IO_INPROGRESS flag for this seems 
bad. For starters, because you're not holding buffer's I/O lock, I 
believe the checkpointer would busy-wait on the buffers until the 
truncation has completed. See StartBufferIO() and AbortBufferIO().


Perhaps a better approach would be to prevent the checkpoint from 
completing, until all in-progress truncations have completed. We have a 
mechanism to wait out in-progress commits at the beginning of a 
checkpoint, right after the redo point has been established. See 
comments around the GetVirtualXIDsDelayingChkpt() function call in 
CreateCheckPoint(). We could have a similar mechanism to wait out the 
truncations before *completing* a checkpoint.


- Heikki

Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits

[Ibrar Ahmed]

On Mon, Aug 17, 2020 at 2:19 PM Hamid Akhtar  wrote:

> Unfortunately the latest patch doesn't apply cleanly on the master branch.
> Can you please share an updated one.


Please see the attached patch rebased with master (
a28d731a1187e8d9d8c2b6319375fcbf0a8debd5)
-- 
Ibrar Ahmed


copy-freeze-vm_freeze_v2.patch
Description: Binary data

Re: EDB builds Postgres 13 with an obsolete ICU version

[Dave Page]

On Mon, Aug 17, 2020 at 11:19 AM Magnus Hagander 
wrote:

>
>
> On Fri, Aug 14, 2020 at 3:00 PM Bruce Momjian  wrote:
>
>> On Tue, Aug 11, 2020 at 02:58:30PM +0200, Magnus Hagander wrote:
>> > On Tue, Aug 4, 2020 at 11:42 AM Dave Page  wrote:
>> > That would require fairly large changes to the installer to allow
>> it to
>> > login to the database server (whether that would work would
>> be dependent on
>> > how pg_hba.conf is configured), and also assumes that the ICU ABI
>> hasn't
>> > changed between releases. It would also require some hacky renaming
>> of
>> > DLLs, as they have the version number in them.
>> >
>> > I assumed it had code for that stuff already. Mainly because I assumed
>> it
>> > supported doing pg_upgrade, which requires similar things no?
>>
>
No, the installers don't support pg_upgrade directly. They ship it of
course, and the user can manually run it, but the installers won't do that,
and have no ability to login to a cluster except during the post-initdb
phase.


>
>> While pg_upgrade requires having the old and new cluster software in
>> place, I don't think it helps allowing different ICU versions for each
>> cluster.
>
>
> Depends on where they are installed (and disclaimer, I don't know how the
> windows installers do that). But as long as the ICU libraries are installed
> in separate locations for the two versions, which I *think* they are or at
> least used to be, it shouldn't have an effect on this in either direction.
>

They are.


>
> That argument really only holds for different versions, not for different
> clusters of the same version. But I don't think the installers (natively)
> supports multiple clusters of the same version anyway.
>

They don't. You'd need to manually init a new cluster and register a new
server instance. The installer only has any knowledge of the cluster it
sets up.


>
> The tricky thing is if you want to allow the user to *choose* which ICU
> version should be used with postgres version .  Because then the user
> might also expect an upgrade-path wherein they only upgrade the icu library
> on an existing install...
>
>
>> I guess you can argue that if you know the user is _not_ going
>> to be using pg_upgrade, then a new ICU version should be used for the
>> new cluster.
>>
>
> Yes, that's exactly the argument I meant :) If the user got the option to
> "pick version of ICU: , ", with a comment saying "pick old only
> if you plan to do a pg_upgrade based upgrade of a different cluster, or if
> this instance should participate in replication with a node using ",
> that would probably help for the vast majority of cases. And of course, if
> the installer through other options can determine with certainty that it's
> going to be running pg_upgrade for the user, then it can reword the dialog
> based on that (that is, it should still allow the user to pick the new
> version, as long as they know that their indexes are going to need
> reindexing)
>

That seems like a very hacky and extremely user-unfriendly approach. How
many users are going to understand options in the installer to deal with
that, or want to go decode the ICU filenames on their existing
installations (which our installers may not actually know about) to figure
out what their current version is?

I would suggest that the better way to handle this would be for pg_upgrade
to (somehow) check the ICU version on the old and new clusters and if
there's a mismatch perform a reindex of any ICU based indexes. I suspect
that may require that the server exposes the ICU version though. That way,
the installers could freely upgrade the ICU version with a new major
release.

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: http://www.enterprisedb.com

Re: Creating a function for exposing memory usage of backend process

[Fujii Masao]

On 2020/08/11 15:24, torikoshia wrote:

On 2020-08-08 10:44, Michael Paquier wrote:

On Fri, Jul 31, 2020 at 03:23:52PM -0400, Robert Haas wrote:

On Fri, Jul 31, 2020 at 4:25 AM torikoshia  wrote:

And as Fujii-san told me in person, exposing memory address seems
not preferable considering there are security techniques like
address space layout randomization.


Yeah, exactly. ASLR wouldn't do anything to improve security if there
were no other security bugs, but there are, and some of those bugs are
harder to exploit if you don't know the precise memory addresses of
certain data structures. Similarly, exposing the addresses of our
internal data structures is harmless if we have no other security
bugs, but if we do, it might make those bugs easier to exploit. I
don't think this information is useful enough to justify taking that
risk.


FWIW, this is the class of issues where it is possible to print some
areas of memory, or even manipulate the stack so as it was possible to
pass down a custom pointer, so exposing the pointer locations is a
real risk, and this has happened in the past.  Anyway, it seems to me
that if this part is done, we could just make it superuser-only with
restrictive REVOKE privileges, but I am not sure that we have enough
user cases to justify this addition.



Thanks for your comments!

I convinced that exposing pointer locations introduce security risks
and it seems better not to do so.

And I now feel identifying exact memory context by exposing memory
address or other means seems overkill.
Showing just the context name of the parent would be sufficient and
0007 pattch takes this way.


Agreed.





On 2020-08-07 16:38, Kasahara Tatsuhito wrote:

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   not tested
Documentation:    tested, passed

I tested the latest
patch(0007-Adding-a-function-exposing-memory-usage-of-local-backend.patch)
with the latest PG-version (199cec9779504c08aaa8159c6308283156547409)
and test was passed.
It looks good to me.

The new status of this patch is: Ready for Committer


Thanks for your testing!


Thanks for updating the patch! Here are the review comments.

+ 
+  pg_backend_memory_contexts
+  backend memory contexts
+ 

The above is located just after pg_matviews entry. But it should be located
just after pg_available_extension_versions entry. Because the rows in the table
"System Views" should be located in alphabetical order.


+ 
+  pg_backend_memory_contexts

Same as above.


+   The view pg_backend_memory_contexts displays all
+   the local backend memory contexts.

This description seems a bit confusing because maybe we can interpret this
as "... displays the memory contexts of all the local backends" wrongly. 
Thought?
What about the following description, instead?

The view pg_backend_memory_contexts displays all
the memory contexts of the server process attached to the current session.


+   const char *name = context->name;
+   const char *ident = context->ident;
+
+   if (context == NULL)
+   return;

The above check "context == NULL" is useless? If "context" is actually NULL,
"context->name" would cause segmentation fault, so ISTM that the check
will never be performed.

If "context" can be NULL, the check should be performed before accessing
to "contect". OTOH, if "context" must not be NULL per the specification of
PutMemoryContextStatsTupleStore(), assertion test checking
"context != NULL" should be used here, instead?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Re: Creating a function for exposing memory usage of backend process

[Fujii Masao]

On 2020/08/17 21:14, Fujii Masao wrote:



On 2020/08/11 15:24, torikoshia wrote:

On 2020-08-08 10:44, Michael Paquier wrote:

On Fri, Jul 31, 2020 at 03:23:52PM -0400, Robert Haas wrote:

On Fri, Jul 31, 2020 at 4:25 AM torikoshia  wrote:

And as Fujii-san told me in person, exposing memory address seems
not preferable considering there are security techniques like
address space layout randomization.


Yeah, exactly. ASLR wouldn't do anything to improve security if there
were no other security bugs, but there are, and some of those bugs are
harder to exploit if you don't know the precise memory addresses of
certain data structures. Similarly, exposing the addresses of our
internal data structures is harmless if we have no other security
bugs, but if we do, it might make those bugs easier to exploit. I
don't think this information is useful enough to justify taking that
risk.


FWIW, this is the class of issues where it is possible to print some
areas of memory, or even manipulate the stack so as it was possible to
pass down a custom pointer, so exposing the pointer locations is a
real risk, and this has happened in the past.  Anyway, it seems to me
that if this part is done, we could just make it superuser-only with
restrictive REVOKE privileges, but I am not sure that we have enough
user cases to justify this addition.



Thanks for your comments!

I convinced that exposing pointer locations introduce security risks
and it seems better not to do so.

And I now feel identifying exact memory context by exposing memory
address or other means seems overkill.
Showing just the context name of the parent would be sufficient and
0007 pattch takes this way.


Agreed.





On 2020-08-07 16:38, Kasahara Tatsuhito wrote:

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   not tested
Documentation:    tested, passed

I tested the latest
patch(0007-Adding-a-function-exposing-memory-usage-of-local-backend.patch)
with the latest PG-version (199cec9779504c08aaa8159c6308283156547409)
and test was passed.
It looks good to me.

The new status of this patch is: Ready for Committer


Thanks for your testing!


Thanks for updating the patch! Here are the review comments.

+ 
+  pg_backend_memory_contexts
+  backend memory contexts
+ 

The above is located just after pg_matviews entry. But it should be located
just after pg_available_extension_versions entry. Because the rows in the table
"System Views" should be located in alphabetical order.


+ 
+  pg_backend_memory_contexts

Same as above.


+   The view pg_backend_memory_contexts displays all
+   the local backend memory contexts.

This description seems a bit confusing because maybe we can interpret this
as "... displays the memory contexts of all the local backends" wrongly. 
Thought?
What about the following description, instead?

     The view pg_backend_memory_contexts displays all
     the memory contexts of the server process attached to the current session.


+    const char *name = context->name;
+    const char *ident = context->ident;
+
+    if (context == NULL)
+    return;

The above check "context == NULL" is useless? If "context" is actually NULL,
"context->name" would cause segmentation fault, so ISTM that the check
will never be performed.

If "context" can be NULL, the check should be performed before accessing
to "contect". OTOH, if "context" must not be NULL per the specification of
PutMemoryContextStatsTupleStore(), assertion test checking
"context != NULL" should be used here, instead?


Here is another comment.

+   if (parent == NULL)
+   nulls[2] = true;
+   else
+   /*
+* We labeled dynahash contexts with just the hash table name.
+* To make it possible to identify its parent, we also display
+* parent's ident here.
+*/
+   if (parent->ident && strcmp(parent->name, "dynahash") == 0)
+   values[2] = CStringGetTextDatum(parent->ident);
+   else
+   values[2] = CStringGetTextDatum(parent->name);

PutMemoryContextsStatsTupleStore() doesn't need "parent" memory context,
but uses only the name of "parent" memory context. So isn't it better to use
"const char *parent" instead of "MemoryContext parent", as the argument of
the function? If we do that, we can simplify the above code.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Re: track_planning causing performance regression

[Fujii Masao]

On 2020/08/17 18:34, Hamid Akhtar wrote:



On Mon, Aug 17, 2020 at 2:21 PM Fujii Masao mailto:masao.fu...@oss.nttdata.com>> wrote:



On 2020/07/31 21:40, Hamid Akhtar wrote:
 > 
 >
 > On Mon, Jul 6, 2020 at 10:29 AM Fujii Masao mailto:masao.fu...@oss.nttdata.com> >> wrote:
 >
 >
 >
 >     On 2020/07/04 12:22, Pavel Stehule wrote:
 >      >
 >      >
 >      > pá 3. 7. 2020 v 13:02 odesílatel Fujii Masao mailto:masao.fu...@oss.nttdata.com> > 
       >
 >      >
 >      >
 >      >     On 2020/07/03 16:02, Pavel Stehule wrote:
 >      >      >
 >      >      >
 >      >      > pá 3. 7. 2020 v 8:57 odesílatel Fujii Masao mailto:masao.fu...@oss.nttdata.com> 
>  
>>  
>  
 napsal:
 >      >      >
 >      >      >
 >      >      >
 >      >      >     On 2020/07/03 13:05, Pavel Stehule wrote:
 >      >      >      > Hi
 >      >      >      >
 >      >      >      > pá 3. 7. 2020 v 4:39 odesílatel Fujii Masao mailto:masao.fu...@oss.nttdata.com> >  >> 
 >   
>  >>  >  
> napsal:
 >      >      >      >
 >      >      >      >
 >      >      >      >
 >      >      >      >     On 2020/07/01 7:37, Peter Geoghegan wrote:
 >      >      >      >      > On Tue, Jun 30, 2020 at 6:40 AM Fujii Masao mailto:masao.fu...@oss.nttdata.com> >  >> 
 >   
>  >>
 >  
> wrote:
 >      >      >      >      >> Ants and Andres suggested to replace the 
spinlock used in pgss_store() with
 >      >      >      >      >> LWLock. I agreed with them and posted the 
POC patch doing that. But I think
 >      >      >      >      >> the patch is an item for v14. The patch may 
address the reported performance
 >      >      >      >      >> issue, but may cause other performance 
issues in other workloads. We would
 >      >      >      >      >> need to measure how the patch affects the 
performance in vario

Re: Add header support to text format and matching feature

[vignesh C]

Thanks for your comments, Please find my thoughts inline.

> In my tests it works fine except for one crash that I can reproduce
> on a fresh build and default configuration with:
>
> $ cat >file.txt
> i
> 1
>
> $ psql
> postgres=# create table x(i int);
> CREATE TABLE
> postgres=# \copy x(i) from file.txt with (header match)
> COPY 1
> postgres=# \copy x(i) from file.txt with (header match)
> COPY 1
> postgres=# \copy x(i) from file.txt with (header match)
> COPY 1
> postgres=# \copy x(i) from file.txt with (header match)
> COPY 1
> postgres=# \copy x(i) from file.txt with (header match)
> COPY 1
> postgres=# \copy x(i) from file.txt with (header match)
> PANIC:  ERRORDATA_STACK_SIZE exceeded
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
>

Fixed, replaced PG_TRY/PG_CATCH with strcmp logic to get the header option.

>
> Code comments:
>
>
> +/*
> + * Represents whether the header must be absent, present or present and
> match.
> + */
> +typedef enum CopyHeader
> +{
> +   COPY_HEADER_ABSENT,
> +   COPY_HEADER_PRESENT,
> +   COPY_HEADER_MATCH
> +} CopyHeader;
> +
>  /*
>   * This struct contains all the state variables used throughout a COPY
>   * operation. For simplicity, we use the same struct for all variants of
> COPY,
> @@ -136,7 +146,7 @@ typedef struct CopyStateData
> boolbinary; /* binary format? */
> boolfreeze; /* freeze rows on loading? */
> boolcsv_mode;   /* Comma Separated Value
> format? */
> -   boolheader_line;/* CSV or text header line? */
> +   CopyHeader  header_line;/* CSV or text header line? */
>
>
> After the redefinition into this enum type, there are still a
> bunch of references to header_line that treat it like a boolean:
>
> 1190:   if (cstate->header_line)
> 1398:   if (cstate->binary && cstate->header_line)
> 2119:   if (cstate->header_line)
> 3635:   if (cstate->cur_lineno == 0 && cstate->header_line)
>
> It works fine since COPY_HEADER_ABSENT is 0 as the first value of the enum,
> but maybe it's not good style to count on that.

Fixed. Changed it to cstate->header_line != COPY_HEADER_ABSENT.

>
>
>
> +   PG_TRY();
> +   {
> +   if (defGetBoolean(defel))
> +   cstate->header_line =
> COPY_HEADER_PRESENT;
> +   else
> +   cstate->header_line =
> COPY_HEADER_ABSENT;
> +   }
> +   PG_CATCH();
> +   {
> +   char*sval = defGetString(defel);
> +
> +   if (!cstate->is_copy_from)
> +   PG_RE_THROW();
> +
> +   if (pg_strcasecmp(sval, "match") == 0)
> +   cstate->header_line =
> COPY_HEADER_MATCH;
> +   else
> +   ereport(ERROR,
> +
> (errcode(ERRCODE_SYNTAX_ERROR),
> +errmsg("header requires a
> boolean or \"match\"")));
> +   }
> +   PG_END_TRY();
>
> It seems wrong to use a PG_CATCH block for this. I understand that
> it's because defGetBoolean() calls ereport() on non-booleans, but then
> it should be split into an error-throwing function and a
> non-error-throwing lexical analysis of the boolean, the above code
> calling the latter.
> Besides the comments in elog.h above PG_TRY say that
>  "the error recovery code
>   can either do PG_RE_THROW to propagate the error outwards, or do a
>   (sub)transaction abort. Failure to do so may leave the system in an
>   inconsistent state for further processing."
> Maybe this is what happens with the repeated uses of "match"
> eventually failing with ERRORDATA_STACK_SIZE exceeded.
>

Fixed, replaced PG_TRY/PG_CATCH with strcmp logic to get the header option.

>
> -HEADER [ boolean ]
> +HEADER { match | true |
> false }
>
> This should be enclosed in square brackets because HEADER
> with no argument is still accepted.
>

Fixed.

>
>
>
> +  names from the table. On input, the first line is discarded when set
> +  to true or required to match the column names if
> set
>
> The elision of "header" as the subject might be misinterpreted as if
> it's the first line that is true.  I'd suggest
> "when header>/literal> is set to ..."  to avoid any confusion.
>

Fixed.

Attached v5 patch with the fixes of above comments.
Thoughts?

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com
From 847c0d64dfd4991d52b6c6e47abcd10e23d4bf8b Mon Sep 17 00:00:00 2001
Fro

Re: Terminate the idle sessions

[Bharath Rupireddy]

On Fri, Aug 14, 2020 at 1:32 PM Li Japin  wrote:
>
> On Aug 14, 2020, at 2:15 PM, Bharath Rupireddy <
bharath.rupireddyforpostg...@gmail.com> wrote:
>
> I think, since the idle_session_timeout is by default disabled, we
> have no problem. My thought is what if a user enables the
> feature(knowingly or unknowingly) on the remote backend? If the user
> knows about the above scenario, that may be fine. On the other hand,
> either we can always the feature on the remote backend(at the
> beginning of the remote txn, like we set for some other configuration
> settings see - configure_remote_session() in connection.c) or how
> about mentioning the above scenario in this feature documentation?
>
> Though we can disable the idle_session_timeout when using postgres_fdw,
> there still has locally cached connection cache entries when the remote
sessions
> terminated by accident.  AFAIK, you have provided a patch to solve this
> problem, and it is in current CF [1].
>
> [1] - https://commitfest.postgresql.org/29/2651/
>

Yes, that solution can retry the cached connections at only the beginning
of the remote txn and not at the middle of the remote txn and that makes
sense as we can not retry connecting to a different remote backend in the
middle of a remote txn.

+1 for disabling the idle_session_timeout feature in case of postgres_fdw.
This can avoid the remote backends to timeout during postgres_fdw usages.

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

Re: Newline after --progress report

[Tom Lane]

Heikki Linnakangas  writes:
> Good point. Pushed a patch along those lines.

Uh ... you patched v12 but not v13?

Also, I'd recommend that you NOT do this:

+   fprintf(stderr, (!finished && isatty(fileno(stderr))) ? "\r" : "\n");

as it breaks printf format verification in many/most compilers.

regards, tom lane

Re: Is it useful to record whether plans are generic or custom?

[Fujii Masao]

On 2020/07/30 16:34, legrand legrand wrote:

 >> Main purpose is to decide (1) the user interface and (2) the

way to get the plan type from pg_stat_statements.

(1) the user interface
I added a new boolean column 'generic_plan' to both
pg_stat_statements view and the member of the hash key of
pg_stat_statements.

This is because as Legrand pointed out the feature seems
useful under the condition of differentiating all the
counters for a queryid using a generic plan and the one
using a custom one.



I don't like this because this may double the number of entries in pgss.
Which means that the number of entries can more easily reach
pg_stat_statements.max and some entries will be discarded.


Not all the entries will be doubled, only the ones that are prepared.
And even if auto prepare was implemented, having 5000, 1 or 2
max entries seems not a problem to me.


I thought it might be preferable to make a GUC to enable
or disable this feature, but changing the hash key makes
it harder.



What happens if the server was running with this option enabled and then
restarted with the option disabled? Firstly two entries for the same query
were stored in pgss because the option was enabled. But when it's disabled
and the server is restarted, those two entries should be merged into one
at the startup of server? If so, that's problematic because it may take
a long time.


What would you think about a third value for this flag to handle disabled
case (no need to merge entries in this situation) ?


Sorry I failed to understand your point. You mean that we can have another flag
to specify whether to merge the entries for the same query at that case or not?

If those entries are not merged, what does pg_stat_statements return?
It returns the sum of those entries? Or either generic or custom entry is
returned?





Therefore I think that it's better and simple to just expose the number of
times generic/custom plan was chosen for each query.


I thought this feature was mainly needed to identifiy "under optimal generic
plans". Without differentiating execution counters, this will be simpler but
useless in this case ... isn't it ?


Could you elaborate "under optimal generic plans"? Sorry, I failed to
understand your point.. But maybe you're thinking to use this feature to
check which generic or custom plan is better for each query?

I was just thinking that this feature was useful to detect the case where
the query was executed with unpected plan mode. That is, we can detect
the unexpected case where the query that should be executed with generic
plan is actually executed with custom plan, and vice versa.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Re: jsonb, collection & postgres_fdw

[Bharath Rupireddy]

 On Fri, Aug 14, 2020 at 12:46 PM Konstantin Knizhnik <
k.knizh...@postgrespro.ru> wrote:
>
> Right now postgres_fdw treat as shippable only builtin functions or
> functions from extensions explicitly specified as shippable extensions
> in parameters of this FDW server. So I do no see a problem here. Yes,
> foreign server may have different version of Postgres which doesn't have
> this built-in function or its  profile is different. It can happen if
> postgres_fdw is used to connect two different servers which are
> maintained independently. But in most cases I think, postgres_fdw is
> used to organize some kind of cluster. In this case all nodes are
> identical (hardware, OS, postgres version) and performance is very
> critical (because scalability - of one of the goal of replacing single
> node with cluster).
> This is why push down of predicates is very critical in this case.
>

Agree, push down of predicates(with functions) to the remote backend helps
a lot. But, is it safe to push all the functions? For instance, functions
that deal with time/time zones, volatile functions etc. I'm not exactly
sure whether we will have some issues here. Since postgres_fdw can also be
used for independently maintained postgres servers(may be with different
versions), we must have a mechanism to know the compatibility.

>
>  From my point of view, it will be nice to have flag in postgres_fdw
> server indicating that foreign and remote servers are identical
> and treat all functions as shippable in this case (not only built-in
> ones are belonging to explicitly specified shippable extensions).
> It will simplify using postres_fdw in clusters and makes it more
efficient.
>

I think it's better not to have a flag for this. As we have to deal with
the compatibility not only at the server version level, but also at each
function level. We could have something like a configuration file which
allows the user to specify the list of functions that are safely pushable
to remote in his/her own postgres_fdw setup, and let the postgres_fdw refer
this configuration file, while checking the pushability of the functions to
remote. This way, the user has some control over what's pushed and what's
not. Of course, this pushability check can only happen after the mandatory
checks happening currently such as remote backend configuration settings
such as collations etc.

Feel free to correct me.

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

Re: Improve planner cost estimations for alternative subplans

[Andy Fan]

On Mon, Jun 22, 2020 at 9:39 AM Tom Lane  wrote:

> I wrote:
> > Nope.  The entire reason why we have that kluge is that we don't know
> > until much later how many times we expect to execute the subplan.
> > AlternativeSubPlan allows the decision which subplan form to use to be
> > postponed till runtime; but when we're doing things like estimating the
> > cost and selectivity of a where-clause, we don't know that.
>
> > Maybe there's some way to recast things to avoid that problem,
> > but I have little clue what it'd look like.
>
> Actually ... maybe it's not that bad.  Clearly there would be a
> circularity issue for selectivity estimation, but all the alternatives
> should have the same selectivity.  Cost estimation is a different story:
> by the time we need to do cost estimation for a subexpression, we do in
> many cases have an idea how often the subexpression will be executed.
>
>
I read your idea of "ripping out all executor support for AlternativeSubPlan
 and instead having the planner replace an AlternativeSubPlan with
 the desired specific SubPlan somewhere late in planning, possibly
setrefs.c."
in [1].  I was thinking that if we can do such a replacement sooner,
for example once we know the num_calls for the subplans, Unknown if it
is possible though.  If we can, then we can handle the issue here as well.

The attached is a very PoC version,  I'm not sure if it is the right
direction
to go.  I'm sorry that I still need more time to understand your solution
below but I'm too excited about your original idea.

[1] https://www.postgresql.org/message-id/1992952.1592785...@sss.pgh.pa.us



> I experimented with adding a number-of-evaluations parameter to
> cost_qual_eval, and found that the majority of call sites do have
> something realistic they can pass.  The attached very-much-WIP
> patch shows my results so far.  There's a lot of loose ends:
>
> * Any call site using COST_QUAL_EVAL_DUMMY_NUM_EVALS is a potential spot
> for future improvement.  The only one that seems like it might be
> fundamentally unsolvable is cost_subplan's usage; but that would only
> matter if a subplan's testexpr contains an AlternativeSubPlan, which is
> probably a negligible case in practice.  The other ones seem to just
> require more refactoring than I cared to do on a Sunday afternoon.
>
> * I did not do anything for postgres_fdw.c beyond making it compile.
> We can surely do better there, but it might require some rethinking
> of the way that plan costs get cached.
>
> * The merge and hash join costsize.c functions estimate costs of qpquals
> (i.e. quals to be applied at the join that are not being used as merge
> or hash conditions) by computing cost_qual_eval of the whole
> joinrestrictlist and then subtracting off the cost of the merge or hash
> quals.  This is kind of broken if we want to use different num_eval
> estimates for the qpquals and the merge/hash quals, which I think we do.
> This probably just needs some refactoring to fix.  We also need to save
> the relevant rowcounts in the join Path nodes so that createplan.c can
> do the right thing.
>
> * I think it might be possible to improve the situation in
> get_agg_clause_costs() if we're willing to postpone collection
> of the actual aggregate costs till later.  This'd require two
> passes over the aggregate expressions, but that seems like it
> might not be terribly expensive.  (I'd be inclined to also look
> at the idea of merging duplicate agg calls at plan time not
> run time, if we refactor that.)
>
> * I had to increase the number of table rows in one updatable_views.sql
> test to keep the plans the same.  Without that, the planner realized
> that a seqscan would be cheaper than an indexscan.  The result wasn't
> wrong exactly, but it failed to prove that leakproof quals could be
> used as indexquals, so I think we need to keep the plan choice the same.
>
> Anyway, this is kind of invasive, but I think it shouldn't really
> add noticeable costs as long as we save relevant rowcounts rather
> than recomputing them in createplan.c.  Is it worth doing?  I dunno.
> AlternativeSubPlan is pretty much a backwater, I think --- if it
> were interesting performance-wise to a lot of people, more would
> have been done with it by now.
>
> regards, tom lane
>
>

-- 
Best Regards
Andy Fan


v1-0001-Convert-the-AlternativeSubplan-to-Subplan-as-soon.patch
Description: Binary data

Re: Parallel bitmap index scan

[Bharath Rupireddy]

On Sun, Jul 26, 2020 at 6:43 PM Dilip Kumar  wrote:
>
> I would like to propose a patch for enabling the parallelism for the
> bitmap index scan path.
>
> Background:
> Currently, we support only a parallel bitmap heap scan path.  Therein,
> the underlying bitmap index scan is done by a single worker called the
> leader.  The leader creates a bitmap in shared memory and once the
> bitmap is ready it creates a shared iterator and after that, all the
> workers process the shared iterator and scan the heap in parallel.
> While analyzing the TPCH plan we have observed that some of the
> queries are spending significant time in preparing the bitmap.  So the
> idea of this patch is to use the parallel index scan for preparing the
> underlying bitmap in parallel.
>
> Design:
> If underlying index AM supports the parallel path (currently only
> BTREE support it), then we will create a parallel bitmap heap scan
> path on top of the parallel bitmap index scan path.  So the idea of
> this patch is that each worker will do the parallel index scan and
> generate their part of the bitmap.  And, we will create a barrier so
> that we can not start preparing the shared iterator until all the
> worker is ready with their bitmap.  The first worker, which is ready
> with the bitmap will keep a copy of its TBM and the page table in the
> shared memory.  And, all the subsequent workers will merge their TBM
> with the shared TBM.  Once all the TBM are merged we will get one
> common shared TBM and after that stage, the worker can continue.  The
> remaining part is the same,  basically, again one worker will scan the
> shared TBM and prepare the shared iterator and once it is ready all
> the workers will jointly scan the heap in parallel using shared
> iterator.
>

Though I have not looked at the patch or code for the existing
parallel bitmap heap scan, one point keeps bugging in my mind. I may
be utterly wrong or my question may be so silly, anyways I would like
to ask here:

>From the above design: each parallel worker creates partial bitmaps
for the index data that they looked at. Why should they merge these
bitmaps to a single bitmap in shared memory? Why can't each parallel
worker do a bitmap heap scan using the partial bitmaps they built
during it's bitmap index scan and emit qualified tuples/rows so that
the gather node can collect them? There may not be even lock
contention as bitmap heap scan takes read locks for the heap
pages/tuples.

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

Re: Parallel bitmap index scan

[Dilip Kumar]

On Mon, 17 Aug 2020 at 7:42 PM, Bharath Rupireddy <
bharath.rupireddyforpostg...@gmail.com> wrote:

> On Sun, Jul 26, 2020 at 6:43 PM Dilip Kumar  wrote:
>
> >
>
> > I would like to propose a patch for enabling the parallelism for the
>
> > bitmap index scan path.
>
> >
>
> > Background:
>
> > Currently, we support only a parallel bitmap heap scan path.  Therein,
>
> > the underlying bitmap index scan is done by a single worker called the
>
> > leader.  The leader creates a bitmap in shared memory and once the
>
> > bitmap is ready it creates a shared iterator and after that, all the
>
> > workers process the shared iterator and scan the heap in parallel.
>
> > While analyzing the TPCH plan we have observed that some of the
>
> > queries are spending significant time in preparing the bitmap.  So the
>
> > idea of this patch is to use the parallel index scan for preparing the
>
> > underlying bitmap in parallel.
>
> >
>
> > Design:
>
> > If underlying index AM supports the parallel path (currently only
>
> > BTREE support it), then we will create a parallel bitmap heap scan
>
> > path on top of the parallel bitmap index scan path.  So the idea of
>
> > this patch is that each worker will do the parallel index scan and
>
> > generate their part of the bitmap.  And, we will create a barrier so
>
> > that we can not start preparing the shared iterator until all the
>
> > worker is ready with their bitmap.  The first worker, which is ready
>
> > with the bitmap will keep a copy of its TBM and the page table in the
>
> > shared memory.  And, all the subsequent workers will merge their TBM
>
> > with the shared TBM.  Once all the TBM are merged we will get one
>
> > common shared TBM and after that stage, the worker can continue.  The
>
> > remaining part is the same,  basically, again one worker will scan the
>
> > shared TBM and prepare the shared iterator and once it is ready all
>
> > the workers will jointly scan the heap in parallel using shared
>
> > iterator.
>
> >
>
>
>
> Though I have not looked at the patch or code for the existing
>
> parallel bitmap heap scan, one point keeps bugging in my mind. I may
>
> be utterly wrong or my question may be so silly, anyways I would like
>
> to ask here:
>
>
>
> From the above design: each parallel worker creates partial bitmaps
>
> for the index data that they looked at. Why should they merge these
>
> bitmaps to a single bitmap in shared memory? Why can't each parallel
>
> worker do a bitmap heap scan using the partial bitmaps they built
>
> during it's bitmap index scan and emit qualified tuples/rows so that
>
> the gather node can collect them? There may not be even lock
>
> contention as bitmap heap scan takes read locks for the heap
>
> pages/tuples.


The main reason is that there could be lossy pages in bitmap and if that is
the case then there will be duplicate data.  Maybe if there is no lossy
data in any of the bitmap we might do as u describe but still I think that
it is very much possible that different bitmap might have many common heap
pages because bitmap is prepared using index scan.  And in such cases we
will be doing duplicate i/o.

> --
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: Newline after --progress report

[Heikki Linnakangas]

On 17/08/2020 16:59, Tom Lane wrote:

Heikki Linnakangas  writes:

Good point. Pushed a patch along those lines.


Uh ... you patched v12 but not v13?


Darn, I forgot it exists.


Also, I'd recommend that you NOT do this:

+   fprintf(stderr, (!finished && isatty(fileno(stderr))) ? "\r" : "\n");

as it breaks printf format verification in many/most compilers.


Ok. I pushed the same commit to v12 as to other branches now, to keep 
them in sync. I'll go fix that as a separate commit. Thanks!


- Heikki

Re: One-off failure in "cluster" test

[Tom Lane]

Thomas Munro  writes:
> Ahh, I see what's happening.  You don't need a concurrent process
> scanning *your* table for scan order to be nondeterministic.  The
> preceding CLUSTER command can leave the start block anywhere if its
> call to ss_report_location() fails to acquire SyncScanLock
> conditionally.  So I think we just need to disable that for this test,
> like in the attached.

Hmm.  I'm not terribly thrilled about band-aiding one unstable test
case at a time.

heapgettup makes a point of ensuring that its scan end position
gets reported:

page++;
if (page >= scan->rs_nblocks)
page = 0;
finished = (page == scan->rs_startblock) ||
(scan->rs_numblocks != InvalidBlockNumber ? 
--scan->rs_numblocks == 0 : false);

/*
 * Report our new scan position for synchronization purposes. We
 * don't do that when moving backwards, however. That would just
 * mess up any other forward-moving scanners.
 *
 * Note: we do this before checking for end of scan so that the
 * final state of the position hint is back at the start of the
 * rel.  That's not strictly necessary, but otherwise when you run
 * the same query multiple times the starting position would shift
 * a little bit backwards on every invocation, which is confusing.
 * We don't guarantee any specific ordering in general, though.
 */
if (scan->rs_base.rs_flags & SO_ALLOW_SYNC)
ss_report_location(scan->rs_base.rs_rd, page);

Seems like the conditional LWLockAcquire is pissing away that attempt
at stability.  Maybe we should adjust things so that the final
location report isn't done conditionally.

regards, tom lane

Re: EDB builds Postgres 13 with an obsolete ICU version

[Magnus Hagander]

On Mon, Aug 17, 2020 at 1:44 PM Dave Page  wrote:

>
>
> On Mon, Aug 17, 2020 at 11:19 AM Magnus Hagander 
> wrote:
>
>>
>>
>> On Fri, Aug 14, 2020 at 3:00 PM Bruce Momjian  wrote:
>>
>>> On Tue, Aug 11, 2020 at 02:58:30PM +0200, Magnus Hagander wrote:
>>> > On Tue, Aug 4, 2020 at 11:42 AM Dave Page  wrote:
>>> > That would require fairly large changes to the installer to allow
>>> it to
>>> > login to the database server (whether that would work would
>>> be dependent on
>>> > how pg_hba.conf is configured), and also assumes that the ICU ABI
>>> hasn't
>>> > changed between releases. It would also require some hacky
>>> renaming of
>>> > DLLs, as they have the version number in them.
>>> >
>>> > I assumed it had code for that stuff already. Mainly because I assumed
>>> it
>>> > supported doing pg_upgrade, which requires similar things no?
>>>
>>
> No, the installers don't support pg_upgrade directly. They ship it of
> course, and the user can manually run it, but the installers won't do that,
> and have no ability to login to a cluster except during the post-initdb
> phase.
>

Oh, I just assumed it did :)

If it doesn't, I think shipping with a modern ICU is a much smaller problem
really...


While pg_upgrade requires having the old and new cluster software in
>>> place, I don't think it helps allowing different ICU versions for each
>>> cluster.
>>
>>
>> Depends on where they are installed (and disclaimer, I don't know how the
>> windows installers do that). But as long as the ICU libraries are installed
>> in separate locations for the two versions, which I *think* they are or at
>> least used to be, it shouldn't have an effect on this in either direction.
>>
>
> They are.
>

Good. So putting both in wouldn't break things.



That argument really only holds for different versions, not for different
>> clusters of the same version. But I don't think the installers (natively)
>> supports multiple clusters of the same version anyway.
>>
>
> They don't. You'd need to manually init a new cluster and register a new
> server instance. The installer only has any knowledge of the cluster it
> sets up.
>

I'd say that's "unsupported enough" to not be a scenario one has to
consider.



>> The tricky thing is if you want to allow the user to *choose* which ICU
>> version should be used with postgres version .  Because then the user
>> might also expect an upgrade-path wherein they only upgrade the icu library
>> on an existing install...
>>
>>
>>> I guess you can argue that if you know the user is _not_ going
>>> to be using pg_upgrade, then a new ICU version should be used for the
>>> new cluster.
>>>
>>
>> Yes, that's exactly the argument I meant :) If the user got the option to
>> "pick version of ICU: , ", with a comment saying "pick old only
>> if you plan to do a pg_upgrade based upgrade of a different cluster, or if
>> this instance should participate in replication with a node using ",
>> that would probably help for the vast majority of cases. And of course, if
>> the installer through other options can determine with certainty that it's
>> going to be running pg_upgrade for the user, then it can reword the dialog
>> based on that (that is, it should still allow the user to pick the new
>> version, as long as they know that their indexes are going to need
>> reindexing)
>>
>
> That seems like a very hacky and extremely user-unfriendly approach. How
> many users are going to understand options in the installer to deal with
> that, or want to go decode the ICU filenames on their existing
> installations (which our installers may not actually know about) to figure
> out what their current version is?
>


That was more if the installer actually handles the whole chain. It clearly
doesn't today (since it doesn't support upgrades), I agree this might
definitely be overkill. But then also I don't really see the problem with
just putting a new version of ICU in with the newer versions of PostgreSQL.
That's just puts the user in the same position as they are with any other
platform wrt manual pg_upgrade runs.



>
> I would suggest that the better way to handle this would be for pg_upgrade
> to (somehow) check the ICU version on the old and new clusters and if
> there's a mismatch perform a reindex of any ICU based indexes. I suspect
> that may require that the server exposes the ICU version though. That way,
> the installers could freely upgrade the ICU version with a new major
> release.
>

Having pg_upgrade spit out a script that does reindex specifically on the
indexes required would certainly be useful in the generic case, and help
others as well.


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

Re: EDB builds Postgres 13 with an obsolete ICU version

[Dave Page]

On Mon, Aug 17, 2020 at 4:14 PM Magnus Hagander  wrote:

>
>
> On Mon, Aug 17, 2020 at 1:44 PM Dave Page  wrote:
>
>>
>>
>> On Mon, Aug 17, 2020 at 11:19 AM Magnus Hagander 
>> wrote:
>>
>>>
>>>
>>> On Fri, Aug 14, 2020 at 3:00 PM Bruce Momjian  wrote:
>>>
 On Tue, Aug 11, 2020 at 02:58:30PM +0200, Magnus Hagander wrote:
 > On Tue, Aug 4, 2020 at 11:42 AM Dave Page  wrote:
 > That would require fairly large changes to the installer to allow
 it to
 > login to the database server (whether that would work would
 be dependent on
 > how pg_hba.conf is configured), and also assumes that the ICU ABI
 hasn't
 > changed between releases. It would also require some hacky
 renaming of
 > DLLs, as they have the version number in them.
 >
 > I assumed it had code for that stuff already. Mainly because I
 assumed it
 > supported doing pg_upgrade, which requires similar things no?

>>>
>> No, the installers don't support pg_upgrade directly. They ship it of
>> course, and the user can manually run it, but the installers won't do that,
>> and have no ability to login to a cluster except during the post-initdb
>> phase.
>>
>
> Oh, I just assumed it did :)
>
> If it doesn't, I think shipping with a modern ICU is a much smaller
> problem really...
>
>
> While pg_upgrade requires having the old and new cluster software in
 place, I don't think it helps allowing different ICU versions for each
 cluster.
>>>
>>>
>>> Depends on where they are installed (and disclaimer, I don't know how
>>> the windows installers do that). But as long as the ICU libraries are
>>> installed in separate locations for the two versions, which I *think* they
>>> are or at least used to be, it shouldn't have an effect on this in either
>>> direction.
>>>
>>
>> They are.
>>
>
> Good. So putting both in wouldn't break things.
>
>
>
> That argument really only holds for different versions, not for different
>>> clusters of the same version. But I don't think the installers (natively)
>>> supports multiple clusters of the same version anyway.
>>>
>>
>> They don't. You'd need to manually init a new cluster and register a new
>> server instance. The installer only has any knowledge of the cluster it
>> sets up.
>>
>
> I'd say that's "unsupported enough" to not be a scenario one has to
> consider.
>

Agreed. Plus it's not really any different from running multiple clusters
on other OSs where we're likely to be using a vendor supplied ICU that the
user also couldn't change easily.


>
>
>
>>> The tricky thing is if you want to allow the user to *choose* which ICU
>>> version should be used with postgres version .  Because then the user
>>> might also expect an upgrade-path wherein they only upgrade the icu library
>>> on an existing install...
>>>
>>>
 I guess you can argue that if you know the user is _not_ going
 to be using pg_upgrade, then a new ICU version should be used for the
 new cluster.

>>>
>>> Yes, that's exactly the argument I meant :) If the user got the option
>>> to "pick version of ICU: , ", with a comment saying "pick old
>>> only if you plan to do a pg_upgrade based upgrade of a different cluster,
>>> or if this instance should participate in replication with a node using
>>> ", that would probably help for the vast majority of cases. And of
>>> course, if the installer through other options can determine with certainty
>>> that it's going to be running pg_upgrade for the user, then it can reword
>>> the dialog based on that (that is, it should still allow the user to pick
>>> the new version, as long as they know that their indexes are going to need
>>> reindexing)
>>>
>>
>> That seems like a very hacky and extremely user-unfriendly approach. How
>> many users are going to understand options in the installer to deal with
>> that, or want to go decode the ICU filenames on their existing
>> installations (which our installers may not actually know about) to figure
>> out what their current version is?
>>
>
>
> That was more if the installer actually handles the whole chain. It
> clearly doesn't today (since it doesn't support upgrades), I agree this
> might definitely be overkill. But then also I don't really see the problem
> with just putting a new version of ICU in with the newer versions of
> PostgreSQL. That's just puts the user in the same position as they are with
> any other platform wrt manual pg_upgrade runs.
>

Well we can certainly do that if everyone is happy in the knowledge that
it'll mean pg_upgrade users will need to reindex if they've used ICU
collations.

Sandeep; can you have someone do a test build with the latest ICU please
(for background, this would be with the Windows and Mac installers)? If
noone objects, we can push that into the v13 builds before GA. We'd also
need to update the README if we do so.


>
>
>
>>
>> I would suggest that the better way to handle this would be for
>> pg_upgrade to (somehow) check

Re: [PATCH] Covering SPGiST index

[Pavel Borisov]

With a little bugfix

вт, 11 авг. 2020 г. в 22:50, Pavel Borisov :

>
>
> вт, 11 авг. 2020 г. в 12:11, Pavel Borisov :
>
>> I added changes in documentation into the patch.
>>
>>
>> --
>> Best regards,
>> Pavel Borisov
>>
>> Postgres Professional: http://postgrespro.com
>> 
>>
>


v6-0001-Covering-SP-GiST-index-support-for-INCLUDE-column.patch
Description: Binary data

RE: Is it useful to record whether plans are generic or custom?

[legrand legrand]

 I thought it might be preferable to make a GUC to enable
 or disable this feature, but changing the hash key makes
 it harder.
>>
>>> What happens if the server was running with this option enabled and then
>>> restarted with the option disabled? Firstly two entries for the same query
>>> were stored in pgss because the option was enabled. But when it's disabled
>>> and the server is restarted, those two entries should be merged into one
>>> at the startup of server? If so, that's problematic because it may take
>>> a long time.
>>
>> What would you think about a third value for this flag to handle disabled
>> case (no need to merge entries in this situation) ?
>
> Sorry I failed to understand your point. You mean that we can have another 
> flag
> to specify whether to merge the entries for the same query at that case or 
> not?
>
> If those entries are not merged, what does pg_stat_statements return?
> It returns the sum of those entries? Or either generic or custom entry is
> returned?

The idea is to use a plan_type enum with 'generic','custom','unknown' or 
'unset'.
if tracking plan_type is disabled, then plan_type='unknown',
else plan_type can take 'generic' or 'custom' value.

I'm not proposing to merge results for plan_type when disabling or enabling its 
tracking.


>>> Therefore I think that it's better and simple to just expose the number of
>>> times generic/custom plan was chosen for each query.
>>
>> I thought this feature was mainly needed to identifiy "under optimal generic
>> plans". Without differentiating execution counters, this will be simpler but
>> useless in this case ... isn't it ?

> Could you elaborate "under optimal generic plans"? Sorry, I failed to
> understand your point.. But maybe you're thinking to use this feature to
> check which generic or custom plan is better for each query?

> I was just thinking that this feature was useful to detect the case where
> the query was executed with unpected plan mode. That is, we can detect
> the unexpected case where the query that should be executed with generic
> plan is actually executed with custom plan, and vice versa.

There are many exemples in pg lists, where users comes saying that sometimes
their query takes a (very) longer time than before, without understand why.
I some of some exemples, it is that there is a switch from custom to generic 
after
n executions, and it takes a longer time because generic plan is not as good as
custom one (I call them under optimal generic plans). If pgss keeps counters
aggregated for both plan_types, I don't see how this (under optimal) can be 
shown.
If there is a line in pgss for custom and an other for generic, then it would 
be easier
to compare.

Does this makes sence ?

Regards
PAscal

> Regards,
> --
> Fujii Masao
> Advanced Computing Technology Center
> Research and Development Headquarters
> NTT DATA CORPORATION

Re: Add information to rm_redo_error_callback()

[Alvaro Herrera]

On 2020-Aug-17, Drouvot, Bertrand wrote:

> Having this "pg_waldump" kind of format in this place
> (rm_redo_error_callback()) ensures that we'll always see the same piece of
> information during rm_redo.
> 
> I think it's good to guarantee that we'll always see the same piece of
> information (should a new RM desc() be created in the future for example),
> even if it could lead to some information overlap in some cases.

I agree.

I think we should treat the changes to remove rm_desc-specific info
items that are redundant as separate improvements that don't need to
block this patch.  They would be, at worst, only minor annoyances.
And the removal, as was said, can affect other things that we might want
to think about separately.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Include access method in listTables output

[vignesh C]

On Sat, Aug 1, 2020 at 8:12 AM vignesh C  wrote:
> > >
> > > +-- access method column should not be displayed for sequences
> > > +\ds+
> > >
> > > -  List of relations
> > >
> > >
> > > -   Schema | Name | Type | Owner | Persistence | Size | Description
> > > ++--+--+---+-+--+-
> > > +(0 rows)
> > >
> > > We can include one test for view.

I felt adding one test for view is good and added it.
Attached a new patch for the same.

I felt patch is in shape for committer to have a look at this.

Regards,
Vignesh
From 59d102e21c0261c7349fc5ad55026d0125972836 Mon Sep 17 00:00:00 2001
From: Georgios  
Date: Mon, 17 Aug 2020 22:51:17 +0530
Subject: [PATCH v6] Include access method in listTables output

Author: Georgios  
Discussion: https://www.postgresql.org/message-id/svaS1VTOEscES9CLKVTeKItjJP1EEJuBhTsA0ESOdlnbXeQSgycYwVlliL5zt8Jwcfo4ATYDXtEqsExxjkSkkhCSTCL8fnRgaCAJdr0unUg%3D%40protonmail.com
---
 doc/src/sgml/ref/psql-ref.sgml |  4 +++-
 src/bin/psql/describe.c| 14 ++-
 src/test/regress/expected/psql.out | 49 ++
 src/test/regress/sql/psql.sql  | 17 -
 4 files changed, 76 insertions(+), 8 deletions(-)

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index fc16e6c..7667687 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1183,7 +1183,9 @@ testdb=>
 columns of the table are shown, as is the presence of OIDs in the
 table, the view definition if the relation is a view, a non-default
 replica
-identity setting.
+identity setting, the access method name
+ if the relation has access
+method.
 
 
 
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index d81f157..ac6a93c 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3678,7 +3678,7 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
 	PGresult   *res;
 	printQueryOpt myopt = pset.popt;
 	int			cols_so_far;
-	bool		translate_columns[] = {false, false, true, false, false, false, false, false};
+	bool		translate_columns[] = {false, false, true, false, false, false, false, false, false};
 
 	/* If tabtypes is empty, we default to \dtvmsE (but see also command.c) */
 	if (!(showTables || showIndexes || showViews || showMatViews || showSeq || showForeign))
@@ -3751,6 +3751,12 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
 		 * to; this might change with future additions to the output columns.
 		 */
 
+		if (pset.sversion >= 12 && !pset.hide_tableam &&
+			(showTables || showMatViews || showIndexes))
+			appendPQExpBuffer(&buf,
+			  ",\n  am.amname as \"%s\"",
+			  gettext_noop("Access Method"));
+
 		/*
 		 * As of PostgreSQL 9.0, use pg_table_size() to show a more accurate
 		 * size of a table, including FSM, VM and TOAST tables.
@@ -3772,6 +3778,12 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
 	appendPQExpBufferStr(&buf,
 		 "\nFROM pg_catalog.pg_class c"
 		 "\n LEFT JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace");
+
+	if (pset.sversion >= 12 && !pset.hide_tableam &&
+		(showTables || showMatViews || showIndexes))
+		appendPQExpBufferStr(&buf,
+		 "\n LEFT JOIN pg_catalog.pg_am am ON am.oid = c.relam");
+
 	if (showIndexes)
 		appendPQExpBufferStr(&buf,
 			 "\n LEFT JOIN pg_catalog.pg_index i ON i.indexrelid = c.oid"
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 555d464..e497adf 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -2796,19 +2796,24 @@ Type| func
 
 \pset tuples_only false
 -- check conditional tableam display
--- Create a heap2 table am handler with heapam handler
+\pset expanded off
+CREATE SCHEMA conditional_tableam_display;
+CREATE ROLE	conditional_tableam_display_role;
+ALTER SCHEMA conditional_tableam_display OWNER TO conditional_tableam_display_role;
+SET search_path TO conditional_tableam_display;
 CREATE ACCESS METHOD heap_psql TYPE TABLE HANDLER heap_tableam_handler;
+SET role TO conditional_tableam_display_role;
 CREATE TABLE tbl_heap_psql(f1 int, f2 char(100)) using heap_psql;
 CREATE TABLE tbl_heap(f1 int, f2 char(100)) using heap;
 \d+ tbl_heap_psql
-   Table "public.tbl_heap_psql"
+Table "conditional_tableam_display.tbl_heap_psql"
  Column |  Type  | Collation | Nullable | Default | Storage  | Stats target | Description 
 ++---+--+-+--+--+-
  f1 | integer|   |  | | plain|  | 
  f2 | character(100) |   |  | | extended |

Re: run pgindent on a regular basis / scripted manner

[Bruce Momjian]

On Sat, Aug 15, 2020 at 01:44:34PM -0400, Tom Lane wrote:
> Andres Freund  writes:
> > Without a properly indented baseline that's hard to do, because it'll
> > cause damage all over. So I don't think we easily can start just there -
> > we'd first need to re-indent everything.
> 
> Well, we can certainly do a tree-wide re-indent anytime we're ready.
> I doubt it would be very painful right now, with so little new work
> since the last run.

Uh, I thought Tom was saying we need to reindent all branches, which
would be a big change for those tracking forks.

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

  The usefulness of a cup is in its emptiness, Bruce Lee

Re: run pgindent on a regular basis / scripted manner

[Andres Freund]

Hi,

On 2020-08-17 13:54:15 -0400, Bruce Momjian wrote:
> On Sat, Aug 15, 2020 at 01:44:34PM -0400, Tom Lane wrote:
> > Andres Freund  writes:
> > > Without a properly indented baseline that's hard to do, because it'll
> > > cause damage all over. So I don't think we easily can start just there -
> > > we'd first need to re-indent everything.
> > 
> > Well, we can certainly do a tree-wide re-indent anytime we're ready.
> > I doubt it would be very painful right now, with so little new work
> > since the last run.
> 
> Uh, I thought Tom was saying we need to reindent all branches, which
> would be a big change for those tracking forks.

I don't think he said that? He said *if* we were to reindent all
branches, forks would probably have an issue. We're already reindenting
HEAD on a regular basis (just very infrequent), so it can't be a
fundamental issue.

Greetings,

Andres Freund

Re: run pgindent on a regular basis / scripted manner

[Tom Lane]

Bruce Momjian  writes:
> On Sat, Aug 15, 2020 at 01:44:34PM -0400, Tom Lane wrote:
>> Well, we can certainly do a tree-wide re-indent anytime we're ready.
>> I doubt it would be very painful right now, with so little new work
>> since the last run.

> Uh, I thought Tom was saying we need to reindent all branches, which
> would be a big change for those tracking forks.

No, I'm not for reindenting the back branches in general.  There was
some discussion about whether to try to indent back-patched patches
to meet the conventions of the specific back branch, but I doubt
that that's worth troubling over.  I think we really only care
about whether HEAD is fully consistently indented.

regards, tom lane

Re: run pgindent on a regular basis / scripted manner

[Andres Freund]

Hi,

On 2020-08-15 13:44:34 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > One thing is that some here are actively against manually adding entries
> > to typedefs.list.
> 
> I've been of the opinion that it's pointless to do so under the current
> regime where (a) only a few people do that and (b) we only officially
> re-indent once a year anyway.  When I want to manually run pgindent,
> I always pull down a fresh typedefs.list from the buildfarm, which is
> reasonably up-to-date regardless of what people added or didn't add,
> and then add any new typedefs from my current patch to that out-of-tree
> copy.

Well, properly indenting new code still is worthwhile. And once you go
through the trouble of adding the typedefs locally, I don't really see
the reason not to also include them in the commit. Sure it'll not help
much with tree-wide re-indents, but with individual files it can still
make life less painful.

Greetings,

Andres Freund

Re: Corruption during WAL replay

[Andres Freund]

Hi,

On 2020-08-17 14:05:37 +0300, Heikki Linnakangas wrote:
> On 14/04/2020 22:04, Teja Mupparti wrote:
> > Thanks Kyotaro and Masahiko for the feedback. I think there is a
> > consensus on the critical-section around truncate,
> 
> +1

I'm inclined to think that we should do that independent of the far more
complicated fix for other related issues.


> > but I just want to emphasize the need for reversing the order of the
> > dropping the buffers and the truncation.
> > 
> >   Repro details (when full page write = off)
> > 
> >           1) Page on disk has empty LP 1, Insert into page LP 1
> >           2) checkpoint START (Recovery REDO eventually starts here)
> >           3) Delete all rows on the page (page is empty now)
> >           4) Autovacuum kicks in and truncates the pages
> >                   DropRelFileNodeBuffers - Dirty page NOT written, LP 1
> > on disk still empty
> >           5) Checkpoint completes
> >           6) Crash
> >           7) smgrtruncate - Not reached (this is where we do the
> > physical truncate)
> > 
> >   Now the crash-recovery starts
> > 
> >           Delete-log-replay (above step-3) reads page with empty LP 1
> > and the delete fails with PANIC (old page on disk with no insert)
> > 
> > Doing recovery, truncate is even not reached, a WAL replay of the
> > truncation will happen in the future but the recovery fails (repeatedly)
> > even before reaching that point.
> 
> Hmm. I think simply reversing the order of DropRelFileNodeBuffers() and
> truncating the file would open a different issue:
> 
>   1) Page on disk has empty LP 1, Insert into page LP 1
>   2) checkpoint START (Recovery REDO eventually starts here)
>   3) Delete all rows on the page (page is empty now)
>   4) Autovacuum kicks in and starts truncating
>   5) smgrtruncate() truncates the file
>   6) checkpoint writes out buffers for pages that were just truncated away,
> expanding the file again.
> 
> Your patch had a mechanism to mark the buffers as io-in-progress before
> truncating the file to fix that, but I'm wary of that approach. Firstly, it
> requires scanning the buffers that are dropped twice, which can take a long
> time.

I was thinking that we'd keep track of all the buffers marked as "in
progress" that way, avoiding the second scan.

It's also worth keeping in mind that this code is really only relevant
for partial truncations, which don't happen at the same frequency as
transactional truncations.


> I remember that people have already complained that
> DropRelFileNodeBuffers() is slow, when it has to scan all the buffers
> once.

But that's when dropping many relations, normally. E.g. at the end of a
regression test.


> More importantly, abusing the BM_IO_INPROGRESS flag for this seems
> bad. For starters, because you're not holding buffer's I/O lock, I
> believe the checkpointer would busy-wait on the buffers until the
> truncation has completed. See StartBufferIO() and AbortBufferIO().

I think we should apply Robert's patch that makes io locks into
condition variables. Then we can fairly easily have many many buffers io
locked.  Obviously there's some issues with doing so in the back
branches :(

I'm working on an AIO branch, and that also requires to be able to mark
multiple buffers as in-progress, FWIW.


> Perhaps a better approach would be to prevent the checkpoint from
> completing, until all in-progress truncations have completed. We have a
> mechanism to wait out in-progress commits at the beginning of a checkpoint,
> right after the redo point has been established. See comments around the
> GetVirtualXIDsDelayingChkpt() function call in CreateCheckPoint(). We could
> have a similar mechanism to wait out the truncations before *completing* a
> checkpoint.

What I outlined earlier *is* essentially a way to do so, by preventing
checkpointing from finishing the buffer scan while a dangerous state
exists.

Greetings,

Andres Freund

Re: EDB builds Postgres 13 with an obsolete ICU version

[Bruce Momjian]

On Mon, Aug 17, 2020 at 04:55:13PM +0100, Dave Page wrote:
> That was more if the installer actually handles the whole chain. It 
> clearly
> doesn't today (since it doesn't support upgrades), I agree this might
> definitely be overkill. But then also I don't really see the problem with
> just putting a new version of ICU in with the newer versions of 
> PostgreSQL.
> That's just puts the user in the same position as they are with any other
> platform wrt manual pg_upgrade runs.
> 
> Well we can certainly do that if everyone is happy in the knowledge that it'll
> mean pg_upgrade users will need to reindex if they've used ICU collations.
> 
> Sandeep; can you have someone do a test build with the latest ICU please (for
> background, this would be with the Windows and Mac installers)? If noone
> objects, we can push that into the v13 builds before GA. We'd also need to
> update the README if we do so.

Woh, we don't have any support in pg_upgrade to reindex just indexes
that use ICU collations, and frankly, if they have to reindex, they
might decide that they should just do pg_dump/reload of their cluster at
that point because pg_upgrade is going to be very slow, and they will be
surprised.  I can see a lot more people being disappointed by this than
will be happy to have Postgres using a newer ICU library.

Also, is it the ICU library version we should be tracking for reindex,
or each _collation_ version?  If the later, do we store the collation
version for each index?

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

  The usefulness of a cup is in its emptiness, Bruce Lee

Re: Optimising compactify_tuples()

[Peter Geoghegan]

On Mon, Aug 17, 2020 at 4:01 AM Thomas Munro  wrote:
> While writing this email, I checked the archives and discovered that a
> couple of other people have complained about this hot spot before and
> proposed faster sorts already[2][3], and then there was a wide ranging
> discussion of various options which ultimately seemed to conclude that
> we should do what I'm now proposing ... and then it stalled.

I saw compactify_tuples() feature prominently in profiles when testing
the deduplication patch. We changed the relevant nbtdedup.c logic to
use a temp page rather than incrementally rewriting the authoritative
page in shared memory, which sidestepped the problem.

I definitely think that we should have something like this, though.
It's a relatively easy win. There are plenty of workloads that spend
lots of time on pruning.

-- 
Peter Geoghegan

Re: [BUG] Error in BRIN summarization

[Alvaro Herrera]

On 2020-Aug-15, Tom Lane wrote:

> hyrax's latest report suggests that this patch has issues under
> CLOBBER_CACHE_ALWAYS:
> 
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hyrax&dt=2020-08-13%2005%3A09%3A58
> 
> Hard to tell whether there's an actual bug there or just test instability,
> but either way it needs to be resolved.

Hmm, the only explanation I can see for this is that autovacuum managed
to summarize the range before the test script did it.  So the solution
would simply be to disable autovacuum for the table across the whole
script.

I'm running the scripts and dependencies to verify that fix, but under
CLOBBER_CACHE_ALWAYS that takes quite a bit.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: run pgindent on a regular basis / scripted manner

[Tom Lane]

Andres Freund  writes:
> On 2020-08-15 13:44:34 -0400, Tom Lane wrote:
>> Andres Freund  writes:
>>> One thing is that some here are actively against manually adding entries
>>> to typedefs.list.

>> I've been of the opinion that it's pointless to do so under the current
>> regime where (a) only a few people do that and (b) we only officially
>> re-indent once a year anyway.  When I want to manually run pgindent,
>> I always pull down a fresh typedefs.list from the buildfarm, which is
>> reasonably up-to-date regardless of what people added or didn't add,
>> and then add any new typedefs from my current patch to that out-of-tree
>> copy.

> Well, properly indenting new code still is worthwhile. And once you go
> through the trouble of adding the typedefs locally, I don't really see
> the reason not to also include them in the commit.

Yeah, I'm quite religious about making sure my commits have been through
pgindent already (mainly to reduce subsequent "git blame" noise).
However, relying on manual updates to the in-tree typedefs.list only
works if every committer is equally religious about it.  They're not;
else we'd not be having this discussion.  The workflow I describe above
is not dependent on how careful everybody else is, which is why I
prefer it.

I think that the main new idea that's come out of this thread so far
is that very frequent reindents would be as good, maybe better, as
once-a-year reindents in terms of the amount of rebasing pain they
cause for not-yet-committed patches.  If we can fix it so that any
mis-indented commit is quickly corrected, then rebasing would only be
needed in places that were changed anyway.  So it seems like that
would be OK as a new project policy if we can make it happen.

However, I don't see any way to make it happen like that without
more-or-less automated reindents and typedefs.list updates,
and that remains a bit scary.

I did just have an idea that seems to ameliorate the scariness
a good bit: allow the reindent bot to auto-commit its results
only if the only lines it's changing are ones that were touched
by commits since the last auto-reindent.  Otherwise punt and ask
a human to review the results.  Not sure how hard that is to
implement, though.

Another good safety check would be to not proceed unless the latest
typedefs list available from the buildfarm is newer than the last
commit --- then we won't mess up recent commits whose typedefs are
not in the list yet.

regards, tom lane

Re: Improving connection scalability: GetSnapshotData()

[Alvaro Herrera]

On 2020-Aug-16, Peter Geoghegan wrote:

> On Sun, Aug 16, 2020 at 2:11 PM Andres Freund  wrote:
> > For the first, one issue is that there's no obviously good candidate for
> > an uninitialized xid. We could use something like FrozenTransactionId,
> > which may never be in the procarray. But it's not exactly pretty.
> 
> Maybe it would make sense to mark the fields as inaccessible or
> undefined to Valgrind. That has advantages and disadvantages that are
> obvious.

... and perhaps making Valgrind complain about it is sufficient.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [BUG] Error in BRIN summarization

[Alvaro Herrera]

On 2020-Aug-17, Alvaro Herrera wrote:

> Hmm, the only explanation I can see for this is that autovacuum managed
> to summarize the range before the test script did it.  So the solution
> would simply be to disable autovacuum for the table across the whole
> script.
> 
> I'm running the scripts and dependencies to verify that fix, but under
> CLOBBER_CACHE_ALWAYS that takes quite a bit.

I ran a subset of tests a few times, but was unable to reproduce the
problem.  I'll just push this to all branches and hope for the best.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 6e70443edacfc86674995c0c10ade0aec7a4fddf Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Mon, 17 Aug 2020 16:20:06 -0400
Subject: [PATCH] Disable autovacuum for BRIN test table

This should improve stability in the tests.

Per buildfarm member hyrax (CLOBBER_CACHE_ALWAYS) via Tom Lane.

Discussion: https://postgr.es/m/871534.1597503...@sss.pgh.pa.us
---
 src/test/regress/expected/brin.out | 2 +-
 src/test/regress/sql/brin.sql  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/test/regress/expected/brin.out b/src/test/regress/expected/brin.out
index 0b14c73fc6..18403498df 100644
--- a/src/test/regress/expected/brin.out
+++ b/src/test/regress/expected/brin.out
@@ -26,7 +26,7 @@ CREATE TABLE brintest (byteacol bytea,
 	int4rangecol int4range,
 	lsncol pg_lsn,
 	boxcol box
-) WITH (fillfactor=10);
+) WITH (fillfactor=10, autovacuum_enabled=off);
 INSERT INTO brintest SELECT
 	repeat(stringu1, 8)::bytea,
 	substr(stringu1, 1, 1)::"char",
diff --git a/src/test/regress/sql/brin.sql b/src/test/regress/sql/brin.sql
index 1289e76ecb..d1a82474f3 100644
--- a/src/test/regress/sql/brin.sql
+++ b/src/test/regress/sql/brin.sql
@@ -26,7 +26,7 @@ CREATE TABLE brintest (byteacol bytea,
 	int4rangecol int4range,
 	lsncol pg_lsn,
 	boxcol box
-) WITH (fillfactor=10);
+) WITH (fillfactor=10, autovacuum_enabled=off);
 
 INSERT INTO brintest SELECT
 	repeat(stringu1, 8)::bytea,
-- 
2.20.1

Re: doc examples for pghandler

[Mark Wong]

On Fri, Aug 14, 2020 at 02:25:52PM +0900, Michael Paquier wrote:
> On Tue, Aug 11, 2020 at 01:01:10PM -0700, Mark Wong wrote:
> > Ah, right.  For the moment I've added some empty conditionals for
> > trigger and event trigger handling.
> > 
> > I've created a new entry in the commitfest app. [1]  I'll keep at it. :)
> 
> Thanks for the patch.  I have reviewed and reworked it as the
> attached.  Some comments below.
> 
> +PGFILEDESC = "PL/Sample - procedural language"
> +
> +REGRESS = create_pl create_func select_func
> +
> +EXTENSION = plsample
> +EXTVERSION = 0.1
> 
> This makefile has a couple of mistakes, and can be simplified a lot:
> - make check does not work, as you forgot a PGXS part.
> - MODULES can just be used as there is only one file (forgot WIN32RES
> in OBJS for example)
> - DATA does not need the .control file.
> 
> .gitignore was missing.
> 
> We could just use 1.0 instead of 0.1 for the version number.  That's
> not a big deal one way or another, but 1.0 is more consistent with the
> other modules.
> 
> plsample--1.0.sql should complain if attempting to load the file from
> psql.  Also I have cleaned up the README.
> 
> Not sure that there is a point in having three different files for the
> regression tests.  create_pl.sql is actually not necessary as you
> can do the same with CREATE EXTENSION.
> 
> The header list of plsample.c was inconsistent with the style used
> normally in modules, and I have extended a bit the handler function so
> as we return a result only if the return type of the procedure is text
> for the source text of the function, tweaked the results a bit, etc.
> There was a family of small issues, like using ALLOCSET_SMALL_SIZES
> for the context creation.  We could of course expand the sample
> handler more in the future to check for pseudotype results, have a
> validator, but that could happen later, if necessary.

Thanks for fixing all of that up for me.  I did have a couple mental
notes for a couple of the last items. :)

I've attached a small word diff to suggest a few different words to use
in the README, if that sounds better?

Regards,
Mark
-- 
Mark Wong
2ndQuadrant - PostgreSQL Solutions for the Enterprise
https://www.2ndQuadrant.com/
diff --git a/src/test/modules/plsample/README b/src/test/modules/plsample/README
index 7d44d7b3f2..afe3fa6402 100644
--- a/src/test/modules/plsample/README
+++ b/src/test/modules/plsample/README
@@ -2,5 +2,5 @@ PL/Sample
=

PL/Sample is an example template of procedural-language handler.  It is
[-kept-]a [-maximum simple, and-]{+simple implementation, yet+} demonstrates 
some of the things that can be
done to build a fully functional procedural-language handler.

Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits

[Alvaro Herrera]

On 2020-Aug-14, Ibrar Ahmed wrote:

> The table used for the test contains three columns (integer, text,
> varchar).
> The total number of rows is 1000 in total.
> 
> Unpatched (Master: 92c12e46d5f1e25fc85608a6d6a19b8f5ea02600)
> COPY: 9069.432 ms vacuum; 2567.961ms
> COPY: 9004.533 ms vacuum: 2553.075ms
> COPY: 8832.422 ms vacuum: 2540.742ms
> 
> Patched (Master: 92c12e46d5f1e25fc85608a6d6a19b8f5ea02600)
> COPY: 10031.723 ms vacuum: 127.524 ms
> COPY: 9985.109  ms vacuum: 39.953 ms
> COPY: 9283.373  ms vacuum: 37.137 ms
> 
> Time to take the copy slightly increased but the vacuum time significantly
> decrease.

"Slightly"?  It seems quite a large performance drop to me -- more than
10%.  Where is that time being spent?  Andres said in [1] that he
thought the performance shouldn't be affected noticeably, but this
doesn't seem to hold true.  As I understand, the idea was that there
would be little or no additional WAL records .. only flags in the
existing record.  So what is happening?

[1] https://postgr.es/m/20190408010427.4l63qr7h2fjcy...@alap3.anarazel.de

Also, when Andres posted this patch first, he said this was only for
heap_multi_insert because it was a prototype.  But I think we expect
that the table_insert path (CIM_SINGLE mode in copy) should also receive
that treatment.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: use pg_get_functiondef() in pg_dump

[Corey Huinker]

>
> I'm sure there's a lot of folks who'd like to see more of the logic we
> have in pg_dump for building objects from the catalog available to more
> tools through libpgcommon- psql being one of the absolute first
> use-cases for exactly that (there's certainly no shortage of people
> who've asked how they can get a CREATE TABLE statement for a table by
> using psql...).
>

I count myself among those folks (see
https://www.postgresql.org/message-id/CADkLM%3DfxfsrHASKk_bY_A4uomJ1Te5MfGgD_rwwQfV8wP68ewg%40mail.gmail.com
for
discussion of doing DESCRIBE and SHOW CREATE-ish functions either on server
side or client side).

I'm all for having this as "just" as set of pg_get_*def functions, because
they allow for the results to be used in queries. Granted, the shape of the
result set may not be stable, but that's the sort of thing we can warn for
the same way we have warnings for changes to pg_stat_activity. At that
point any DESCRIBE/SHOW CREATE server side functions essentially become
just shells around the pg_get_*def(), with no particular requirement to
make those new commands work inside a SELECT.

Would it be totally out of left field to have the functions have an
optional "version" parameter, defaulted to null, that would be used to give
backwards compatible results if and when we do make a breaking change?

Re: EDB builds Postgres 13 with an obsolete ICU version

[Michael Paquier]

On Mon, Aug 17, 2020 at 02:23:57PM -0400, Bruce Momjian wrote:
> Also, is it the ICU library version we should be tracking for reindex,
> or each _collation_ version?  If the later, do we store the collation
> version for each index?

You need to store the collation version(s) for each index.  This
thread deals with the problem:
https://commitfest.postgresql.org/29/2367/
https://www.postgresql.org/message-id/CAEepm%3D0uEQCpfq_%2BLYFBdArCe4Ot98t1aR4eYiYTe%3DyavQygiQ%40mail.gmail.com

That's not all of it as you would still need some filtering
capabilities in the backend to reindex only the collation-sensitive
indexes with a reindex, but that's one step forward into being able to
do that.
--
Michael


signature.asc
Description: PGP signature

Re: EDB builds Postgres 13 with an obsolete ICU version

[Bruce Momjian]

On Tue, Aug 18, 2020 at 09:44:35AM +0900, Michael Paquier wrote:
> On Mon, Aug 17, 2020 at 02:23:57PM -0400, Bruce Momjian wrote:
> > Also, is it the ICU library version we should be tracking for reindex,
> > or each _collation_ version?  If the later, do we store the collation
> > version for each index?
> 
> You need to store the collation version(s) for each index.  This
> thread deals with the problem:
> https://commitfest.postgresql.org/29/2367/
> https://www.postgresql.org/message-id/CAEepm%3D0uEQCpfq_%2BLYFBdArCe4Ot98t1aR4eYiYTe%3DyavQygiQ%40mail.gmail.com
> 
> That's not all of it as you would still need some filtering
> capabilities in the backend to reindex only the collation-sensitive
> indexes with a reindex, but that's one step forward into being able to
> do that.

Oh, we don't even have the version in the system catalogs yet?  I guess
when pg_upgrade runs create_index we could grab it then, and for the
pg_upgrade _after_ that, do the checks.  This seems like it is years
away from being useful.

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

  The usefulness of a cup is in its emptiness, Bruce Lee

Re: Terminate the idle sessions

[Kyotaro Horiguchi]

Hello.

At Mon, 17 Aug 2020 19:28:10 +0530, Bharath Rupireddy 
 wrote in 
> On Fri, Aug 14, 2020 at 1:32 PM Li Japin  wrote:
> >
> > On Aug 14, 2020, at 2:15 PM, Bharath Rupireddy <
> bharath.rupireddyforpostg...@gmail.com> wrote:
> >
> > I think, since the idle_session_timeout is by default disabled, we
> > have no problem. My thought is what if a user enables the
> > feature(knowingly or unknowingly) on the remote backend? If the user
> > knows about the above scenario, that may be fine. On the other hand,
> > either we can always the feature on the remote backend(at the
> > beginning of the remote txn, like we set for some other configuration
> > settings see - configure_remote_session() in connection.c) or how
> > about mentioning the above scenario in this feature documentation?
> >
> > Though we can disable the idle_session_timeout when using postgres_fdw,
> > there still has locally cached connection cache entries when the remote
> sessions
> > terminated by accident.  AFAIK, you have provided a patch to solve this
> > problem, and it is in current CF [1].
> >
> > [1] - https://commitfest.postgresql.org/29/2651/
> >
> 
> Yes, that solution can retry the cached connections at only the beginning
> of the remote txn and not at the middle of the remote txn and that makes
> sense as we can not retry connecting to a different remote backend in the
> middle of a remote txn.
> 
> +1 for disabling the idle_session_timeout feature in case of postgres_fdw.
> This can avoid the remote backends to timeout during postgres_fdw usages.

The same already happens for idle_in_transaction_session_timeout and
we can use "ALTER ROLE/DATABASE SET" to dislable or loosen them, it's
a bit cumbersome, though. I don't think we should (at least
implicitly) disable those timeouts ad-hockerly for postgres_fdw.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Terminate the idle sessions

[Li Japin]

On Aug 18, 2020, at 9:19 AM, Kyotaro Horiguchi 
mailto:horikyota@gmail.com>> wrote:

The same already happens for idle_in_transaction_session_timeout and
we can use "ALTER ROLE/DATABASE SET" to dislable or loosen them, it's
a bit cumbersome, though. I don't think we should (at least
implicitly) disable those timeouts ad-hockerly for postgres_fdw.

+1.

Re: doc examples for pghandler

[Michael Paquier]

On Mon, Aug 17, 2020 at 04:30:07PM -0700, Mark Wong wrote:
> I've attached a small word diff to suggest a few different words to use
> in the README, if that sounds better?

Sounds good to me.  So applied with those changes.  It is really
tempting to add an example of validator (one simple thing would be to
return an error if trying to use TRIGGEROID or EVTTRIGGEROID), but
that may not be the best thing to do for a test module.  And what we
have here is already much better than the original docs.
--
Michael


signature.asc
Description: PGP signature

Re: display offset along with block number in vacuum errors

[Amit Kapila]

On Mon, Aug 17, 2020 at 11:38 AM Masahiko Sawada
 wrote:
>
> On Sat, 15 Aug 2020 at 12:19, Amit Kapila  wrote:
> >
> > The reason why I have not included heap_page_prune related change in
> > the patch is that I don't want to sprinkle this in every possible
> > function (code path) called via vacuum especially if the probability
> > of an error in that code path is low. But, I am fine if you and or
> > others think that it is a good idea to update offset in
> > heap_page_prune as well.
>
> I agree to not try sprinkling it many places than necessity.
>
> Regarding heap_page_prune(), I'm concerned a bit that
> heap_page_prune() is typically the first function to check the tuple
> visibility within the vacuum code. I've sometimes observed an error
> with the message like "DETAIL: could not open file “pg_xact/00AB”: No
> such file or directory" due to a tuple header corruption. I suspect
> this message was emitted while checking tuple visibility in
> heap_page_prune(). So I guess the likelihood of an error in that code
> is not so low.
>

Fair point.

> On the other hand, if we want to update the offset number in
> heap_page_prune() we will need to expose some vacuum structs defined
> as a static including LVRelStats.
>

I don't think we need to expose LVRelStats. We can just pass the
address of vacrelstats->offset_num to achieve what we want. I have
tried that and it works, see the
v6-0002-additinal-error-context-information-in-heap_page_ patch
attached. Do you see any problem with it?

-- 
With Regards,
Amit Kapila.


v6-0001-Add-additional-information-in-vacuum-errcontext.patch
Description: Binary data


v6-0002-additinal-error-context-information-in-heap_page_.patch
Description: Binary data

Print logical WAL message content

[Ashutosh Bapat]

Hi,
Right now pg_waldump just prints whether the message is transactional
or not and its size. That doesn't help much to understand the message
itself. If it prints the contents of a logical WAL message, it helps
debugging logical replication related problems. Prefix is a
null-terminated ASCII string, so no problem printing that. Even the
contents can be printed as a series of hex bytes. Here's a patch to do
that.

I tested this manually as below

postgres=# select pg_logical_emit_message(false, 'some_prefix', 'some
message'::text);
 pg_logical_emit_message
-
 0/1570658
(1 row)

$> pg_waldump --start 0/1570600 -p data/
first record is after 0/1570600, at 0/1570608, skipping over 8 bytes
rmgr: LogicalMessage len (rec/tot): 74/74, tx:  0,
lsn: 0/01570608, prev 0/015705D0, desc: MESSAGE nontransactional
message size 12 bytes, prefix some_prefix; mesage: 73 6F 6D 65 20 6D
65 73 73 61 67 65
rmgr: Standby len (rec/tot): 50/50, tx:  0, lsn:
0/01570658, prev 0/01570608, desc: RUNNING_XACTS nextXid 504
latestCompletedXid 503 oldestRunningXid 504
pg_waldump: fatal: error in WAL record at 0/1570658: invalid record
length at 0/1570690: wanted 24, got 0

I didn't find any tests for pg_waldump to test its output, so haven't
added one in the patch.

-- 
Best Wishes,
Ashutosh Bapat
From 1547277944f7aceb1d5b0a3ae46ec2acf02f3b06 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat 
Date: Tue, 18 Aug 2020 11:05:29 +0530
Subject: [PATCH] Print prefix and logical WAL message content in pg_waldump

Print logical WAL message prefix which is a null terminated ASCII
string. Print the actual message as a space separated hex byte string
since it may contain binary data. This is useful for debugging purposes.

Ashutosh Bapat
---
 src/backend/access/rmgrdesc/logicalmsgdesc.c | 18 --
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/src/backend/access/rmgrdesc/logicalmsgdesc.c b/src/backend/access/rmgrdesc/logicalmsgdesc.c
index bff298c928..fdd03362e7 100644
--- a/src/backend/access/rmgrdesc/logicalmsgdesc.c
+++ b/src/backend/access/rmgrdesc/logicalmsgdesc.c
@@ -24,10 +24,24 @@ logicalmsg_desc(StringInfo buf, XLogReaderState *record)
 	if (info == XLOG_LOGICAL_MESSAGE)
 	{
 		xl_logical_message *xlrec = (xl_logical_message *) rec;
+		/*
+		 * Per LogLogicalMessage() actual logical message follows a null-terminated prefix of length
+		 * prefix_size.
+		 */
+		char   *prefix = xlrec->message;
+		char   *message = xlrec->message + xlrec->prefix_size;
+		int		cnt;
+		char   *sep = "";
 
-		appendStringInfo(buf, "%s message size %zu bytes",
+		appendStringInfo(buf, "%s message size %zu bytes, prefix %s; mesage: ",
 		 xlrec->transactional ? "transactional" : "nontransactional",
-		 xlrec->message_size);
+		 xlrec->message_size, prefix);
+		/* Write actual message as a series of hex bytes. */
+		for (cnt = 0; cnt < xlrec->message_size; cnt++)
+		{
+			appendStringInfo(buf, "%s%02X", sep, (unsigned char)message[cnt]);
+			sep = " ";
+		}
 	}
 }
 
-- 
2.17.1

Re: [Patch] Optimize dropping of relation buffers using dlist

[Amit Kapila]

On Fri, Aug 7, 2020 at 9:33 AM Tom Lane  wrote:
>
> Amit Kapila  writes:
> > On Sat, Aug 1, 2020 at 1:53 AM Andres Freund  wrote:
> >> We could also just use pg_class.relpages. It'll probably mostly be
> >> accurate enough?
>
> > Don't we need the accurate 'number of blocks' if we want to invalidate
> > all the buffers? Basically, I think we need to perform BufTableLookup
> > for all the blocks in the relation and then Invalidate all buffers.
>
> Yeah, there is no room for "good enough" here.  If a dirty buffer remains
> in the system, the checkpointer will eventually try to flush it, and fail
> (because there's no file to write it to), and then checkpointing will be
> stuck.  So we cannot afford to risk missing any buffers.
>

Today, again thinking about this point it occurred to me that during
recovery we can reliably find the relation size and after Thomas's
recent commit c5315f4f44 (Cache smgrnblocks() results in recovery), we
might not need to even incur the cost of lseek. Why don't we fix this
first for 'recovery' (by following something on the lines of what
Andres suggested) and then later once we have a generic mechanism for
"caching the relation size" [1], we can do it for non-recovery paths.
I think that will at least address the reported use case with some
minimal changes.

[1] - 
https://www.postgresql.org/message-id/CAEepm%3D3SSw-Ty1DFcK%3D1rU-K6GSzYzfdD4d%2BZwapdN7dTa6%3DnQ%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

Re: psql: add \si, \sm, \st and \sr functions to show CREATE commands for indexes, matviews, triggers and tables

[a . pervushina]

Anna Akenteva wrote 2020-08-11 13:37:

About the patch:

1) There is some code duplication for the exec_command_[sm|si|st|sr]
functions. Plus, it seems weird to separate sm (show matview) from sv
(show view). Perhaps it would be more convenient to combine some of
the code? Maybe by editing the already-existing exec_command_sf_sv()
function.


I've combined most of the functions into one, as the code was mostly 
duplicated. Had to change the argument from is_func to object type, 
because the number of values has increased. I've attached a patch with 
those changes.diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 560eacc7f0c..3faac9e25a6 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -49,7 +49,11 @@
 typedef enum EditableObjectType
 {
 	EditableFunction,
-	EditableView
+	EditableView,
+	EditableMatview,
+	EditableIndex,
+	EditableTrigger,
+	EditableTable
 } EditableObjectType;
 
 /* local function declarations */
@@ -117,8 +121,10 @@ static backslashResult exec_command_s(PsqlScanState scan_state, bool active_bran
 static backslashResult exec_command_set(PsqlScanState scan_state, bool active_branch);
 static backslashResult exec_command_setenv(PsqlScanState scan_state, bool active_branch,
 		   const char *cmd);
-static backslashResult exec_command_sf_sv(PsqlScanState scan_state, bool active_branch,
-		  const char *cmd, bool is_func);
+static backslashResult exec_command_sf_sv_sm_si_sr(PsqlScanState scan_state, bool active_branch,
+		  const char *cmd, EditableObjectType type);
+static backslashResult exec_command_st(PsqlScanState scan_state, bool active_branch,
+	   const char *cmd);
 static backslashResult exec_command_t(PsqlScanState scan_state, bool active_branch);
 static backslashResult exec_command_T(PsqlScanState scan_state, bool active_branch);
 static backslashResult exec_command_timing(PsqlScanState scan_state, bool active_branch);
@@ -154,6 +160,8 @@ static bool do_shell(const char *command);
 static bool do_watch(PQExpBuffer query_buf, double sleep);
 static bool lookup_object_oid(EditableObjectType obj_type, const char *desc,
 			  Oid *obj_oid);
+static bool lookup_trigger_oid(const char *table_name, const char *trigger_name,
+  Oid *obj_oid);
 static bool get_create_object_cmd(EditableObjectType obj_type, Oid oid,
   PQExpBuffer buf);
 static int	strip_lineno_from_objdesc(char *obj);
@@ -381,9 +389,17 @@ exec_command(const char *cmd,
 	else if (strcmp(cmd, "setenv") == 0)
 		status = exec_command_setenv(scan_state, active_branch, cmd);
 	else if (strcmp(cmd, "sf") == 0 || strcmp(cmd, "sf+") == 0)
-		status = exec_command_sf_sv(scan_state, active_branch, cmd, true);
+		status = exec_command_sf_sv_sm_si_sr(scan_state, active_branch, cmd, EditableFunction);
 	else if (strcmp(cmd, "sv") == 0 || strcmp(cmd, "sv+") == 0)
-		status = exec_command_sf_sv(scan_state, active_branch, cmd, false);
+		status = exec_command_sf_sv_sm_si_sr(scan_state, active_branch, cmd, EditableView);
+	else if (strcmp(cmd, "sm") == 0 || strcmp(cmd, "sm+") == 0)
+		status = exec_command_sf_sv_sm_si_sr(scan_state, active_branch, cmd, EditableMatview);
+	else if (strcmp(cmd, "si") == 0 || strcmp(cmd, "si+") == 0)
+		status = exec_command_sf_sv_sm_si_sr(scan_state, active_branch, cmd, EditableIndex);
+	else if (strcmp(cmd, "st") == 0 || strcmp(cmd, "st+") == 0)
+		status = exec_command_st(scan_state, active_branch, cmd);
+	else if (strcmp(cmd, "sr") == 0 || strcmp(cmd, "sr+") == 0)
+		status = exec_command_sf_sv_sm_si_sr(scan_state, active_branch, cmd, EditableTable);
 	else if (strcmp(cmd, "t") == 0)
 		status = exec_command_t(scan_state, active_branch);
 	else if (strcmp(cmd, "T") == 0)
@@ -2318,11 +2334,11 @@ exec_command_setenv(PsqlScanState scan_state, bool active_branch,
 }
 
 /*
- * \sf/\sv -- show a function/view's source code
+ * \sf/\sv/\sm/\si/\sr -- show a function/view's/matview's/index/table's source code
  */
 static backslashResult
-exec_command_sf_sv(PsqlScanState scan_state, bool active_branch,
-   const char *cmd, bool is_func)
+exec_command_sf_sv_sm_si_sr(PsqlScanState scan_state, bool active_branch,
+   const char *cmd, EditableObjectType type)
 {
 	backslashResult status = PSQL_CMD_SKIP_LINE;
 
@@ -2332,39 +2348,60 @@ exec_command_sf_sv(PsqlScanState scan_state, bool active_branch,
 		PQExpBuffer buf;
 		char	   *obj_desc;
 		Oid			obj_oid = InvalidOid;
-		EditableObjectType eot = is_func ? EditableFunction : EditableView;
 
 		buf = createPQExpBuffer();
 		obj_desc = psql_scan_slash_option(scan_state,
 		  OT_WHOLE_LINE, NULL, true);
-		if (pset.sversion < (is_func ? 80400 : 70400))
+		if ((pset.sversion < 80400 && type == EditableFunction) || (pset.sversion < 70400 && type == EditableView))
 		{
 			char		sverbuf[32];
 
 			formatPGVersionNumber(pset.sversion, false,
   sverbuf, sizeof(sverbuf));
-			if (is_func)
-pg_log_error("The server (version %s) does not support showing function sour

Re: Display individual query in pg_stat_activity

[Masahiro Ikeda]

Hi,


I've attached a patch to display individual query in the
pg_stat_activity query field when multiple SQL statements are
currently displayed.

Motivation:

When multiple statements are displayed then we don’t know which
one is currently running.

I'm not sure I'd want that to happen, as it could make it much
harder to track the activity back to a query in the application
layer or server logs.

Perhaps a separate field could be added for the current statement,
or a value to indicate what the current statement number in the
query is?


As a user, I think this feature is useful to users.

It would be nice that pg_stat_activity also show currently running query
in a user defined function(PL/pgSQL) .

I understood that this patch is not for user defined functions.
Please let me know if it's better to make another thread.

In general, PL/pgSQL functions have multiple queries,
and users want to know the progress of query execution, doesn't it?

--
Masahiro Ikeda
NTT DATA CORPORATION