Re: LD_LIBRARY_PATH_RPATH

[Andrew Gierth]

> "Tom" == Tom Lane  writes:

 >> At least on my FreeBSD 11 box, the current definition of
 >> $(with_temp_install) is not sufficient to ensure that the various
 >> .so files are loaded from tmp_install and not from the compiled
 >> rpath (which will be the final install dir, which may of course
 >> contain old libraries).

 >> LD_LIBRARY_PATH_RPATH=1 fixes this (by giving LD_LIBRARY_PATH
 >> priority over the DT_RPATH tag in the object).

 >> Is this also an issue on any other platforms?

 Tom> Hm. Can't reproduce that on current NetBSD or macOS; don't have
 Tom> OpenBSD booted up to try right now.

The OpenBSD manpage I found says that they give LD_LIBRARY_PATH priority
over DT_RPATH, so I guess it's not an issue there.

 Tom> However, if it helps on some platforms, I can't see much harm in
 Tom> adding it in the relevant places. It should be a no-op everywhere
 Tom> else.

Is there some reason why ld_library_path_var is defined using a bunch of
$(if) constructs rather than putting the value (if not LD_LIBRARY_PATH)
in the individual port makefiles?

-- 
Andrew (irc:RhodiumToad)

RE: reloption to prevent VACUUM from truncating empty pages at the end of relation

[Tsunakawa, Takayuki]

From: Jamison, Kirk/ジャミソン カーク
> I wonder if there is a better reloption name for shrink_enabled.
> (truncate_enabled, vacuum_enabled? Hmm. No?)
> On the other hand, shrink_enabled seems to describe well what it's supposed
> to do when vacuuming tables.
> Besides there's a similarly-named autovacuum_enabled option.

Yeah, I used vacuum_truncation_enabled at first.  But I thought shrink is 
better because it represents the final effect from the user perspective, while 
truncation is the system action to reach the desired state.


> I think if most seem to agree to have this solution in place
> and to review this further and cover what might be missing,
> then shall we register this to next CF?

I've already done it.


Regards
Takayuki Tsunakawa

RE: reloption to prevent VACUUM from truncating empty pages at the end of relation

[Jamison, Kirk]

On February 1, 2019, Tsunakawa, Takayuki wrote: 

>> As most people seem to agree adding the reloption, here's the patch.  
>> It passes make check, and works like this:

>Sorry, I forgot to include the modified header file.  Revised patch attached.

Thank you for this.
I applied the patch. It builds successfully, and passed the regression tests.
I also tried testing with the parameter when its enabled and disabled,
and it works as intended for CREATE TABLE and ALTER TABLE a SET (shrink_enabled 
= on/off) and RESET (shrink_enabled).
I have yet to test the performance benchmark.

I wonder if there is a better reloption name for shrink_enabled. 
(truncate_enabled, vacuum_enabled? Hmm. No?)
On the other hand, shrink_enabled seems to describe well what it's supposed to 
do when vacuuming tables.
Besides there's a similarly-named autovacuum_enabled option.

I think if most seem to agree to have this solution in place
and to review this further and cover what might be missing,
then shall we register this to next CF?

Regards,
Kirk Jamison

initdb --allow-group-access behaviour in windows

[Haribabu Kommi]

Hi Hackers,

During the testing of new feature allowing group access mode for the data
directory by specifying initdb --allow-group-access by one of my collegue,
we didn't observe any change in the data folder permissions. With/without
group access the data folder have permissions to the group.

As Microsoft windows doesn't support POSIX style of permissions, we always
set for the permissions GUC's as not supported in windows. Even the new GUC
that is added with
--allow-group-access feature also mentioned the same.

The initdb --allow-group-access doesn't have any impact on the microsoft
windows, so I feel it should be better to write the same in initdb docs?

need a patch?

Regards,
Haribabu Kommi
Fujitsu Australia

RE: ALTER SESSION

[Tsunakawa, Takayuki]

Thanks for a cool feature with nice UI.  Can I expect it to work for background 
processes?  For troubleshooting, I wanted to investigate how autovacuum 
launcher/worker behaves by having them emit DEBUG messages.

(My comments follow)


From: Kyotaro HORIGUCHI [mailto:horiguchi.kyot...@lab.ntt.co.jp]
> .auto.conf is already a kind of such.. My first version signals the change
> via shared memory (in a largely-improvable way) and add the GUC system the
> complex "nontransactional SET" feature, which lets a change persists beyond
> transaction end if any. Holding changes until the session becomes idle seems
> also complex.
> 
> https://www.postgresql.org/message-id/20181127.193622.252197705.horigu
> chi.kyot...@lab.ntt.co.jp
> 
> The most significant reason for passing-by-file is the affinity with the
> current GUC system.

Regarding the target session specification, do we want to use pid or a session 
ID like the following?

https://www.postgresql.org/docs/devel/runtime-config-logging.html
--
The %c escape prints a quasi-unique session identifier, consisting of two 
4-byte hexadecimal numbers (without leading zeros) separated by a dot. The 
numbers are the process start time and the process ID, so %c can also be used 
as a space saving way of printing those items. For example, to generate the 
session identifier from pg_stat_activity, use this query:

SELECT to_hex(trunc(EXTRACT(EPOCH FROM backend_start))::integer) || '.' ||
   to_hex(pid)
FROM pg_stat_activity;

  PRIMARY KEY (session_id, session_line_num)
--


pid is easier to type.  However, the session startup needs to try to delete the 
leftover file.  Is the file system access negligible compared with the total 
heavy session startup processing?

If we choose session ID, the session startup doesn't have to care about the 
leftover file.  However, the background process crash could leave the file for 
a long time, since the crash may not lead to postmaster restart.  Also, we will 
get inclined to add sessionid column in pg_stat_activity (the concept of 
session ID can be useful for other uses.)


I'm OK about passing parameter changes via a file.  But I'm not sure whether 
using DSM (DSA) is easier with less code.

And considering the multi-threaded server Konstantin is proposing, would it 
better to take pass-by-memory approach?  I imagine that if the server gets 
multi-threaded, the parameter change would be handled like:

1. Allocate memory for one parameter change.
2. Write the change to that memory.
3. Link the memory to a session-specific list.
4. The target session removes the entry from the list, applies the change, and 
frees the memory.

The code modification may be minimal when we migrate to the multi-threaded 
server -- only memory allocation and free functions.


Regards
Takayuki Tsunakawa

Re: possible deadlock: different lock ordering for heap pages

[Nishant, Fnu]

On 1/31/19, 9:21 PM, "Amit Kapila"  wrote:


> BTW, do you have a reproducible test case for this fix?  How did you
catch this, browsing code?

Yes, I observed it while reading code. I do not have a repro.

> How do we pronounce your name, is Nishant Fnu okay?  I would like to
mention you as the author of the patch, so need to write your name.
Yes, Nishant Fnu is okay.

Thanks
Nishant 

Re: possible deadlock: different lock ordering for heap pages

[Amit Kapila]

On Mon, Jan 21, 2019 at 10:39 PM Nishant, Fnu  wrote:
>
> Thanks Amit for your review.
>
> On 1/20/19, 6:55 AM, "Amit Kapila"  wrote:
> > I think you need to change below code as well
>Assert(buffer2 == InvalidBuffer || buffer1 <= buffer2);
>
> Done. Updated the patch.
>

Attached is an updated patch.  I have edited comments and commit
message in the patch.  I would like to backpatch this till 9.4 unless
anyone thinks otherwise.

BTW, do you have a reproducible test case for this fix?  How did you
catch this, browsing code?

How do we pronounce your name, is Nishant Fnu okay?  I would like to
mention you as the author of the patch, so need to write your name.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


lock-order.1.patch
Description: Binary data

RE: reloption to prevent VACUUM from truncating empty pages at the end of relation

[Tsunakawa, Takayuki]

From: Tsunakawa, Takayuki [mailto:tsunakawa.ta...@jp.fujitsu.com]
> As most people seem to agree adding the reloption, here's the patch.  It
> passes make check, and works like this:

Sorry, I forgot to include the modified header file.  Revised patch attached.


Regards
Takayuki Tsunakawa



disable-vacuum-truncation_v2.patch
Description: disable-vacuum-truncation_v2.patch

Re: LD_LIBRARY_PATH_RPATH

[Tom Lane]

Andrew Gierth  writes:
> At least on my FreeBSD 11 box, the current definition of
> $(with_temp_install) is not sufficient to ensure that the various .so
> files are loaded from tmp_install and not from the compiled rpath (which
> will be the final install dir, which may of course contain old
> libraries).

> LD_LIBRARY_PATH_RPATH=1 fixes this (by giving LD_LIBRARY_PATH priority
> over the DT_RPATH tag in the object).

> Is this also an issue on any other platforms?

Hm.  Can't reproduce that on current NetBSD or macOS; don't have
OpenBSD booted up to try right now.

However, if it helps on some platforms, I can't see much harm in
adding it in the relevant places.  It should be a no-op everywhere
else.

regards, tom lane

Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

[Tom Lane]

Michael Paquier  writes:
> Attached is a patch doing that.  Thoughts?

WFM.

regards, tom lane

Re: [HACKERS] generated columns

[Michael Paquier]

On Thu, Jan 17, 2019 at 10:12:26AM +0900, Michael Paquier wrote:
> Yes, something like that grows the memory and CPU usage rather
> linearly:
> CREATE TABLE tab (a int, b int GENERATED ALWAYS AS (a * 2) STORED);
> INSERT INTO tab VALUES (generate_series(1,1));

The latest patch set got plenty of feedback not addressed yet, so I am
marking it as returned with feedback.
--
Michael


signature.asc
Description: PGP signature

Re: pg_dumpall --exclude-database option

[Michael Paquier]

On Thu, Dec 27, 2018 at 10:55:43PM +0100, Fabien COELHO wrote:
> I do not "want" to do a patch for that:-) Anyway, a small patch is attached
> which adds comments to pg_dumpall output sections.

Patch has been moved to next CF with the same status, ready for
committer.  Andrew, are you planning to look at it?
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

[Michael Paquier]

On Thu, Jan 31, 2019 at 11:19:11AM +0900, Michael Paquier wrote:
> Ah yes, good point about CFLAGS and LDFLAGS.  It would be better to
> add a comment about that and document the difference, aka "prepend" or
> "append" the flag values.
> 
> CXXFLAGS applies to compiler options like -g -O2 which you would like
> to enforce for the C++ compiler, so it seems to me that like CFLAGS
> the custom values should be added at the end and not at the beginning,
> no?

Attached is a patch doing that.  Thoughts?
--
Michael
diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml
index a6b77c1cfe..b5e59d542a 100644
--- a/doc/src/sgml/extend.sgml
+++ b/doc/src/sgml/extend.sgml
@@ -1353,7 +1353,34 @@ include $(PGXS)
   PG_CPPFLAGS
   

-will be added to CPPFLAGS
+will be prepended to CPPFLAGS
+   
+  
+ 
+
+ 
+  PG_CFLAGS
+  
+   
+will be appended to CFLAGS
+   
+  
+ 
+
+ 
+  PG_CXXFLAGS
+  
+   
+will be appended to CXXFLAGS
+   
+  
+ 
+
+ 
+  PG_LDFLAGS
+  
+   
+will be prepended to LDFLAGS

   
  
diff --git a/src/makefiles/pgxs.mk b/src/makefiles/pgxs.mk
index d214cb9cf2..909a49f5be 100644
--- a/src/makefiles/pgxs.mk
+++ b/src/makefiles/pgxs.mk
@@ -52,7 +52,10 @@
 #   NO_INSTALLCHECK -- don't define an installcheck target, useful e.g. if
 # tests require special configuration, or don't use pg_regress
 #   EXTRA_CLEAN -- extra files to remove in 'make clean'
-#   PG_CPPFLAGS -- will be added to CPPFLAGS
+#   PG_CPPFLAGS -- will be prepended to CPPFLAGS
+#   PG_CFLAGS -- will be appended to CFLAGS
+#   PG_CXXFLAGS -- will be appended to CXXFLAGS
+#   PG_LDFLAGS -- will be prepended to LDFLAGS
 #   PG_LIBS -- will be added to PROGRAM link line
 #   PG_LIBS_INTERNAL -- same, for references to libraries within build tree
 #   SHLIB_LINK -- will be added to MODULE_big link line
@@ -119,6 +122,15 @@ endif
 ifdef PG_CPPFLAGS
 override CPPFLAGS := $(PG_CPPFLAGS) $(CPPFLAGS)
 endif
+ifdef PG_CFLAGS
+override CFLAGS := $(CFLAGS) $(PG_CFLAGS)
+endif
+ifdef PG_CXXFLAGS
+override CXXFLAGS := $(CXXFLAGS) $(PG_CXXFLAGS)
+endif
+ifdef PG_LDFLAGS
+override LDFLAGS := $(PG_LDFLAGS) $(LDFLAGS)
+endif
 
 # logic for HEADERS_* stuff
 


signature.asc
Description: PGP signature

Re: WIP: Avoid creation of the free space map for small tables

[Amit Kapila]

On Thu, Jan 31, 2019 at 9:18 PM John Naylor  wrote:
>
> On Thu, Jan 31, 2019 at 4:06 PM Amit Kapila  wrote:
> > I don't think that moving fsm tests to brin would be a good approach.
> > We want to have a separate test for each access method.  I think if we
> > want to do something to avoid portability issues, maybe we can do what
> > Masahiko San has just suggested.
>
> We could also use the same plpgsql loop as in fsm.sql to check the ctid, 
> right?
>

Yes, however, I feel we should leave it as it is for now unless we see
any risk of portability issues.  The only reason to do that way is to
avoid any failure for bigger block size (say BLCKSZ is 16KB or 32KB).
 Does anyone else have any opinion on whether we should try to write
tests which should care for bigger block size?  I see that existing
regression tests fail if we configure with bigger block size, so not
sure if we should try to avoid that here.  In an ideal scenario,  I
think it would be good if we can write tests which pass on all kind of
block sizes, that will make the life easier if tomorrow one wants to
set up a buildfarm or do the testing for bigger block sizes.

> > OTOH, I think we are just good w.r.t
> > this issue with the last patch I sent.  I think unless we see some
> > problem here, we should put energy into having a reproducible test for
> > the fourth problem mentioned in my mail up thread [1].  Do you think
> > it makes sense to run make check in loop for multiple times or do you
> > have any idea how we can have a reproducible test?
>
> Okay. Earlier I tried running make installcheck with
> force_parallel_mode='regress', but didn't get a failure.
>

AFAICS, this failure was not for force_parallel_mode='regress'.  See
the config at [1].

> I may not
> have run enough times, though.
>

Yeah, probably running make check or make installcheck many times
would help, but not sure.

> I'll have to think about how to induce
> it.
>

Thanks!

[1] - 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2019-01-28%2004%3A00%3A23

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: tab-completion debug print

[Michael Paquier]

On Thu, Jan 31, 2019 at 04:53:49AM -0800, Andres Freund wrote:
> My impression is that this patch ought to marked as rejected?

I may be missing something, but I have the same impression.
--
Michael


signature.asc
Description: PGP signature

RE: reloption to prevent VACUUM from truncating empty pages at the end of relation

[Tsunakawa, Takayuki]

From: Jamison, Kirk [mailto:k.jami...@jp.fujitsu.com]
> >On Thu, Nov 15, 2018 at 2:30 PM Alvaro Herrera 
> wrote:
> >>
> >> On 2018-Nov-15, Laurenz Albe wrote:
> >>
> > > > This new option would not only mitigate the long shared_buffers
> > > > scan, it would also get rid of the replication conflict caused by
> > > > the AccessExclusiveLock taken during truncation, which is discussed
> > > > in
> > > >
> https://www.postgresql.org/message-id/c9374921e50a5e8fb1ecf04eb8c6eb
> > > > c3%40postgrespro.ru and seems to be a more difficult problem than
> > > > anticipated.
> > >
> > > FWIW I was just reminded yesterday that the AEL-for-truncation has
> > > been diagnosed to be a severe problem in production, and with no other
> > > solution in sight, I propose to move forward with the stop-gap.
> 
> I just want to ask whether or not we could proceed with this approach for
> now and
> if it is possible that we could have this solution integrated before PG12
> development ends?

As most people seem to agree adding the reloption, here's the patch.  It passes 
make check, and works like this:

postgres=# CREATE TABLE a (c int) WITH (shrink_enabled  = off);
postgres=# INSERT INTO a VALUES(1);
postgres=# DELETE FROM a;
postgres=# SELECT pg_relation_size('a');
 pg_relation_size
--
 8192
(1 row)

postgres=# VACUUM a;
postgres=# SELECT pg_relation_size('a');
 pg_relation_size
--
 8192
(1 row)

postgres=#


As Tom said, we want to shorten the shared buffer scan during table truncation 
as a separate undertaking.  Kirk will do it for PG 13.  I'd appreciate much 
help from many people, because I'm afraid it will be very dificult.

And Tom mentioned likewise, I recognize I have to refresh my memory for fixing 
the data corruption by failed TRUNCATE...


Regards
Takayuki Tsunakawa



disable-vacuum-truncation.patch
Description: disable-vacuum-truncation.patch

Re: allow online change primary_conninfo

[Michael Paquier]

On Thu, Jan 31, 2019 at 04:13:22PM +0300, Sergei Kornilov wrote:
> I my opinion this is not issue, database logs can have sensitive
> data. User queries, for example.  If we not want expose such info -
> it is ok just hide new value from logs with new GUC flag? Or i need
> implement masked conninfo for this purpose?

You have problems with things in this area for any commands logged and
able to show a connection string or a password, which can go down as
well to CREATE/ALTER ROLE or FDWs.  So for the purpose of what's
discussed on this thread it does not sound like a requirement to be
able to hide that.  Role DDLs can take an already-hashed input to
avoid that, still knowing the MD5 hash is sufficient for connection
(not for SCRAM!).  Now for FDWs..
--
Michael


signature.asc
Description: PGP signature

Re: A few new options for vacuumdb

[Michael Paquier]

On Thu, Jan 31, 2019 at 03:41:34PM +, Bossart, Nathan wrote:
> Thanks for the diligent reviews, as always.  I don't plan to pick up
> --min-relation-size right now, but I may attempt it again in a future
> commitfest.

Sure, thanks for the patches!
--
Michael


signature.asc
Description: PGP signature

Re: Don't wake up to check trigger file if none is configured

[Michael Paquier]

On Thu, Jan 31, 2019 at 04:09:56PM +0900, Michael Paquier wrote:
> So, does somebody have an objection if I apply the comment patch?  Per
> the reasons above, the proposed patch is not correct, but the code can
> be more descriptive.

The comment patch has been applied, and the patch has been marked as
returned with feedback.
--
Michael


signature.asc
Description: PGP signature

Re: XLogInsert() of dangling pointer while logging replica identity

[Michael Paquier]

On Thu, Jan 31, 2019 at 11:51:36PM +0300, Stas Kelvich wrote:
>   It seems that heapam.c:3082 calls XLogRegisterData() with an argument
> allocated on stack, but following call to XLogInsert() happens after
> end of context for that variable.
>   Issue spotted by clang's AddressSanitizer. Fix attached.

Oh, good catch.  Committed and back-patched down to 9.4.
--
Michael


signature.asc
Description: PGP signature

Re: Delay locking partitions during query execution

[David Rowley]

On Fri, 1 Feb 2019 at 10:32, Robert Haas  wrote:
>
> On Sun, Jan 27, 2019 at 8:26 PM David Rowley
>  wrote:
> > One way around this would be to always perform an invalidation on the
> > partition's parent when performing a relcache invalidation on the
> > partition.  We could perhaps invalidate all the way up to the top
> > level partitioned table, that way we could just obtain a lock on the
> > target partitioned table during AcquireExecutorLocks(). I'm currently
> > only setting the delaylock flag to false for leaf partitions only.
>
> Would this problem go away if we adopted the proposal discussed in
> http://postgr.es/m/24823.1544220...@sss.pgh.pa.us and, if so, is that
> a good fix?
>
> I don't quite understand why this is happening.  It seems like as long
> as you take at least one new lock, you'll process *every* pending
> invalidation message, and that should trigger replanning as long as
> the dependencies are correct.  But maybe the issue is that you hold
> all the other locks you need already, and the lock on the partition at
> issue is only acquired during execution, at which point it's too late
> to replan.  If so, then I think something along the lines of the above
> might make a lot of sense.

I think perhaps accepting invalidations at the start of the statement
might appear to fix the problem in master, but I think there's still a
race condition around CheckCachedPlan() since we'll ignore
invalidation messages when we perform LockRelationOid() in
AcquireExecutorLocks() due to the lock already being held.  So there's
likely still a window where the invalidation message could come in and
we miss it.That said, if lock is already taken in one session, and
some other session alters some relation we have a lock on, then that
alternation can't have been done with an AEL, as that would have
blocked on our lock, so it must be some ShareUpdateExclusiveLock
change, and if so that change must not be important enough to block
concurrently executing queries, so likely shouldn't actually break
anything.

So in summary, I think that checking for invalidation messages at the
start of the statement allows us to replan within a transaction, but
does not guarantee we can the exact correct plan for the current
settings.

I also don't have a full grasp on why we must ignore invalidation
messages when we already have a lock on the relation.  I assume it's
not a huge issue since obtaining a new lock on any other relation will
process the invalidation queue anyway. I know that f868a8143a9
recently made some changes to fix some recursive issues around these
invalidations.

(I admit to not having the best grasp on how all this works, so feel
free to shoot this down)



--
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: Prepare Transaction support for ON COMMIT DROP temporary tables

[Michael Paquier]

On Mon, Jan 28, 2019 at 04:06:11PM +0900, Michael Paquier wrote:
> If you really intend to drop any trace of the objects at PREPARE
> phase, that does not seem completely impossible to me, still you would
> also need handling for the case where the temp table created also
> creates the temporary schema for the session.

More work needs to be done, and the patch has problems, so I am
marking this patch as returned with feedback.
--
Michael


signature.asc
Description: PGP signature

LD_LIBRARY_PATH_RPATH

[Andrew Gierth]

At least on my FreeBSD 11 box, the current definition of
$(with_temp_install) is not sufficient to ensure that the various .so
files are loaded from tmp_install and not from the compiled rpath (which
will be the final install dir, which may of course contain old
libraries).

LD_LIBRARY_PATH_RPATH=1 fixes this (by giving LD_LIBRARY_PATH priority
over the DT_RPATH tag in the object).

Is this also an issue on any other platforms?

-- 
Andrew (irc:RhodiumToad)

Re: amcheck verification for GiST

[Peter Geoghegan]

On Tue, Jan 1, 2019 at 2:11 AM Andrey Borodin  wrote:
> > This isn't consistent with what you do within verify_nbtree.c, which
> > deliberately avoids ever holding more than a single buffer lock at a
> > time, on general principle. That isn't necessarily a reason why you
> > have to do the same, but it's not clear why you do things that way.
> > Why isn't it enough to have a ShareLock on the relation? Maybe this is
> > a sign that it would be a good idea to always operate on a palloc()'d
> > copy of the page, by introducing something equivalent to
> > palloc_btree_page(). (That would also be an opportunity to do very
> > basic checks on every page.)
> If we unlock parent page before checking child, some insert can adjust tuple 
> on parent, sneak into child and insert new tuple.
> This can trigger false positive. I'll think about it more.

I think that holding a buffer lock on an internal pages for as long as
it takes to check all of the child pages is a non-starter. If you
can't think of a way of not doing that that's race free with a
relation-level AccessShareLock, then a relation-level ShareLock (which
will block VACUUM) seems necessary.

I think that you should duplicate some of what's in
bt_index_check_internal() -- lock the heap table as well as the index,
to avoid deadlocks. We might not do this with contrib/pageinspect, but
that's not really intended to be used in production in the same way
this will be.

> > * Maybe gist_index_check() should be called gist_index_parent_check(),
> > since it's rather like the existing verification function
> > bt_index_parent_check().
> >
> > * Alternatively, you could find a way to make your function only need
> > an AccessShareLock -- that would make gist_index_check() an
> > appropriate name. That would probably require careful thought about
> > VACUUM.
>
> I've changed lock level to AccessShareLock. IMV scan is just as safe as 
> regular GiST index scan.
> There is my patch with VACUUM on CF, it can add deleted pages. I'll update 
> one of these two patches accordingly, if other is committed.

Maybe you should have renamed it to gist_index_parent_check() instead,
and kept the ShareLock. I don't think that this business with buffer
locks is acceptable. And it is mostly checking parent/child
relationships. Right?

You're not really able to check GiST tuples against anything other
than their parent, unlike with B-Tree (you can only do very simple
things, like the IndexTupleSize()/lp_len cross-check). Naming the
function gist_index_parent_check() seems like the right way to go,
even if you could get away with an AccessShareLock (which I now tend
to doubt). It was way too optimistic to suppose that there might be a
clever way of locking down race conditions that allowed you to not
couple buffer locks, but also use an AccessShareLock. After all, even
the B-Tree amcheck code doesn't manage to do this when verifying
parent/child relationships (it only does something clever with the
sibling page cross-check).

> > * Why is it okay to do this?:
> >
> >> +   if (GistTupleIsInvalid(idxtuple))
> >> +   ereport(LOG,
> >> +   (errmsg("index \"%s\" contains an inner tuple marked 
> >> as invalid",
> >> +   RelationGetRelationName(rel)),
> >> +errdetail("This is caused by an incomplete page split 
> >> at crash recovery before upgrading to PostgreSQL 9.1."),
> >> +errhint("Please REINDEX it.")));
> >
> > You should probably mention the gistdoinsert() precedent for this.
> This invalid tuple will break inserts, but will not affect select. I do not 
> know, should this be error or warning in amcheck?

It should be an error. Breaking inserts is a serious problem.

-- 
Peter Geoghegan

Re: pg_stat_ssl additions

[Peter Eisentraut]

On 29/01/2019 13:11, Peter Eisentraut wrote:
> On 29/01/2019 04:18, Kyotaro HORIGUCHI wrote:
>> Some further investigation told me that the file
>> ~/.postgresql/root.cert was the culprit.
> 
> OK, I could reproduce the problem and found a fix for it.  Basically you
> need to specify sslrootcert in each test, and these new tests didn't do
> it.  All other tests were OK, so a local fix was enough.
> 
> I have committed 0001..0003 now.  Attached is the latest version of
> 0004, about which I didn't not get an explicit comment in your last
> review message.

I have committed the rest of this.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Delay locking partitions during query execution

[Tomas Vondra]

On 1/31/19 10:31 PM, Robert Haas wrote:
> On Sun, Jan 27, 2019 at 8:26 PM David Rowley
>  wrote:
>> One way around this would be to always perform an invalidation on the
>> partition's parent when performing a relcache invalidation on the
>> partition.  We could perhaps invalidate all the way up to the top
>> level partitioned table, that way we could just obtain a lock on the
>> target partitioned table during AcquireExecutorLocks(). I'm currently
>> only setting the delaylock flag to false for leaf partitions only.
> 
> Would this problem go away if we adopted the proposal discussed in
> http://postgr.es/m/24823.1544220...@sss.pgh.pa.us and, if so, is that
> a good fix?
> 
> I don't quite understand why this is happening.  It seems like as long
> as you take at least one new lock, you'll process *every* pending
> invalidation message, and that should trigger replanning as long as
> the dependencies are correct.  But maybe the issue is that you hold
> all the other locks you need already, and the lock on the partition at
> issue is only acquired during execution, at which point it's too late
> to replan.  If so, then I think something along the lines of the above
> might make a lot of sense.
> 

It happens because ConditionalLockRelation does this:

if (res != LOCKACQUIRE_ALREADY_CLEAR)
{
AcceptInvalidationMessages();
MarkLockClear(locallock);
}

and with the prepared statement already planned, we end up skipping that
because res == LOCKACQUIRE_ALREADY_CLEAR. Which happens because
GrantLockLocal (called from LockAcquireExtended) find the relation in as
already locked.

I don't know if this is correct or not, though.


regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: ATTACH/DETACH PARTITION CONCURRENTLY

[Alvaro Herrera]

On 2019-Jan-31, Robert Haas wrote:

> OK, that seems to be pretty easy.  New patch series attached.  The
> patch with that new logic is 0004.  I've consolidated some of the
> things I had as separate patches in my last post and rewritten the
> commit messages to explain more clearly the purpose of each patch.

Looks awesome.

> - For now, I haven't tried to handle the DETACH PARTITION case.  I
> don't think there's anything preventing someone - possibly even me -
> from implementing the counter-based approach that I described in the
> previous message, but I think it would be good to have some more
> discussion first on whether it's acceptable to make concurrent queries
> error out.  I think any queries that were already up and running would
> be fine, but any that were planned before the DETACH and tried to
> execute afterwards would get an ERROR.  That's fairly low-probability,
> because normally the shared invalidation machinery would cause
> replanning, but there's a race condition, so we'd have to document
> something like: if you use this feature, it'll probably just work, but
> you might get some funny errors in other sessions if you're unlucky.
> That kinda sucks but maybe we should just suck it up.  Possibly we
> should consider making the concurrent behavior optional, so that if
> you'd rather take blocking locks than risk errors, you have that
> option.  Of course I guess you could also just let people do an
> explicit LOCK TABLE if that's what they want.  Or we could try to
> actually make it work in that case, I guess by ignoring the detached
> partitions, but that seems a lot harder.

I think telling people to do LOCK TABLE beforehand if they care about
errors is sufficient.  On the other hand, I do hope that we're only
going to cause queries to fail if they would affect the partition that's
being detached and not other partitions in the table.  Or maybe because
of the replanning on invalidation this doesn't matter as much as I think
it does.

> - 0003 doesn't have any handling for parallel query at this point, so
> even though within a single backend a single query execution will
> always get the same PartitionDesc for the same relation, the answers
> might not be consistent across the parallel group.

That doesn't sound good.  I think the easiest would be to just serialize
the PartitionDesc and send it to the workers instead of them recomputing
it, but then I worry that this might have bad performance when the
partition desc is large.  (Or maybe sending bytes over pqmq is faster
than reading all those catalog entries and so this isn't a concern
anyway.)

> - 0003 also changes the order in which locks are acquired.  I am not
> sure whether we care about this, especially in view of other pending
> changes.

Yeah, the drawbacks of the unpredictable locking order are worrisome,
but then the performance gain is hard to dismiss.  Not this patch only
but the others too.  If we're okay with the others going in, I guess we
don't have concerns about this one either.

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

Re: Using POPCNT and other advanced bit manipulation instructions

[Alvaro Herrera]

I only have cosmetic suggestions for this patch.  For one thing, I think
the .c file should be in src/port and its header should be in
src/include/port/, right beside the likes of pg_bswap.h and pg_crc32c.h.
For another, I think the arrangement of all those "ifdef
HAVE_THIS_OR_THAT" in the bitutils.c file is a bit hard to read.  I'd
lay them out like this:

  #ifdef HAVE__BUILTIN_CTZ
  int   (*pg_rightmost_one32) (uint32 word) = 
pg_rightmost_one32_choose;
  #else
  int   (*pg_rightmost_one32) (uint32 word) = 
pg_rightmost_one32_slow;
  #endif

  #ifdef HAVE__BUILTIN_CTZ
  /*
   * This gets called on the first call. It replaces the function pointer
   * so that subsequent calls are routed directly to the chosen implementation.
   */
  static int
  pg_rightmost_one32_choose(uint32 word)
  {
  ...

(You need declarations for the "choose" variants at the top of the file,
but that seems okay.)

Finally, the part in bitmapset.c is repetitive on the #ifdefs; I'd just
put at the top of the file something like 

  #if bms are 32 bits
  #define pg_rightmost_one(x) pg_rightmost_one32(x)
  #define pg_popcount(x) pg_popcount32(x)
  #elif they are 64 bits
  #define ...
  #else
  #error ...
  #endif

This way, each place that uses the functions does not need the ifdefs.

Other than those minor changes, I think we should just get this pushed
and see what the buildfarm thinks.  In the words of a famous PG hacker:
if a platform ain't in the buildfarm, we don't support it.

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

Re: Delay locking partitions during INSERT and UPDATE

[David Rowley]

On Fri, 1 Feb 2019 at 07:46, Robert Haas  wrote:
> I have reviewed this patch and I am in favor of it.  I think it likely
> needs minor rebasing because of the heap_open -> table_open renaming.

Many thanks for looking at it.   The v2 patch was based on top of the
heap_open -> table_open change.


-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: Delay locking partitions during query execution

[Robert Haas]

On Sun, Jan 27, 2019 at 8:26 PM David Rowley
 wrote:
> One way around this would be to always perform an invalidation on the
> partition's parent when performing a relcache invalidation on the
> partition.  We could perhaps invalidate all the way up to the top
> level partitioned table, that way we could just obtain a lock on the
> target partitioned table during AcquireExecutorLocks(). I'm currently
> only setting the delaylock flag to false for leaf partitions only.

Would this problem go away if we adopted the proposal discussed in
http://postgr.es/m/24823.1544220...@sss.pgh.pa.us and, if so, is that
a good fix?

I don't quite understand why this is happening.  It seems like as long
as you take at least one new lock, you'll process *every* pending
invalidation message, and that should trigger replanning as long as
the dependencies are correct.  But maybe the issue is that you hold
all the other locks you need already, and the lock on the partition at
issue is only acquired during execution, at which point it's too late
to replan.  If so, then I think something along the lines of the above
might make a lot of sense.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] Block level parallel vacuum

[Masahiko Sawada]

On Wed, Jan 30, 2019 at 2:06 AM Haribabu Kommi  wrote:
>
>
> On Thu, Jan 24, 2019 at 1:16 PM Masahiko Sawada  wrote:
>>
>>
>> Attached the latest patches.
>
>
> Thanks for the updated patches.
> Some more code review comments.
>

Thank you!

> + started by a single utility command.  Currently, the parallel
> + utility commands that support the use of parallel workers are
> + CREATE INDEX and VACUUM
> + without FULL option, and only when building
> + a B-tree index.  Parallel workers are taken from the pool of
>
>
> I feel the above sentence may not give the proper picture, how about the
> adding following modification?
>
> CREATE INDEX only when building a B-tree index
> and VACUUM without FULL option.
>
>

Agreed.

>
> + * parallel vacuum, we perform both index vacuum and index cleanup in 
> parallel.
> + * Individual indexes is processed by one vacuum process. At beginning of
>
> How about vacuum index and cleanup index similar like other places?
>
>
> + * memory space for dead tuples. When starting either index vacuum or cleanup
> + * vacuum, we launch parallel worker processes. Once all indexes are 
> processed
>
> same here as well?
>
>
> + * Before starting parallel index vacuum and parallel cleanup index we launch
> + * parallel workers. All parallel workers will exit after processed all 
> indexes
>
> parallel vacuum index and parallel cleanup index?
>
>

ISTM we're using like "index vacuuming", "index cleanup" and "FSM
vacuming" in vacuumlazy.c so maybe "parallel index vacuuming" and
"parallel index cleanup" would be better?

> + /*
> + * If there is already-updated result in the shared memory we
> + * use it. Otherwise we pass NULL to index AMs and copy the
> + * result to the shared memory segment.
> + */
> + if (lvshared->indstats[idx].updated)
> + result = &(lvshared->indstats[idx].stats);
>
> I didn't really find a need of the flag to differentiate the stats pointer 
> from
> first run to second run? I don't see any problem in passing directing the 
> stats
> and the same stats are updated in the worker side and leader side. Anyway no 
> two
> processes will do the index vacuum at same time. Am I missing something?
>
> Even if this flag is to identify whether the stats are updated or not before
> writing them, I don't see a need of it compared to normal vacuum.
>

The passing stats = NULL to amvacuumcleanup and ambulkdelete means the
first time execution. For example, btvacuumcleanup skips cleanup if
it's not NULL.In the normal vacuum we pass NULL to ambulkdelete or
amvacuumcleanup when the first time calling. And they store the result
stats to the memory allocated int the local memory. Therefore in the
parallel vacuum I think that both worker and leader need to move it to
the shared memory and mark it as updated as different worker could
vacuum different indexes at the next time.

>
> + * Enter the parallel mode, allocate and initialize a DSM segment. Return
> + * the memory space for storing dead tuples or NULL if no workers are 
> prepared.
> + */
>
> + pcxt = CreateParallelContext("postgres", "heap_parallel_vacuum_main",
> + request, true);
>
> But we are passing as serializable_okay flag as true, means it doesn't return
> NULL. Is it expected?
>
>

I think you're right. Since the request never be 0 and
serializable_okey is true it should not return NULL. Will fix.

> + initStringInfo(&buf);
> + appendStringInfo(&buf,
> + ngettext("launched %d parallel vacuum worker %s (planned: %d",
> +   "launched %d parallel vacuum workers %s (planned: %d",
> +   lvstate->pcxt->nworkers_launched),
> + lvstate->pcxt->nworkers_launched,
> + for_cleanup ? "for index cleanup" : "for index vacuum",
> + lvstate->pcxt->nworkers);
> + if (lvstate->options.nworkers > 0)
> + appendStringInfo(&buf, ", requested %d", lvstate->options.nworkers);
>
> what is the difference between planned workers and requested workers, aren't 
> both
> are same?

The request is the parallel degree that is specified explicitly by
user whereas the planned is the actual number we planned based on the
number of indexes the table has. For example, if we do like 'VACUUM
(PARALLEL 3000) tbl' where the tbl has 4 indexes, the request is 3000
and the planned is 4. Also if max_parallel_maintenance_workers is 2
the planned is 2.

>
>
> - COMPARE_SCALAR_FIELD(options);
> - COMPARE_NODE_FIELD(rels);
> + if (a->options.flags != b->options.flags)
> + return false;
> + if (a->options.nworkers != b->options.nworkers)
> + return false;
>
> Options is changed from SCALAR to check, but why the rels check is removed?
> The options is changed from int to a structure so using SCALAR may not work
> in other function like _copyVacuumStmt and etc?

Agreed and will fix.

>
> +typedef struct VacuumOptions
> +{
> + VacuumFlag flags; /* OR of VacuumFlag */
> + int nworkers; /* # of parallel vacuum workers */
> +} VacuumOptions;
>
>
> Do we need to add NodeTag for the above structure? Because this structure is
> part of Vacu

XLogInsert() of dangling pointer while logging replica identity

[Stas Kelvich]

Hi, hackers.

  It seems that heapam.c:3082 calls XLogRegisterData() with an argument
allocated on stack, but following call to XLogInsert() happens after
end of context for that variable.
  Issue spotted by clang's AddressSanitizer. Fix attached.

--
Stas Kelvich
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company



0001-Fix-use-after-scope.patch
Description: Binary data

Re: [Patch] Log10 and hyperbolic functions for SQL:2016 compliance

[Lætitia Avrot]

Hi,

Thanks to Emil Iggland's kind review, I added precision on documentation
for hyperbolic functions.

I added the patch to the next commitfest.

Cheers,

Lætitia

Le dim. 27 janv. 2019 à 20:39, Lætitia Avrot  a
écrit :

> Hi,
>
> Thanks for your time and advice, Tom!
>
>
>> > [ adding_log10_and_hyperbolic_functions_v1.patch ]
>>
>> No objection to the feature, but
>>
>> - Why are you using the float4-width library functions (coshf etc)
>> rather than the float8-width ones (cosh etc)?
>>
>> Well, I guess the only reason is that I wasn't paying attention enough...
> I changed for the float8-width library functions.
>
>
>> - I wonder whether these library functions exist everywhere.
>> If they don't, what will we do about it ... throw an error?
>>
>> I see that SUSv2 requires cosh() and so on, so it's quite possible
>> that these do exist everywhere we care about.  I'd be okay with
>> throwing a patch onto the buildfarm to see, and adding an autoconf
>> test only if the buildfarm is unhappy.  But we should be clear on
>> what we're going to do about it if that happens.
>>
>> I think I was too confident because of the "it works on my laptop"
> syndrome... I don't know how to answer to this point.
>
>
>> > I added regression tests for the new functions, but I didn't for log10
>> > function, assuming that if log function worked, log10 will work too.
>>
>> Not quite sure I believe that.
>>
>> Do you mean I should also add a regression test for the new log10
> function too ?
>
>
>> Actually, what I'd counsel is that you *not* include tests of what
>> these do with Inf and NaN.  There is no upside to doing so, and lots
>> of downside if older platforms are squirrely in their behavior, which
>> is hardly unlikely (cf opossum ...)
>>
>
> I changed the regression tests for hyperbolic functions, so it doesn't
> test for Inf and NaN.
>
> You'll find enclosed the new version of the patch.
>
> Cheers,
>
> Lætitia
>


-- 
*Think! Do you really need to print this email ? *
*There is no Planet B.*
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 4930ec17f6..30ecf496ca 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -896,6 +896,19 @@
2
   
 
+  
+   
+
+ log10
+
+log10(dp or numeric)
+   
+   (same as input)
+   base 10 logarithm
+   log10(100.0)
+   2
+  
+
   
log(b numeric,
 x numeric)
@@ -1147,7 +1160,7 @@
   
 
   
-   Finally,  shows the
+shows the
available trigonometric functions.  All trigonometric functions
take arguments and return values of type double
precision.  Each of the trigonometric functions comes in
@@ -1311,8 +1324,66 @@

   
 
-  
+  
+   Finally,  shows the
+   available hyperbolic functions. All hyperbolic functions apply to
+   hyperbolic angles expressed in hyperbolic radians.
+  
 
+  
+Hyperbolic Functions
+
+
+ 
+  
+   Function
+   Return Type
+   Description
+   Example
+   Result
+  
+ 
+ 
+  
+   
+
+ sinh
+
+sinh(dp)
+   
+   dp
+   hyperbolic sine
+   sinh(0)
+   0
+  
+  
+   
+
+ cosh
+
+cosh(dp)
+   
+   dp
+   hyperbolic cosine
+   cosh(0)
+   1
+  
+  
+   
+
+ tanh
+
+tanh(dp)
+   
+   dp
+   hyperbolic tangent
+   tanh(0)
+   0
+  
+ 
+
+   
+  
 
   
String Functions and Operators
diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index 117ded8d1d..7712c3d5bd 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -2375,6 +2375,54 @@ radians(PG_FUNCTION_ARGS)
 	PG_RETURN_FLOAT8(float8_mul(arg1, RADIANS_PER_DEGREE));
 }
 
+/* == HYPERBOLIC FUNCTIONS == */
+
+/*
+ *		dsinh			- returns the hyperbolic sine of arg1
+ */
+Datum
+dsinh(PG_FUNCTION_ARGS)
+{
+	float8		arg1 = PG_GETARG_FLOAT8(0);
+	float8		result;
+
+	result = sinh(arg1);
+
+	check_float8_val(result, isinf(arg1), true);
+	PG_RETURN_FLOAT8(result);
+}
+
+
+/*
+ *		dcosh			- returns the hyperbolic cosine of arg1
+ */
+Datum
+dcosh(PG_FUNCTION_ARGS)
+{
+	float8		arg1 = PG_GETARG_FLOAT8(0);
+	float8		result;
+
+	result = cosh(arg1);
+
+	check_float8_val(result, isinf(arg1), false);
+	PG_RETURN_FLOAT8(result);
+}
+
+/*
+ *		dtanh			- returns the hyperbolic tangent of arg1
+ */
+Datum
+dtanh(PG_FUNCTION_ARGS)
+{
+	float8		arg1 = PG_GETARG_FLOAT8(0);
+	float8		result;
+
+	errno = 0;
+	result = tanh(arg1);
+
+	check_float8_val(result, false, true);
+	PG_RETURN_FLOAT8(result);
+}
 
 /*
  *		drandom		- returns a random number
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 3ecc2e12c3..ca6b09a13d 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -2617,6 +2617,9 @@
 { oid => '1340', descr => 'base 10

Re: Proposed refactoring of planner header files

[Tom Lane]

Robert Haas  writes:
> I do not have a powerful opinion on exactly what to do here, but I
> offer the following for what it's worth:

> - I do not really think much of the selfuncs.c naming.  Nobody who is
> not deeply immersed in the PostgreSQL code knows what a "selfunc" is.
> Therefore, breaking selfuncs.c into opr_selfuncs.c and
> index_selfuncs.c doesn't strike me as the ideal naming choice.  I
> would suggest looking for a name that is less steeped in venerable
> tradition; perhaps estimator.c or indexsupport.c or selectivity.c or
> whatever could be considered for whatever new files we are creating.

Hm.  I'm not really persuaded.  There isn't any part of this code that
isn't "steeped in venerable tradition"; the problem I'm ultimately
trying to fix here can fairly be characterized as 25-year-old technical
debt.  I see your point that adt/ is not where new people might look for
selectivity estimators, but we should also have some sympathy for people
who *do* know the code base expecting to find these functions more or
less where they are.

> - I have always found the placement of selfuncs.c a bit strange: why
> is it in utils/adt?  Perhaps there is an argument for having the
> things that are specific to a particular type or a particular operator
> in that directory, but if so, shouldn't they be grouped with the type
> or operator to which they relate, rather than each other?

We do already have that to some extent:

src/backend/tsearch/ts_selfuncs.c
src/backend/utils/adt/array_selfuncs.c
src/backend/utils/adt/rangetypes_selfuncs.c
src/backend/utils/adt/geo_selfuncs.c
src/backend/utils/adt/network_selfuncs.c
src/backend/utils/adt/selfuncs.c
src/include/utils/index_selfuncs.h
src/include/utils/selfuncs.h
contrib/intarray/_int_selfuncs.c

Breaking up selfuncs.c would carry this a bit further, but giving it
a random new name doesn't seem to me to really improve matters.

Now, if we move the support functions to someplace under optimizer/,
we do have to think about a name for that.  I was considering 
optimizer/utils/selsupport.c and optimizer/selsupport.h, but I'm
certainly not wedded to that.

regards, tom lane

Re: pg_upgrade: Pass -j down to vacuumdb

[Jesper Pedersen]

Hi,

On 1/30/19 7:59 PM, Jamison, Kirk wrote:

I added most of the documentation back, as requested by Kirk.


Actually, I find it useful to be documented. However, major contributors have 
higher opinions in terms of experience, so I think it's alright with me not to 
include the doc part if they finally say so.



I think we need to leave it to the Committer to decide, as both Peter  
and Michael are committers; provided the patch reaches RfC.



It seems to me that the latest patch sent is incorrect for multiple
reasons:
1) You still enforce -j to use the number of jobs that the caller of
pg_upgrade provides, and we agreed that both things are separate
concepts upthread, no?  What has been suggested by Alvaro is to add a
comment so as one can use VACUUM_OPTS with -j optionally, instead of
suggesting a full-fledged vacuumdb command which depends on what
pg_upgrade uses.  So there is no actual need for the if/else
complication business.



I think it is ok for the echo command to highlight to the user that running 
--analyze-only using the same amount of jobs will give a faster result.


I missed that part.
IIUC, using the $VACUUMDB_OPTS variable would simplify and correct the usage of 
jobs for vacuumdb. (pg_upgrade jobs is not equal to vacuumdb jobs) Plus, it 
might simplify and reduce the number of additional lines.
Tom Lane also suggested above to make the script absorb the value from env.
Would that address your concern of getting a faster result, yes?



The actual line in the script executed is using $VACUUMDB_OPTS at the  
moment, so could you expand on the above ? The 'if' could be removed if  
we assume that pg_upgrade would only be used with server > 9.5, but to  
be safer I would leave it in, as it is only console output.


Best regards,
 Jesper

Re: Proposed refactoring of planner header files

[Robert Haas]

On Thu, Jan 31, 2019 at 2:46 PM Tom Lane  wrote:
> I'm happy to do the work if there's consensus on what to do.  After
> a few moments' thought, I'd suggest:
>
> 1. Move the indexscan cost estimation functions into a new file
> adt/index_selfuncs.c.  Most of them already are declared in
> utils/index_selfuncs.h, and we'd move the remaining declarations
> (primarily, genericcostestimate()) to that file as well.  This
> would affect #includes in contrib/bloom/ and any 3rd-party index
> AMs that might be using genericcostestimate().
>
> 2a. Leave the support functions in selfuncs.c with declarations in
> utils/selfuncs.h, and move the operator-specific estimators to
> a new file, perhaps opr_selfuncs.c.  This would minimize pain for
> existing includers of selfuncs.h, most of which (particularly outside
> core) are presumably interested in the support functions.
>
> 2b. Alternatively, leave the operator-specific estimators where they
> are, and make new files under optimizer/ for the support functions.

I do not have a powerful opinion on exactly what to do here, but I
offer the following for what it's worth:

- I do not really think much of the selfuncs.c naming.  Nobody who is
not deeply immersed in the PostgreSQL code knows what a "selfunc" is.
Therefore, breaking selfuncs.c into opr_selfuncs.c and
index_selfuncs.c doesn't strike me as the ideal naming choice.  I
would suggest looking for a name that is less steeped in venerable
tradition; perhaps estimator.c or indexsupport.c or selectivity.c or
whatever could be considered for whatever new files we are creating.

- I have always found the placement of selfuncs.c a bit strange: why
is it in utils/adt?  Perhaps there is an argument for having the
things that are specific to a particular type or a particular operator
in that directory, but if so, shouldn't they be grouped with the type
or operator to which they relate, rather than each other?  And if
they're not specific to a certain thing, or if grouping them with that
thing would suck because of what we'd have to #include or some other
grounds, then why not put them in the optimizer?  I think that a large
fraction of this code is actually generic, and putting it in the
optimizer directory someplace would be more logical than what we have
now.  Alternatively, since these have to be exposed via pg_proc, maybe
the shims should go in this directory and the core logic elsewhere.
Again, I don't have a strong opinion here, but I've never thought this
made a ton of sense the way it is.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: pgsql: Build src/port files as a library with -fPIC, and use that in li

[Robert Haas]

On Thu, Jan 31, 2019 at 2:50 PM Tom Lane  wrote:
> Robert Haas  writes:
> > Isn't the issue more a matter of whether it's OK to break
> > compatibility without changing SO_*_VERSION?
>
> We're breaking compatibility only if you regard pqsignal() as
> part of the advertised API, which I don't.

Well, I would agree with that if it weren't the case that binaries we
ship apparently use that API.  If we say that the libraries are
API-compatible, but our own binaries don't work, our veracity is
questionable.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: pgsql: Build src/port files as a library with -fPIC, and use that in li

[Tom Lane]

Robert Haas  writes:
> On Thu, Jan 31, 2019 at 2:50 PM Tom Lane  wrote:
>> We're breaking compatibility only if you regard pqsignal() as
>> part of the advertised API, which I don't.

> Well, I would agree with that if it weren't the case that binaries we
> ship apparently use that API.  If we say that the libraries are
> API-compatible, but our own binaries don't work, our veracity is
> questionable.

initdb hasn't depended on those libpq exports since 8.2, and it was
a bug that it did so even then.

regards, tom lane

Re: pgsql: Build src/port files as a library with -fPIC, and use that in li

[Tom Lane]

Robert Haas  writes:
> On Tue, Jan 29, 2019 at 10:10 AM Tom Lane  wrote:
>> Well, 8.2 is *very* long out of support, and there are plenty of
>> nasty bugs you're at risk of if you keep using it.  So I don't
>> find this to be a good argument for contorting what we do in v12.

> Isn't the issue more a matter of whether it's OK to break
> compatibility without changing SO_*_VERSION?

We're breaking compatibility only if you regard pqsignal() as
part of the advertised API, which I don't.

regards, tom lane

Re: Proposed refactoring of planner header files

[Tom Lane]

Robert Haas  writes:
> On Wed, Jan 30, 2019 at 10:08 PM Tom Lane  wrote:
>> However ... there are three pretty clearly identifiable groups of
>> functions in selfuncs.c: operator-specific estimators, support
>> functions, and AM-specific indexscan cost estimation functions.
>> There's a case to be made for splitting them into three separate files.

> I think splitting it along these lines would be pretty sensible.  I'm
> not really a fan of giant source files, especially if there's no
> really good reason to put all that stuff in one source file ... then
> again, I'm also not volunteering to do the work.

I'm happy to do the work if there's consensus on what to do.  After
a few moments' thought, I'd suggest:

1. Move the indexscan cost estimation functions into a new file
adt/index_selfuncs.c.  Most of them already are declared in
utils/index_selfuncs.h, and we'd move the remaining declarations
(primarily, genericcostestimate()) to that file as well.  This
would affect #includes in contrib/bloom/ and any 3rd-party index
AMs that might be using genericcostestimate().

2a. Leave the support functions in selfuncs.c with declarations in
utils/selfuncs.h, and move the operator-specific estimators to
a new file, perhaps opr_selfuncs.c.  This would minimize pain for
existing includers of selfuncs.h, most of which (particularly outside
core) are presumably interested in the support functions.

2b. Alternatively, leave the operator-specific estimators where they
are, and make new files under optimizer/ for the support functions.

I think 2b would make more sense in the abstract, but it would also
result in more #include churn for extensions.  OTOH we already
forced some churn in that area with the planner-header-refactoring
patch, so the delta cost is probably not that much.

In any case, I'm hoping to get rid of the exported declarations for
pattern selectivity (Pattern_Prefix_Status, pattern_fixed_prefix,
etc) altogether.  Those are exported because indxpath.c currently
calls them from its wired-in lossy-index-clause logic.  When the
dust settles, all that code should be associated with planner
support functions for the LIKE and regex operators, and will not
need to be visible outside of whatever module we put those in.
But that patch is yet to be written.

Opinions?

regards, tom lane

Re: pgsql: Build src/port files as a library with -fPIC, and use that in li

[Robert Haas]

On Tue, Jan 29, 2019 at 10:10 AM Tom Lane  wrote:
> Well, 8.2 is *very* long out of support, and there are plenty of
> nasty bugs you're at risk of if you keep using it.  So I don't
> find this to be a good argument for contorting what we do in v12.

Isn't the issue more a matter of whether it's OK to break
compatibility without changing SO_*_VERSION?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Proposed refactoring of planner header files

[Robert Haas]

On Wed, Jan 30, 2019 at 10:08 PM Tom Lane  wrote:
> However ... there are three pretty clearly identifiable groups of
> functions in selfuncs.c: operator-specific estimators, support
> functions, and AM-specific indexscan cost estimation functions.
> There's a case to be made for splitting them into three separate files.
> One point is that selfuncs.c is just too large; at over 8K lines,
> it's currently the 7th-largest hand-maintained C file in our tree.
> Another point is that as things stand, there's a strong temptation
> to bury useful functionality in static functions that can't be
> gotten at by extension estimators.  Separating the support functions
> might help keep us more honest on that point.  (Or not.)
>
> I'm not sure whether those arguments are strong enough to justify
> the added pain-in-the-neck factor from moving a bunch of code
> around.  That complicates back-patching bug fixes and it makes it
> harder to trace the git history of the code.  So I've got mixed
> emotions about whether it's worth doing anything.

I think splitting it along these lines would be pretty sensible.  I'm
not really a fan of giant source files, especially if there's no
really good reason to put all that stuff in one source file ... then
again, I'm also not volunteering to do the work.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Add --include-table-data-where option to pg_dump, to export only a subset of table data

[Carter Thaxton]

> > Unfortunately, current version of this patch has some conflicts, could
> you
> > rebase it?
>
> Given this patch has not been updated, I'm marking this as returned with
> feedback.
>

Here's a rebased patch that works with the latest master branch.  Very
straightforward.


pgdump-include-table-data-where-v5.patch
Description: Binary data

Re: possible deadlock: different lock ordering for heap pages

[Robert Haas]

On Sun, Jan 20, 2019 at 9:55 AM Amit Kapila  wrote:
> Robert, can you please once see if we are missing anything here
> because to me the report and fix look genuine.

I think so, too.  I guess the probability of a deadlock here must be *very* low.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Delay locking partitions during INSERT and UPDATE

[Robert Haas]

On Thu, Jan 24, 2019 at 4:43 PM Tomas Vondra
 wrote:
> Yes, I don't see why would the patch change that and I've been looking
> for such cases. I think David was looking at that this week too, and I
> assume he'll send an update if he finds anything. If not, I plan to get
> it committed soon-ish (possibly next week after the FOSDEM dev meeting,
> unless there are some objections).

I have reviewed this patch and I am in favor of it.  I think it likely
needs minor rebasing because of the heap_open -> table_open renaming.
I also agree that it's worth taking some deadlock risk for the rather
massive performance gain, although I suspect it's likely that a few
users are going to complain about deadlocks and I wonder whether we'll
have to put some energy into that problem at some point.  However, I
think what we probably want to do there is reduce the probability of
deadlocks through some trickery or maybe invent some new locking
mechanisms that work around the problem.  The alternative of trying to
block this patch seems unpalatable.

In short, +1 from me.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: ATTACH/DETACH PARTITION CONCURRENTLY

[Robert Haas]

On Tue, Jan 29, 2019 at 1:59 PM Robert Haas  wrote:
> I don't know how to reduce this to something reliable enough to
> include it in the regression tests, and maybe we don't really need
> that, but it's good to know that this is not a purely theoretical
> problem.  I think next I'll try to write some code to make
> execPartition.c able to cope with the situation when it arises.

OK, that seems to be pretty easy.  New patch series attached.  The
patch with that new logic is 0004.  I've consolidated some of the
things I had as separate patches in my last post and rewritten the
commit messages to explain more clearly the purpose of each patch.

Open issues:

- For now, I haven't tried to handle the DETACH PARTITION case.  I
don't think there's anything preventing someone - possibly even me -
from implementing the counter-based approach that I described in the
previous message, but I think it would be good to have some more
discussion first on whether it's acceptable to make concurrent queries
error out.  I think any queries that were already up and running would
be fine, but any that were planned before the DETACH and tried to
execute afterwards would get an ERROR.  That's fairly low-probability,
because normally the shared invalidation machinery would cause
replanning, but there's a race condition, so we'd have to document
something like: if you use this feature, it'll probably just work, but
you might get some funny errors in other sessions if you're unlucky.
That kinda sucks but maybe we should just suck it up.  Possibly we
should consider making the concurrent behavior optional, so that if
you'd rather take blocking locks than risk errors, you have that
option.  Of course I guess you could also just let people do an
explicit LOCK TABLE if that's what they want.  Or we could try to
actually make it work in that case, I guess by ignoring the detached
partitions, but that seems a lot harder.

- 0003 doesn't have any handling for parallel query at this point, so
even though within a single backend a single query execution will
always get the same PartitionDesc for the same relation, the answers
might not be consistent across the parallel group.  I keep going back
and forth on whether this really matters.  It's too late to modify the
plan, so any relations attached since it was generated are not going
to get scanned.  As for detached relations, we're talking about making
them error out, so we don't have to worry about different backends
come to different conclusions about whether they should be scanned.
But maybe we should try to be smarter instead.  One concern is that
even relations that aren't scanned could still be affected because of
tuple routing, but right now parallel queries can't INSERT or UPDATE
rows anyway.  Then again, maybe we should try not to add more
obstacles in the way of lifting that restriction. Then again again,
AFAICT we wouldn't be able to test that the new code is actually
solving a problem right now today, and how much untested code do we
really want in the tree?  And then on the eleventh hand, maybe there
are other reasons why it's important to use the same PartitionDesc
across all parallel workers that I'm not thinking about at the moment.

- 0003 also changes the order in which locks are acquired.  I am not
sure whether we care about this, especially in view of other pending
changes.

If you know of other problems, have solutions to or opinions about
these, or think the whole approach is wrong, please speak up!

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


0002-Ensure-that-RelationBuildPartitionDesc-sees-a-consis.patch
Description: Binary data


0001-Move-code-for-managing-PartitionDescs-into-a-new-fil.patch
Description: Binary data


0005-Reduce-the-lock-level-required-to-attach-a-partition.patch
Description: Binary data


0004-Teach-runtime-partition-pruning-to-cope-with-concurr.patch
Description: Binary data


0003-Ensure-that-repeated-PartitionDesc-lookups-return-th.patch
Description: Binary data

Re: Fast path for empty relids in check_outerjoin_delay()

[Tom Lane]

Since we've reached the end of the January commitfest, and it's pretty
clear that this patch isn't going to get committed in anything like
its current form, I'm going to close the CF entry as returned with
feedback.  If you come up with a follow-on patch, please create a
new CF entry.

regards, tom lane

Re: Statement-level rollback

[Alvaro Herrera]

On 2019-Jan-31, Greg Stark wrote:

> This looks like repeating the autocommit mistakes of the past.

Yeah, this argument has been made before.  Peter E argued against that:
https://postgr.es/m/ea395aa8-5ac4-6bcd-366d-aab2ff2b0...@2ndquadrant.com

> And based on that experience wouldn't the replacement approach be to do
> this client side?

JDBC has a feature for this already (see "autosave" in [1]), but
apparently you get a good performance improvement by doing it
server-side, which is why this was written in the first place.  psql
also has ON_ERROR_ROLLBACK [2].

> If libpq had a library option to wrap every statement in a
> subtransaction by adding begin/end then this problem would be
> completely sidestepped.

I don't think anyone's talked about doing it in libpq before
specifically, and if you did that, it would obviously have to be
implemented separately on JDBC.

[1] 
https://jdbc.postgresql.org/documentation/head/connect.html#connection-parameters
[2] https://www.postgresql.org/docs/current/app-psql.html

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

Re: Global snapshots

[Stas Kelvich]

> On 31 Jan 2019, at 18:42, Andres Freund  wrote:
> 
> Hi,
> 
> On 2018-11-30 16:00:17 +0300, Stas Kelvich wrote:
>>> On 29 Nov 2018, at 18:21, Dmitry Dolgov <9erthali...@gmail.com> wrote:
>>> Is there any resulting patch where the ideas how to implement this are 
>>> outlined?
>> 
>> Not yet. I’m going to continue work on this in January. And probably try to
>> force some of nearby committers to make a line by line review.
> 
> This hasn't happened yet, so I think this ought to be marked ad returned
> with feedback?
> 

No objections. I don't think this will realistically go in during last CF, so
will open it during next release cycle.

--
Stas Kelvich
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: Installation instructions update (pg_ctl)

[Michael Banck]

Hi,

On Sat, Nov 03, 2018 at 09:56:46PM +0100, Andreas 'ads' Scherbaum wrote:
> >That's indeed one replacement too much.
> >
> >The attached patch is fixing this.
> >
> >Thank you for the review!
> >
> 
> Submitted this to the Commitfest, and took the liberty to add you as a
> reviewer.

Sorry, I didn't look at it during that (November) commitfest cause it
was submitted late, and then forgot about it.

I think the changes are fine and I've marked it Ready for Committer.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

Re: Statement-level rollback

[Greg Stark]

On Fri 7 Dec 2018, 21:43 Robert Haas  On Fri, Dec 7, 2018 at 3:34 PM Alvaro Herrera 
> wrote:
> > Yeah, I agree that this downside is real.  I think our only protection
> > against that is to say "don't do that".  Like any other tool, it has
> > upsides and downsides; we shouldn't keep it away from users only because
> > they might misuse it.
>
> I have a hard time arguing against that given that EDB has this thing
> in our bag of tricks, but if it weren't for that I'd be fighting
> against this tooth and nail.  Behavior-changing GUCs sck.
>

This looks like repeating the autocommit mistakes of the past.

And based on that experience wouldn't the replacement approach be to do
this client side? If libpq had a library option to wrap every statement in
a subtransaction by adding begin/end then this problem would be completely
sidestepped.

>

Re: MERGE SQL statement for PG12

[Alvaro Herrera]

I'm going to close this patch as returned with feedback, since it's had
plenty and it's pretty clear that it will take some time to address it.
Pavan is welcome to resubmit when he has a new version.

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

Re: WIP: Avoid creation of the free space map for small tables

[John Naylor]

On Thu, Jan 31, 2019 at 4:06 PM Amit Kapila  wrote:
> I don't think that moving fsm tests to brin would be a good approach.
> We want to have a separate test for each access method.  I think if we
> want to do something to avoid portability issues, maybe we can do what
> Masahiko San has just suggested.

We could also use the same plpgsql loop as in fsm.sql to check the ctid, right?

> OTOH, I think we are just good w.r.t
> this issue with the last patch I sent.  I think unless we see some
> problem here, we should put energy into having a reproducible test for
> the fourth problem mentioned in my mail up thread [1].  Do you think
> it makes sense to run make check in loop for multiple times or do you
> have any idea how we can have a reproducible test?

Okay. Earlier I tried running make installcheck with
force_parallel_mode='regress', but didn't get a failure. I may not
have run enough times, though. I'll have to think about how to induce
it.

-- 
John Naylorhttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] [PATCH] Generic type subscripting

[Dmitry Dolgov]

> On Thu, Jan 31, 2019 at 4:39 PM Alvaro Herrera  
> wrote:
>
> That having been said ... while the current 0001 patch does apply
> semi-cleanly (`git apply -3` does it), it does not compile, probably
> because of header refactoring.  Please rebase and make sure that each
> individual patch compiles cleanly.

Oh, sorry for that, I'll fix it in a moment.

Re: [HACKERS] [PATCH] Generic type subscripting

[Pavel Stehule]

čt 31. 1. 2019 v 16:39 odesílatel Alvaro Herrera 
napsal:

> Would anybody object to me pushing part 0001 soon?  It seems pointless
> to force Dmitry keep rebasing a huge renaming patch all this time.  I
> think the general feeling is that this is a desirable change, so let's
> keep things moving.
>
> That having been said ... while the current 0001 patch does apply
> semi-cleanly (`git apply -3` does it), it does not compile, probably
> because of header refactoring.  Please rebase and make sure that each
> individual patch compiles cleanly.
>

+1

Pavel

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

Re: Global snapshots

[Andres Freund]

Hi,

On 2018-11-30 16:00:17 +0300, Stas Kelvich wrote:
> > On 29 Nov 2018, at 18:21, Dmitry Dolgov <9erthali...@gmail.com> wrote:
> > Is there any resulting patch where the ideas how to implement this are 
> > outlined?
> 
> Not yet. I’m going to continue work on this in January. And probably try to
> force some of nearby committers to make a line by line review.

This hasn't happened yet, so I think this ought to be marked ad returned
with feedback?

- Andres

Re: A few new options for vacuumdb

[Bossart, Nathan]

On 1/30/19, 8:21 PM, "Michael Paquier"  wrote:
> Fixed and committed.  So we are finally done here, except for the
> debate with the relation size.

Thanks for the diligent reviews, as always.  I don't plan to pick up
--min-relation-size right now, but I may attempt it again in a future
commitfest.

Nathan

Re: [HACKERS] [PATCH] Generic type subscripting

[Alvaro Herrera]

Would anybody object to me pushing part 0001 soon?  It seems pointless
to force Dmitry keep rebasing a huge renaming patch all this time.  I
think the general feeling is that this is a desirable change, so let's
keep things moving.

That having been said ... while the current 0001 patch does apply
semi-cleanly (`git apply -3` does it), it does not compile, probably
because of header refactoring.  Please rebase and make sure that each
individual patch compiles cleanly.

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

Re: PostgreSQL vs SQL/XML Standards

[Pavel Stehule]

so 26. 1. 2019 v 1:38 odesílatel Chapman Flack 
napsal:

> On 01/25/19 00:45, Pavel Stehule wrote:
> > pá 25. 1. 2019 v 5:46 odesílatel Chapman Flack 
> > napsal:
> >> I am:
> >> - re-attaching xmltable-xpath-result-processing-bugfix-5.patch unchanged
> >>   (just so CF app does not lose track)
> >> - re-attaching xmltable-xmlexists-passing-mechanisms-1.patch unchanged
> >> - attaching for the first time xml-functions-type-docfix-1.patch
> >>
> >> The doc patch is made to go on top of the passing-mechanisms patch
>
> Realized xmltable-xmlexists-passing-mechanisms-1.patch didn't add
> a regression test.  Here attaching (or re-attaching):
>
> - xmltable-xpath-result-processing-bugfix-5.patch - unchanged
> - xmltable-xmlexists-passing-mechanisms-2.patch - now with test
> - xml-functions-type-docfix-1.patch - unchanged
>
> I'll venture a review opinion that all of this applies, builds, and passes
> check-world on top of 18c0da8, and that, of the issues I had identified at
> the start of this thread, these changes resolve the ones they set out to
> resolve.
>
> But the second two patches are my own work, so another reviewer is needed.
> The passing-mechanisms patch is tiny while the docfix patch is not, so
> there's an opening for a reviewer with an interest in documentation. :)
>
> There is still nothing in this patch set to address [1], though that
> also seems worth doing, perhaps in another patch, and probably not
> difficult, perhaps needing only a regex.
>
> And of course we're still saddled with all the unfixable limits
> of XPath 1.0; this patch set is fixing a few peripheral fixable things
> around that.
>
>
I am sending a review of these patches

xmltable-xpath-result-processing-bugfix-5.patch - I'll skip it - just all
tests passed

xmltable-xmlexists-passing-mechanisms-2.patch - this patch introduce new
PASSING mechanism BY VALUE - it is just syntactic sugar due compatibility
with standard. It is unhappy so previous implementation was broken and
introduced "BY REF" instead "BY VALUE", but this bug should be fixed 10
years ago. It change nothing, all tests passed and the documentation looks
ok.

Last patch is documentation only patch - I am thinking so the difference
and limits our implementation of XPath based functions are described well
and correctly.

I'll mark this patch as ready for commiters.

Regards

Pavel


> -Chap
>
>
> [1]
> https://www.postgresql.org/message-id/5BD1C44B.6040300%40anastigmatix.net
>

Re: [HACKERS] [PATCH] WIP Add ALWAYS DEFERRED option for constraints

[Andres Freund]

Hi,

On 2018-11-29 19:04:24 +0100, Dmitry Dolgov wrote:
> > On Tue, Oct 2, 2018 at 7:43 AM Michael Paquier  wrote:
> >
> > On Thu, Jul 12, 2018 at 03:07:33PM -0400, Robbie Harwood wrote:
> > > This patch looks good to me.  +1; Álvaro, please update the CF entry
> > > when you're also satisfied.
> >
> > The patch set does not apply anymore, so I have moved it to next CF,
> > waiting on author.
> 
> Nothing was changed since then. Since the patch got some interest I'm moving 
> it
> to the next CF, but please post the rebased version.

Since this hasn't happened yet, I'm going to mark it as returned with
feedback.

Greetings,

Andres Freund

Re: WIP: Avoid creation of the free space map for small tables

[Amit Kapila]

On Thu, Jan 31, 2019 at 7:23 PM John Naylor  wrote:
>
> On Thu, Jan 31, 2019 at 1:52 PM John Naylor  
> wrote:
> >
> > On Thu, Jan 31, 2019 at 1:43 PM Amit Kapila  wrote:
> > > > I have an idea -- instead of adding a bunch of records and hoping that
> > > > the relation size and free space is consistent across platforms, how
> > > > about we revert to the original test input, and add a BRIN index? That
> > > > should have a FSM even with one record.
> > > >
> > >
> > > Why would BRIN index allow having FSM for heap relation?
> >
> > Oops, I forgot this file is for testing heaps only. That said, we
> > could possibly put most of the FSM tests such as
> >
> > SELECT * FROM fsm_page_contents(get_raw_page('test_rel_forks', 'fsm', 0));
> >
> > into brin.sql since we know a non-empty BRIN index will have a FSM.
>
> As in the attached. Applies on top of v20. First to revert to HEAD,
> second to move FSM tests to brin.sql. This is a much less invasive and
> more readable patch, in addition to being hopefully more portable.
>

I don't think that moving fsm tests to brin would be a good approach.
We want to have a separate test for each access method.  I think if we
want to do something to avoid portability issues, maybe we can do what
Masahiko San has just suggested.  OTOH, I think we are just good w.r.t
this issue with the last patch I sent.  I think unless we see some
problem here, we should put energy into having a reproducible test for
the fourth problem mentioned in my mail up thread [1].  Do you think
it makes sense to run make check in loop for multiple times or do you
have any idea how we can have a reproducible test?

[1] - 
https://www.postgresql.org/message-id/CAA4eK1L%3DqWp_bJ5aTc9%2Bfy4Ewx2LPaLWY-RbR4a60g_rupCKnQ%40mail.gmail.com

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: WIP: Avoid creation of the free space map for small tables

[Masahiko Sawada]

On Thu, Jan 31, 2019 at 6:41 AM Amit Kapila  wrote:
>
> On Wed, Jan 30, 2019 at 10:41 PM Masahiko Sawada  
> wrote:
> >
> > On Wed, Jan 30, 2019 at 4:33 AM Amit Kapila  wrote:
> > >
> > > On Tue, Jan 29, 2019 at 8:12 PM John Naylor  
> > > wrote:
> > > >
> > > > On Tue, Jan 29, 2019 at 11:56 AM Amit Kapila  
> > > > wrote:
> > > >
> > > > > You can find this change in attached patch.  Then, I ran the make
> > > > > check in src/bin/pgbench multiple times using test_conc_insert.sh.
> > > > > You can vary the number of times the test should run, if you are not
> > > > > able to reproduce it with this.
> > > > >
> > > > > The attached patch (clear_local_map_if_exists_1.patch) atop the main
> > > > > patch fixes the issue for me.  Kindly verify the same.
> > > >
> > > > I got one failure in 50 runs. With the new patch, I didn't get any
> > > > failures in 300 runs.
> > > >
> > >
> > > Thanks for verification.  I have included it in the attached patch and
> > > I have also modified the page.sql test to have enough number of pages
> > > in relation so that FSM will get created irrespective of alignment
> > > boundaries.  Masahiko San, can you verify if this now works for you?
> > >
> >
> > Thank you for updating the patch!
> >
> > The modified page.sql test could fail if the block size is more than
> > 8kB?
>
> That's right, but I don't think current regression tests will work for
> block size greater than 8KB.  I have tried with 16 and 32 as block
> size, there were few failures on the head itself.

Understood. That means that no build farm configures other block size than 8kB.

>
> > We can ensure the number of pages are more than 4 by checking it
> > and adding more data if no enough but I'm really not sure we should
> > care the bigger-block size cases.
> >
>
> Yeah, I am not sure either.  I think as this is an existing test, we
> should not try to change it too much.  However, if both you and John
> feel it is better to change, we can go with that.
>

So I think the patch you proposed looks good to me but how about
adding the check whether the table is more than 4 pages? For example,

SELECT (pg_relation_size('test_rel_forks') /
current_setting('block_size')::int) > 4;


Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: interval type additional option

[Pavel Stehule]

čt 31. 1. 2019 v 15:33 odesílatel Andrew Gierth 
napsal:

> > "Pavel" == Pavel Stehule  writes:
>
>  Pavel> Hi
>  Pavel> I have a value '400 days 30 sec':: interval
>
>  Pavel> I don't understand why cast
>
>  Pavel> postgres=# select '400days 30sec'::interval minute to second;
>
>  ...
>
>  Pavel> What is sense of this feature?
>
> As far as I know it only exists because of the sql spec, and the
> implementation has never been complete or working. I think it does
> affect some input parsing cases, there are some nasty hacks here and
> there to support it, but given that our intervals don't behave like the
> spec's ones anyway, there seems to be little motivation to make it work
> fully.
>

yes - it looks little bit bizarre

>
> These work, for example:
>
> select interval '4' year to month;  -- 4 months
> select interval '4' hour;   -- 4 hours
> select interval '4' hour to second; -- 4 seconds
> select '4'::interval year to month; -- 4 months
> etc.
>
>
thank you for info

Pavel

-- 
> Andrew (irc:RhodiumToad)
>

Re: interval type additional option

[Andrew Gierth]

> "Pavel" == Pavel Stehule  writes:

 Pavel> Hi
 Pavel> I have a value '400 days 30 sec':: interval

 Pavel> I don't understand why cast

 Pavel> postgres=# select '400days 30sec'::interval minute to second;

 ...

 Pavel> What is sense of this feature?
 
As far as I know it only exists because of the sql spec, and the
implementation has never been complete or working. I think it does
affect some input parsing cases, there are some nasty hacks here and
there to support it, but given that our intervals don't behave like the
spec's ones anyway, there seems to be little motivation to make it work
fully.

These work, for example:

select interval '4' year to month;  -- 4 months
select interval '4' hour;   -- 4 hours
select interval '4' hour to second; -- 4 seconds
select '4'::interval year to month; -- 4 months
etc.

-- 
Andrew (irc:RhodiumToad)

Re: Implementing Incremental View Maintenance

[Yugo Nagata]

On Mon, 7 Jan 2019 00:39:00 -0800
Mitar  wrote:

> That sounds great! I am interested in this topic because I am
> interested in reactive/live queries and support for them in
> PostgreSQL. [1]
> 
> In that context, the problem is very similar: based on some state of
> query results and updated source tables, determine what should be new
> updates to send to the client describing changes to the query results.
> So after computing those incremental changes, instead of applying them
> to materialized view I would send them to the client. One could see
> materialized views only type of consumers of such information about
> incremental change.
> 
> So I would like to ask if whatever is done in this setting is done in
> a way that one could also outside of the context of materialized view.
> Not sure what would API be thought.

I didn't know about reactive/live queries but this seems share a part of
problem with IVM, so we might have common API.

BTW, what is uecase of reactive/live queries? (just curious)
 
> > For these reasons, we started to think to implement IVM without relying on 
> > OIDs
> > and made a bit more surveys.
> 
> I also do not see much difference between asking users to have primary
> key on base tables or asking them to have OIDs. Why do you think that
> a requirement for primary keys is a hard one? I think we should first
> focus on having IVM with base tables with primary keys. Maybe then
> later on we could improve on that and make it also work without.
>  
> To me personally, having unique index on source tables and also on
> materialized view is a reasonable restriction for this feature.
> Especially for initial versions of it.

Initially, I chose to use OIDs for theoretical reason, that is, to handle
"bag-semantics" which allows duplicate rows in tables. However, I agree
that we start from the restriction of having unique index on base tables.
 
> > If we can represent a change of UPDATE on a base table as query-like rather 
> > than
> > OLD and NEW, it may be possible to update the materialized view directly 
> > instead
> > of performing delete & insert.
> 
> Why do you need OLD and NEW? Don't you need just NEW and a list of
> columns which changed from those in NEW? I use such diffing query [4]
> to represent changes: first column has a flag telling if the row is
> representing insert, update, and remove, the second column tells which
> column are being changed in the case of the update, and then the NEW
> columns follow.

According the change propagation equation approach, OLD is necessary
to calculate tuples in MV to be deleted or modified. However, if tables
has unique keys, such tuples can be identifeid using the keys, so
OLD may not be needed, at least in eager approach.

In lazy approach, OLD contents of table is useful. For example, with
a join view MV = R * S, when dR is inserted into R and dS is inserted
into S, the delta to be inserted into MV will be
 
 dMV = (R_old * dS) + (dR * S_new)
= (R_old * dS) + (dR * S_old) + (dR * dS)

, hence the old contents of tables R and S are needed.
 
> I think that maybe standardizing structure for representing those
> changes would be a good step towards making this modular and reusable.
> Because then we can have three parts:
> 
> * Recording and storing changes in a standard format.
> * A function which given original data, stored changes, computes
> updates needed, also in some standard format.
> * A function which given original data and updates needed, applies them.

> I think if we split things into three parts as I described above, then
> this is just a question of configuration. Or you call all three inside
> one trigger to update in "eager" fashion. Or you store computed
> updates somewhere and then on demand apply those in "lazy" fashion.

I agree that defining the format to represent changes is important. However,
I am not sure both of eager and lazy can be handled in the same manner. I'll
consider about this more.

> > In the eager maintenance approache, we have to consider a race condition 
> > where two
> > different transactions change base tables simultaneously as discussed in 
> > [4].
> 
> But in the case of "lazy" maintenance there is a mirror problem: what
> if later changes to base tables invalidate some previous change to the
> materialized view. Imagine that one cell in a base table is first
> updated too "foo" and we compute an update for the materialized view
> to set it to "foo". And then the same cell is updated to "bar" and we
> compute an update for the materialized view again. If we have not
> applied any of those updates (because we are "lazy") now the
> previously computed update can be discarded. We could still apply
> both, but it would not be efficient.

In our PoC implementation, I handled this situation by removing
old contents from NEW delata table. In your example, when the base
table is updated from "foo" to "bar", the "foo" tuple is removed
from and the "bar" tuple is inserted in NEW delta an

Re: Allow auto_explain to log to NOTICE

[Daniel Gustafsson]

> On 5 Jan 2019, at 01:04, Michael Paquier  wrote:
> 
> On Fri, Jan 04, 2019 at 02:45:55PM +0100, Daniel Gustafsson wrote:
>> (Michael: sorry for not having responded to your comments on the patch, $life
>> has had little time over for hacking lately)
> 
> No worries, I understand.
> 
>> There is that.  We might not be excited about writing tests for this
>> contrib module but someone else might want to use this for testing
>> their application in a similar manner to how pg_regress tests?
> 
> If we don't introduce it, there is no way to know.  Still if somebody
> needs to create regression tests they could just use EXPLAIN for the
> same purpose.  However, the option still seems useful to me to get out
> plans with the most generic output, so I support the idea.  If others
> don't feel so, I am fine to give up as well.

This didn’t attract any other interested parties, and I don’t feel the added
complexity is worth a continued discussion, so I’m closing this patch.  Thanks
for the review.

cheers ./daniel

Re: Add client connection check during the execution of the query

[Andres Freund]

Hi,

On 2019-01-13 18:05:39 -0500, Tom Lane wrote:
> s.cherkas...@postgrespro.ru writes:
> > This patch adds verification of the connection with the client during 
> > the execution of the SQL query. The feature enables using the GUC 
> > variable ‘client_connection_check_interval’. The default check interval 
> > is 1 second. If you set the value of ‘client_connection_check_interval’ 
> > to 0, then the check will not be performed.
> 
> I took a quick look through this.
> 
> * It won't even compile on non-Linux platforms, because MSG_DONTWAIT
> is a Linux-ism.  Perhaps that can be replaced by putting the client
> socket into nonblock mode, but I'm not very sure that that'll work
> (especially when using OpenSSL or another TLS implementation).
> 
> * I'm not convinced that this will reliably detect client connection loss.
> AFAICS, if there is any unread data pending, it'd report that all is well
> even if the client dropped off the net after sending that data.  It's hard
> to evaluate how likely such a situation is, but one really obvious case
> is that the client might've sent an 'X' message to try to close the
> connection gracefully.  Also, use of TLS would again make things much
> harder to reason about, because the TLS layer may send or receive data
> that we don't know about.
> 
> * The management of the pending timeout interrupt seems like a mess.
> Why did you involve ProcessInterrupts in that?  It seems likely to queue
> extra timeouts at random times due to unrelated interrupts causing that
> bit of code to run, and/or cause weird gaps in the timeout intervals due
> to not being run promptly.  I'd be inclined to set this up so that the
> timeout handler itself re-queues the timeout (I think that will work, or
> if not, we should probably fix timeout.c so that it can).
> 
> * BTW, I am not on board with making this enabled-by-default.
> 
> This does seem like possibly a useful option if we can make it
> work portably/reliably, but I don't have very high hopes for that.

Given that nothing happened since this message, and the commitfest is
ending, I'm going to mark this as returned with feedback.

Greetings,

Andres Freund

Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)

[Pavel Stehule]

čt 31. 1. 2019 v 14:44 odesílatel Andres Freund  napsal:

> Hi,
>
> On 2018-09-18 11:33:38 -0400, Tom Lane wrote:
> > Pavel Stehule  writes:
> > > [ xml-xpath-default-ns-7.patch ]
> >
> > At Andrew's prompting, I took a look over this patch.  I don't know much
> > of anything about XML, so I have no idea as to standards compliance here,
> > but I do have some comments:
> >
> > * I'm fairly uncomfortable with the idea that we're going to maintain
> > our own XPath parser.  That seems like a recipe for lots of future
> > work ... and the code is far too underdocumented for anyone to actually
> > maintain it.  (Case in point: _transformXPath's arguments are not
> > documented at all, and in fact there's hardly a word about what it's
> > even supposed to *do*.)
>
> We were looking at this patch at the pgday developer meeting: Our
> impression is that this patch should be rejected. If really desired, the
> best approach seems to be actually implement this in libxml, and then go
> from there.
>

Unfortunately,  the development of libxml2 is frozen.

I have to accept it.

Regards

Pavel



> Greetings,
>
> Andres Freund
>

Re: WIP: Avoid creation of the free space map for small tables

[John Naylor]

On Thu, Jan 31, 2019 at 1:52 PM John Naylor  wrote:
>
> On Thu, Jan 31, 2019 at 1:43 PM Amit Kapila  wrote:
> > > I have an idea -- instead of adding a bunch of records and hoping that
> > > the relation size and free space is consistent across platforms, how
> > > about we revert to the original test input, and add a BRIN index? That
> > > should have a FSM even with one record.
> > >
> >
> > Why would BRIN index allow having FSM for heap relation?
>
> Oops, I forgot this file is for testing heaps only. That said, we
> could possibly put most of the FSM tests such as
>
> SELECT * FROM fsm_page_contents(get_raw_page('test_rel_forks', 'fsm', 0));
>
> into brin.sql since we know a non-empty BRIN index will have a FSM.

As in the attached. Applies on top of v20. First to revert to HEAD,
second to move FSM tests to brin.sql. This is a much less invasive and
more readable patch, in addition to being hopefully more portable.

> And in page.sql we could just have a test that the table has no FSM.

This is not possible, since we don't know the relfilenode for the
error text, and it's not important. Better to have everything in
brin.sql.

-- 
John Naylorhttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
commit 18b444cdfc2c50502389b84f4504ec9c7f2ab831
Author: John Naylor 
Date:   Thu Jan 31 14:20:14 2019 +0100

revert contrib so I can redo

diff --git a/contrib/pageinspect/expected/page.out b/contrib/pageinspect/expected/page.out
index 89b73ca991..3fcd9fbe6d 100644
--- a/contrib/pageinspect/expected/page.out
+++ b/contrib/pageinspect/expected/page.out
@@ -1,69 +1,48 @@
 CREATE EXTENSION pageinspect;
-CREATE TABLE test_rel_forks (a int);
--- Make sure there are enough blocks in the heap for the FSM to be created.
-INSERT INTO test_rel_forks SELECT i from generate_series(1,2000) i;
--- set up FSM and VM
-VACUUM test_rel_forks;
+CREATE TABLE test1 (a int, b int);
+INSERT INTO test1 VALUES (16777217, 131584);
+VACUUM test1;  -- set up FSM
 -- The page contents can vary, so just test that it can be read
 -- successfully, but don't keep the output.
-SELECT octet_length(get_raw_page('test_rel_forks', 'main', 0)) AS main_0;
+SELECT octet_length(get_raw_page('test1', 'main', 0)) AS main_0;
  main_0 
 
8192
 (1 row)
 
-SELECT octet_length(get_raw_page('test_rel_forks', 'main', 100)) AS main_100;
-ERROR:  block number 100 is out of range for relation "test_rel_forks"
-SELECT octet_length(get_raw_page('test_rel_forks', 'fsm', 0)) AS fsm_0;
+SELECT octet_length(get_raw_page('test1', 'main', 1)) AS main_1;
+ERROR:  block number 1 is out of range for relation "test1"
+SELECT octet_length(get_raw_page('test1', 'fsm', 0)) AS fsm_0;
  fsm_0 
 ---
   8192
 (1 row)
 
-SELECT octet_length(get_raw_page('test_rel_forks', 'fsm', 20)) AS fsm_20;
-ERROR:  block number 20 is out of range for relation "test_rel_forks"
-SELECT octet_length(get_raw_page('test_rel_forks', 'vm', 0)) AS vm_0;
+SELECT octet_length(get_raw_page('test1', 'fsm', 1)) AS fsm_1;
+ fsm_1 
+---
+  8192
+(1 row)
+
+SELECT octet_length(get_raw_page('test1', 'vm', 0)) AS vm_0;
  vm_0 
 --
  8192
 (1 row)
 
-SELECT octet_length(get_raw_page('test_rel_forks', 'vm', 1)) AS vm_1;
-ERROR:  block number 1 is out of range for relation "test_rel_forks"
+SELECT octet_length(get_raw_page('test1', 'vm', 1)) AS vm_1;
+ERROR:  block number 1 is out of range for relation "test1"
 SELECT octet_length(get_raw_page('xxx', 'main', 0));
 ERROR:  relation "xxx" does not exist
-SELECT octet_length(get_raw_page('test_rel_forks', 'xxx', 0));
+SELECT octet_length(get_raw_page('test1', 'xxx', 0));
 ERROR:  invalid fork name
 HINT:  Valid fork names are "main", "fsm", "vm", and "init".
-SELECT * FROM fsm_page_contents(get_raw_page('test_rel_forks', 'fsm', 0));
- fsm_page_contents 

- 0: 39+
- 1: 39+
- 3: 39+
- 7: 39+
- 15: 39   +
- 31: 39   +
- 63: 39   +
- 127: 39  +
- 255: 39  +
- 511: 39  +
- 1023: 39 +
- 2047: 39 +
- 4095: 39 +
- fp_next_slot: 0  +
- 
-(1 row)
-
-SELECT get_raw_page('test_rel_forks', 0) = get_raw_page('test_rel_forks', 'main', 0);
+SELECT get_raw_page('test1', 0) = get_raw_page('test1', 'main', 0);
  ?column? 
 --
  t
 (1 row)
 
-DROP TABLE test_rel_forks;
-CREATE TABLE test1 (a int, b int);
-INSERT INTO test1 VALUES (16777217, 131584);
 SELECT pagesize, version FROM page_header(get_raw_page('test1', 0));
  pagesize | version 
 --+-
@@ -83,6 +62,26 @@ SELECT tuple_data_split('test1'::regclass, t_data, t_infomask, t_infomask2, t_bi
  {"\\x0101","\\x00020200"}
 (1 row)
 
+SELECT * FROM fsm_page_contents(get_raw_page('test1', 'fsm', 0));
+ fsm_page_contents 
+---
+ 0: 254   +
+ 1: 254   +
+ 3: 254   +
+ 7: 254   +
+ 15: 254  +
+ 31: 254  +
+ 63: 254  +
+ 127:

Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)

[Andres Freund]

Hi,

On 2018-09-18 11:33:38 -0400, Tom Lane wrote:
> Pavel Stehule  writes:
> > [ xml-xpath-default-ns-7.patch ]
> 
> At Andrew's prompting, I took a look over this patch.  I don't know much
> of anything about XML, so I have no idea as to standards compliance here,
> but I do have some comments:
> 
> * I'm fairly uncomfortable with the idea that we're going to maintain
> our own XPath parser.  That seems like a recipe for lots of future
> work ... and the code is far too underdocumented for anyone to actually
> maintain it.  (Case in point: _transformXPath's arguments are not
> documented at all, and in fact there's hardly a word about what it's
> even supposed to *do*.)

We were looking at this patch at the pgday developer meeting: Our
impression is that this patch should be rejected. If really desired, the
best approach seems to be actually implement this in libxml, and then go
from there.

Greetings,

Andres Freund

interval type additional option

[Pavel Stehule]

Hi

I have a value '400 days 30 sec':: interval

I don't understand why cast

postgres=# select '400days 30sec'::interval minute to second;
+---+
| interval  |
+---+
| 400 days 00:00:30 |
+---+
(1 row)

produce 400 days

or

postgres=# create table foo(a interval minute to second);
CREATE TABLE
postgres=# insert into foo values('400 days');
INSERT 0 1
postgres=# select * from foo;
+--+
|a |
+--+
| 400 days |
+--+
(1 row)

What is sense of this feature?

Regards

Pavel

Re: Add --include-table-data-where option to pg_dump, to export only a subset of table data

[Andres Freund]

Hi,

On 2018-11-29 13:50:21 +0100, Dmitry Dolgov wrote:
> Unfortunately, current version of this patch has some conflicts, could you
> rebase it?

Given this patch has not been updated, I'm marking this as returned with 
feedback.

Greetings,

Andres Freund

Re: allow online change primary_conninfo

[Sergei Kornilov]

Hello

Yeah, we have no consensus.

Another open question is about logging new primary_conninfo:

> LOG: parameter "primary_conninfo" changed to "host=/tmp port=5432 
> password=hoge"

I my opinion this is not issue, database logs can have sensitive data. User 
queries, for example.
If we not want expose such info - it is ok just hide new value from logs with 
new GUC flag? Or i need implement masked conninfo for this purpose?

regards, Sergei

Re: bug tracking system

[Alvaro Herrera]

On 2019-Jan-31, Alvaro Herrera wrote:

> Here's a bug tracking system that Nathan set up many years ago and
> apparently has kept going unattended.  It seems to me that it's
> something that we could base a semi-official bug tracking system on.

I didn't really remember that Nathan's system had been discussed so
widely.  Here are the two main threads there were as far as I could
find:

https://postgr.es/m/20151004224254.ga1...@granicus.if.org
https://postgr.es/m/20151006130524.ga4...@granicus.if.org

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

Re: allow online change primary_conninfo

[and...@anarazel.de]

Hi,

On 2018-12-14 16:55:42 +0900, Michael Paquier wrote:
> On Thu, Dec 13, 2018 at 01:51:48PM +0300, Sergei Kornilov wrote:
> > Depends on this discussion: 
> > https://www.postgresql.org/message-id/20181212053042.gk17...@paquier.xyz
> > If walreceiver will use GUC conninfo for startup - then yes, we can
> > remove such static variables.  If walreceiver will still use conninfo
> > from WalRcv - we have race condition between RequestXLogStreaming from
> > startup, config reload, and walreceiver start. In this case I need
> > save conninfo from WalRcv and compare new GUC value with them.
> 
> I would recommend waiting for the conclusion of other thread before
> moving on with this one.  There are arguments for letting the startup
> process deciding which connection string the WAL receiver should use as
> well as letting the WAL receiver use directly the connection string from
> the GUC.

I suggest continuing the development of the patch without relying on
that refactoring.

Greetings,

Andres Freund

Re: tab-completion debug print

[Andres Freund]

Hi,

On 2018-12-14 08:45:49 +0100, Peter Eisentraut wrote:
> On 14/12/2018 06:41, David Fetter wrote:
> > There are problems tab completion, e.g. its catalog queries, could
> > cause in production that would be difficult to simulate elsewhere.
> 
> Where are all these reports of tab completion query problems that we are
> building elaborate infrastructure to diagnose?

My impression is that this patch ought to marked as rejected?

Greetings,

Andres Freund

Re: WIP: Avoid creation of the free space map for small tables

[John Naylor]

On Thu, Jan 31, 2019 at 1:43 PM Amit Kapila  wrote:
> > I have an idea -- instead of adding a bunch of records and hoping that
> > the relation size and free space is consistent across platforms, how
> > about we revert to the original test input, and add a BRIN index? That
> > should have a FSM even with one record.
> >
>
> Why would BRIN index allow having FSM for heap relation?

Oops, I forgot this file is for testing heaps only. That said, we
could possibly put most of the FSM tests such as

SELECT * FROM fsm_page_contents(get_raw_page('test_rel_forks', 'fsm', 0));

into brin.sql since we know a non-empty BRIN index will have a FSM.
And in page.sql we could just have a test that the table has no FSM.

-- 
John Naylorhttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Reorganize collation lookup time and place

[Andres Freund]

Hi,

On 2018-12-13 14:50:53 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > On 2018-12-13 15:05:40 +0100, Peter Eisentraut wrote:
> >> On 12/12/2018 18:56, Andres Freund wrote:
> >>> Why isn't this integrated into fmgr_info()?
> 
> >> fmgr_info() looks up stuff in pg_proc.  pg_newlocale_...() looks up
> >> stuff in pg_collation.  There is no overlap between them.
> 
> > It looks up stuff necessary for calling a function, that doesn't fit
> > looking up the collation necessary to do so too badly. A lot of the the
> > changes you made are rote changes to each caller, taking the collation
> > oid and expanding it with pg_newlocale_from_collation().
> 
> I'm not very thrilled with the idea of changing every single caller of
> InitFunctionCallInfoData and related functions, especially when exactly
> zero functional change ensues.  We should work harder on avoiding that
> code churn; extension developers will thank us.
> 
> >> There is also not necessarily a one-to-one correspondence between
> >> function and collation lookup calls.  For example, in some cases you
> >> need to look up a sorting and a hashing functions, but only one
> >> collation for both.
> 
> > That seems rare enough not to matter, performancewise.
> 
> I think it potentially does matter, but I also agree that it's not
> the common case.  Could we perhaps keep the API for the existing
> functions the same, and introduce new functions alongside them
> to be used by the small number of places where it matters?
> 
> (I've not looked at Peter's patch yet, so maybe I'm talking through
> my hat.  But we should set a pretty high value on avoiding code
> churn in stuff as widely used as the fmgr interfaces.)

So, what's the plan here? Should the CF entry be closed?

Greetings,

Andres Freund

Re: WIP: Avoid creation of the free space map for small tables

[Amit Kapila]

On Thu, Jan 31, 2019 at 2:12 PM John Naylor  wrote:
>
> On Thu, Jan 31, 2019 at 6:41 AM Amit Kapila  wrote:
> >
> > On Wed, Jan 30, 2019 at 10:41 PM Masahiko Sawada  
> > wrote:
> > >
> > > The modified page.sql test could fail if the block size is more than
> > > 8kB?
> >
> > That's right, but I don't think current regression tests will work for
> > block size greater than 8KB.  I have tried with 16 and 32 as block
> > size, there were few failures on the head itself.
> >
> > > We can ensure the number of pages are more than 4 by checking it
> > > and adding more data if no enough but I'm really not sure we should
> > > care the bigger-block size cases.
> > >
> >
> > Yeah, I am not sure either.  I think as this is an existing test, we
> > should not try to change it too much.  However, if both you and John
> > feel it is better to change, we can go with that.
>
> I have an idea -- instead of adding a bunch of records and hoping that
> the relation size and free space is consistent across platforms, how
> about we revert to the original test input, and add a BRIN index? That
> should have a FSM even with one record.
>

Why would BRIN index allow having FSM for heap relation?

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: Implementing Incremental View Maintenance

[Yugo Nagata]

On Tue, 1 Jan 2019 14:46:25 +0700
Nguyễn Trần Quốc Vinh  wrote:

> We have some result on incremental update for MVs. We generate triggers on
> C to do the incremental maintenance. We posted the code to github about 1
> year ago, but unfortunately i posted a not-right ctrigger.h header. The
> mistake was exposed to me when a person could not compile the generated
> triggers and reported to me. And now i re-posted with the right ctrigger.h
> file.
> 
> You can find the codes of the generator here:
> https://github.com/ntqvinh/PgMvIncrementalUpdate/commits/master. You can
> find how did we do here:
> https://link.springer.com/article/10.1134/S0361768816050066. The paper is
> about generating of codes in pl/pgsql. Anyway i see it is useful for
> reading the codes. I don't know if i can share the paper or not so that i
> don't publish anywhere else. The text about how to generate triggers in C
> was published with open-access but unfortunately, it is in Vietnamese.
> 
> We are happy if the codes are useful for someone.

I have read your paper. It is interesting and great so that the algorithm
is described concretely.

After reading this, I have a few questions about your implementation.
Although I may be able to understand by reading your paper and code carefully,
I would appreciate it if you could answer these.

- It is said there are many limitations on the view definition query.
  How does this work when the query is not supported?

- Is it possible to support materialized views that have DISTINCT,
  OUTER JOIN, or sub-query in your approach?

- It is said that AVG is splitted to SUM and COUNT. Are these new additional
  columns in MV visible for users?

- Does this can handle the race condition discussed in [1], that is,
  if concurrent transactions update different two tables in the join
  view definition, is MV updated sucessfully?

[1] 
https://www.postgresql.org/message-id/flat/1368561126.64093.YahooMailNeo%40web162904.mail.bf1.yahoo.com

Regards,
-- 
Yugo Nagata 

Re: Statement-level rollback

[Andres Freund]

On 2018-12-08 17:55:03 -0300, Alvaro Herrera wrote:
> On 2018-Dec-08, Andres Freund wrote:
> 
> > On 2018-12-08 17:10:27 -0300, Alvaro Herrera wrote:
> 
> > > This is what patch 0001 does -- it's only allowed in the connection
> > > string, or on ALTER USER / ALTER DATABASE.  Setting it in
> > > postgresql.conf is forbidden, as well as changing from transaction to
> > > statement in SET (the opposite is allowed, though.)
> > 
> > I don't think allowing to set it on a per-user basis is acceptable
> > either, it still leaves the client in a state where they'll potentially
> > be confused about it.
> 
> Hmm, true.
> 
> > Do you have a proposal to address the issue that this makes it just
> > about impossible to write UDFs in a safe way?
> 
> Not yet, but I'll give it a think next week.

Is this still in development? Or should we mark this as returned with
feedback?

Greetings,

Andres Freund

Re: WIP: Avoid creation of the free space map for small tables

[Amit Kapila]

On Thu, Jan 31, 2019 at 2:02 PM John Naylor  wrote:
>
> On Thu, Jan 31, 2019 at 6:37 AM Amit Kapila  wrote:
> >
> > On Wed, Jan 30, 2019 at 8:11 PM John Naylor  
> > wrote:
> > >
> > > That's probably a good idea to limit risk. I just very basic tests
> > > now, and vacuum before every relation size check to make sure any FSM
> > > extension (whether desired or not) is invoked. Also, in my last patch
> > > I forgot to implement explicit checks of the block number instead of
> > > assuming how many rows will fit on a page. I've used a plpgsql code
> > > block to do this.
> > >
> >
> > -- Extend table with enough blocks to exceed the FSM threshold
> >  -- FSM is created and extended to 3 blocks
> >
> > The second comment line seems redundant to me, so I have removed that
> > and integrated it in the main patch.
>
> FYI, the second comment is still present in v20.
>

oops, forgot to include in commit after making a change, done now.


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


v21-0001-Avoid-creation-of-the-free-space-map-for-small-heap-.patch
Description: Binary data

Re: BUG #15589: Due to missing wal, restore ends prematurely and opens database for read/write

[Kyotaro HORIGUCHI]

At Wed, 30 Jan 2019 15:53:51 +, l...@lako.no wrote in 

> Hi
> I have reported a bug via PostgreSQL bug report form, but havent got any 
> response so far.
> This might not be a bug, but a feature not implemented yet.
> I suggest to make a small addition to StartupXLOG to solve the issue.

I can understand what you want, but it doesn't seem acceptable
since it introduces inconsistency among target kinds.

> The scenario I want to solve is:
> Need to restore backup to another server.
>  Restores pgbasebackup files
>  Restores som wal-files
>  Extract pgbasebackup files
>  creates recover.conf with pit
>  Starts postgresql
>  recover ends before pit due to missing wal-files
>  database opens read/write
> 
> I think database should have paused recovery then I could restore 
> additional wal-files and restart postgresql to continue with recover.

I don't think no one expected that server follows
recovery_target_action without setting a target, so we can change
the behavior when any kind of target is specified. So I propose
to follow recovery_target_action even if not rached the target
when any recovery target isspecified.

With the attached PoC (for master), recovery stops as follows:

LOG:  consistent recovery state reached at 0/2F00
LOG:  database system is ready to accept read only connections
rc_work/0001002F’: No such file or directory
WARNING:  not reached specfied recovery target, take specified action anyway
DETAIL:  This means a wrong target or missing of expected WAL files.
LOG:  recovery has paused
HINT:  Execute pg_wal_replay_resume() to continue.

If no target is specifed, it promtes immediately ignoring r_t_action.

If this is acceptable I'll post complete version (including
documentation). I don't think this back-patcheable.

> With large databases and a lot of wal-files it is time consuming to repeat 
> parts of the process.

I understand your concern.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 2ab7d804f0..081bdd86ec 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7246,12 +7246,25 @@ StartupXLOG(void)
 			 * end of main redo apply loop
 			 */
 
-			if (reachedStopPoint)
+			/*
+			 * If recovery target is specified, specified action is expected
+			 * to be taken regardless whether the target is reached or not .
+			 */
+			if (recoveryTarget != RECOVERY_TARGET_UNSET)
 			{
+/*
+ * At this point we don't consider the case where we are
+ * before consistent point even if not reached stop point.
+ */
 if (!reachedConsistency)
 	ereport(FATAL,
 			(errmsg("requested recovery stop point is before consistent recovery point")));
 
+if (!reachedStopPoint)
+	ereport(WARNING,
+			(errmsg ("not yet reached specfied recovery target, take specified action anyway"),
+			 errdetail("This means a wrong target or missing WAL files.")));
+
 /*
  * This is the last point where we can restart recovery with a
  * new recovery target, if we shutdown and begin again. After

Re: [HACKERS] proposal: schema variables

[Pavel Stehule]

Hi

just rebase

regards

Pavel


schema-variables-20190131.patch.gz
Description: application/gzip

Re: WIP Patch: Precalculate stable functions, infrastructure v1

[Andres Freund]

On 2018-11-29 18:00:15 +0100, Dmitry Dolgov wrote:
> > On Tue, Oct 2, 2018 at 4:22 AM Michael Paquier  wrote:
> >
> > On Thu, May 24, 2018 at 04:00:33PM +0300, Marina Polyakova wrote:
> > > Here there's a 9-th version of the patches for the precalculation of 
> > > stable
> > > or immutable functions, stable or immutable operators and other 
> > > nonvolatile
> > > expressions. This is a try to execute cached expressions as PARAM_EXEC,
> > > thanks to the comments of Tom Lane and Andres Freund [1].
> >
> > Please note that v9-0004 fails to apply, so a rebase is needed.  This
> > patch is moved to next CF, waiting on author.
> 
> Unfortunately, patch still has some conflicts, could you please post an 
> updated
> version?

As nothing has happened since, I'm marking this as returned with
feedback.

Greetings,

Andres Freund

Re: Online enabling of checksums

[Magnus Hagander]

On Thu, Jan 31, 2019 at 11:57 AM Andres Freund  wrote:

> On 2018-09-30 10:48:36 +0200, Tomas Vondra wrote:
> >
> >
> > On 09/29/2018 06:51 PM, Stephen Frost wrote:
> > > Greetings,
> > >
> > > * Tomas Vondra (tomas.von...@2ndquadrant.com) wrote:
> > >> On 09/29/2018 02:19 PM, Stephen Frost wrote:
> > >>> * Tomas Vondra (tomas.von...@2ndquadrant.com) wrote:
> >  While looking at the online checksum verification patch (which I
> guess
> >  will get committed before this one), it occurred to me that
> disabling
> >  checksums may need to be more elaborate, to protect against someone
> >  using the stale flag value (instead of simply switching to "off"
> >  assuming that's fine).
> > 
> >  The signals etc. seem good enough for our internal stuff, but what
> if
> >  someone uses the flag in a different way? E.g. the online checksum
> >  verification runs as an independent process (i.e. not a backend) and
> >  reads the control file to find out if the checksums are enabled or
> not.
> >  So if we just switch from "on" to "off" that will break.
> > 
> >  Of course, we may also say "Don't disable checksums while online
> >  verification is running!" but that's not ideal.
> > >>>
> > >>> I'm not really sure what else we could say here..?  I don't
> particularly
> > >>> see an issue with telling people that if they disable checksums while
> > >>> they're running a tool that's checking the checksums that they're
> going
> > >>> to get odd results.
> > >>
> > >> I don't know, to be honest. I was merely looking at the online
> > >> verification patch and realized that if someone disables checksums it
> > >> won't notice it (because it only reads the flag once, at the very
> > >> beginning) and will likely produce bogus errors.
> > >>
> > >> Although, maybe it won't - it now uses a checkpoint LSN, so that might
> > >> fix it. The checkpoint LSN is read from the same controlfile as the
> > >> flag, so we know the checksums were enabled during that checkpoint.
> Soi
> > >> if we ignore failures with a newer LSN, that should do the trick, no?
> > >>
> > >> So perhaps that's the right "protocol" to handle this?
> > >
> > > I certainly don't think we need to do anything more.
> > >
> >
> > Not sure I agree. I'm not suggesting we absolutely have to write huge
> > amount of code to deal with this issue, but I hope we agree we need to
> > at least understand the issue so that we can put warnings into docs.
> >
> > FWIW pg_basebackup (in the default "verify checksums") has this issue
> > too AFAICS, and it seems rather unfriendly to just start reporting
> > checksum errors during backup in that case.
> >
> > But as I mentioned, maybe there's no problem at all and using the
> > checkpoint LSN deals with it automatically.
>
> Given that this patch has not been developed in a few months, I don't
> see why this has an active 2019-01 CF entry? I think we should mark this
> as Returned With Feedback.
>
> https://commitfest.postgresql.org/21/1535/
>
>
Unfortunately, I agree. I wish that wasn't the case, but due to things
outside my control, that's what happened.

So yes, let's punt it.

//Magnus

Re: POC: GROUP BY optimization

[Andres Freund]

On 2018-11-29 17:56:53 +0100, Dmitry Dolgov wrote:
> > On Tue, Oct 2, 2018 at 4:16 AM Michael Paquier  wrote:
> >
> > On Sat, Jun 30, 2018 at 09:19:13AM +1200, Gavin Flower wrote:
> > > Additionally put an upper limit threshold on the number of combinations to
> > > check, fairly large by default?
> > >
> > > If first threshold is exceeded, could consider checking out a few more
> > > selected at random from paths not yet checked, to avoid any bias caused by
> > > stopping a systematic search.  This might prove important when N! is 
> > > fairly
> > > large.
> >
> > Please note that the latest patch available does not apply, so this has
> > been moved to next CF 2018-11, waiting for input from its author.
> 
> Unfortunately, patch still has some conflicts, could you please rebase it?

As nothing has happened since, I'm marking this as returned with
feedback.

Greetings,

Andres Freund

Re: cost_sort() improvements

[Andres Freund]

On 2018-11-29 17:53:31 +0100, Dmitry Dolgov wrote:
> > On Tue, Oct 2, 2018 at 3:58 AM Michael Paquier  wrote:
> >
> > On Sun, Jul 22, 2018 at 10:22:10PM +0200, Tomas Vondra wrote:
> > > Could you perhaps summarize the reasoning or at least point me to the
> > > relevant parts of the sources, so that I know which parts to focus on?
> >
> > Teodor, this patch is waiting for some input from you.  I have moved it
> > to next CF for now, 2018-11.
> 
> The patch also needs to be adjusted - it fails on "make check" on join,
> aggregates and partition_aggregate.

As nothing has happened since, I'm marking this as returned with
feedback.

Greetings,

Andres Freund

Re: Sample values for pg_stat_statements

[Andres Freund]

On 2018-11-29 16:58:05 +0100, Dmitry Dolgov wrote:
> > On Mon, Oct 1, 2018 at 8:30 AM Michael Paquier  wrote:
> >
> > On Tue, Apr 17, 2018 at 05:58:44PM +0200, Vik Fearing wrote:
> > > I think I handle that well enough with permission checking, but I'm open
> > > to more debate on it.
> >
> > The recent version bump in pg_stat_statements (Sorry my fault!) is
> > causing this patch to not apply anymore.  I have moved it to next CF
> > with waiting on author as status.
> 
> Unfortunately, the patch is still needs to be rebased.

As nothing has happened since, I'm marking this as returned with
feedback.

Greetings,

Andres Freund

Re: Implementing SQL ASSERTION

[Andres Freund]

Hi,

On 2018-11-29 16:54:14 +0100, Dmitry Dolgov wrote:
> > On Tue, Sep 25, 2018 at 1:04 AM Joe Wildish 
> > wrote:
> >
> > All agreed.  I’ll give the patch some TLC and get a new version that
> > addresses the above.
> 
> Hi,
> 
> Just a reminder, that the patch still needs to be rebased, could you please do
> this? I'm moving the item to the next CF.

As nothing has happened, I'm marking this patch as returned with feedback.

Greetings,

Andres Freund

Re: [WIP] [B-Tree] Retail IndexTuple deletion

[Andres Freund]

On 2018-11-29 14:27:50 +0100, Dmitry Dolgov wrote:
> > On Fri, Sep 21, 2018 at 5:52 AM Andrey Lepikhov  
> > wrote:
> >
> > The v6 version of quick vacuum, which utilizes the amtargetdelete()
> > interface for retail indextuple deletion.
> > Now it is more simple and laconic.
> > It must be applied after Retail-IndexTuple-Deletion-Access-Method.patch.
> >
> > BENCHMARKS:
> > ---
> >
> > Initial script:
> > pgbench -i -s $scale # initial DB generation
> > "CREATE INDEX pgbench_accounts_ext ON public.pgbench_accounts USING
> > btree (abalance);" # additional index
> >
> > Comparison with lazy vacuum:
> >
> > script:
> > "DELETE FROM pgbench_accounts WHERE (random() < $factor);" # delete a
> > part of tuples for cleaning strategies comparison
> > "VACUUM pgbench_accounts;" # check time of vacuum process by bash 'date
> > +%s%N | cut -b1-13' command
> >
> > Results:
> > | $scale=10   | $scale=100  |
> > $factor| QVAC | LVAC | QVAC | LVAC |
> > 1E-6   | -| -| 284  | 979  |
> > 1E-5   | 78   | 144  | 288  | 1423 |
> > 1E-4   | 72   | 280  | 388  | 3304 |
> > 1E-3   | 189  | 609  | 2294 | 6029 |
> > 1E-2   | 443  | 783  | 54232| 67884|
> > 1E-1   | 1593 | 1237 | 83092| 86104|
> >
> > where QVAC - forced use of quick vacuum; LVAC - use lazy vacuum for
> > index cleanup. $factor corresponds a number of vacuumed tuples. For
> > example, $scale=10, $factor=1E-1 -> 10 tuples vacuumed. Time
> > measured in ms.
> >
> > So, quick strategy can be used in a vacuum process effectively up to
> > 1-2% of DEAD tuples in a relation.
> 
> Hi,
> 
> Unfortunately, this patch doesn't compile anymore:
> 
> index.c: In function ‘htid2IndexDatum’:
> index.c:4172:2: error: too few arguments to function 
> ‘MakeSingleTupleTableSlot’
>   TupleTableSlot *slot = MakeSingleTupleTableSlot(RelationGetDescr(hrel));
>   ^
> 
> Also I'm a bit confused about the current status of collaboration between this
> patch and [1], one is tightly depends on another or not? Does it makes sense
> to have only one corresponding CF item then? For now I'll move this one to
> the next CF.
> 
> [1]: 
> https://www.postgresql.org/message-id/flat/CAH2-WzkVb0Kom%3DR%2B88fDFb%3DJSxZMFvbHVC6Mn9LJ2n%3DX%3DkS-Uw%40mail.gmail.com


Given this patch has not been updated since Dmitry's message above, I'm
marking this as returned with feedback.

Greetings,

Andres Freund

Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept

[Andres Freund]

Hi,

On 2018-11-29 13:45:15 +0100, Dmitry Dolgov wrote:
> > On Fri, Aug 24, 2018 at 5:53 PM Alexander Korotkov 
> >  wrote:
> >
> > Given I've no feedback on this idea yet, I'll try to implement a PoC
> > patch for that.  It doesn't look to be difficult.  And we'll see how
> > does it work.
> 
> Unfortunately, current version of the patch doesn't pass the tests and fails 
> on
> initdb. But maybe you already have this PoC you were talking about, that will
> also incorporate the feedback from this thread? For now I'll move it to the
> next CF.

Given that nothing has happened since, I'm going to close this CF entry
with returned as feedback.

Greetings,

Andres Freund

Re: Connection slots reserved for replication

[Oleksii Kliukin]

> On 31. Jan 2019, at 11:50, Petr Jelinek  wrote:
> 
> On 31/01/2019 11:25, Oleksii Kliukin wrote:
>> 
>> 
>>> On 25. Jan 2019, at 18:37, Oleksii Kliukin >> > wrote:
>>> 
>>> Hello,
>>> 
 On 24. Jan 2019, at 13:47, Kyotaro HORIGUCHI
 >>> > wrote:
>>> 
>>> Thank you for the review.I took a liberty to address it with v9.
>> 
>> 
>> So, given there are no further feedback or suggestions, can we set it to
>> RFC?
> 
> I vote yes.

Thanks, set it to RFC.

Cheers,
Oleksii

bug tracking system

[Alvaro Herrera]

Here's a bug tracking system that Nathan set up many years ago and
apparently has kept going unattended.  It seems to me that it's
something that we could base a semi-official bug tracking system on.

https://granicus.if.org/pgbugs/

There are clearly things that need fixed, such as failure to
base64-decode the body of messages, etc ... and we probably want to
piggyback on our archives rather than having its own copy of the emails.
But it's a starting point for discussion.

-- 
Álvaro Herrera

Re: Online enabling of checksums

[Andres Freund]

On 2018-09-30 10:48:36 +0200, Tomas Vondra wrote:
> 
> 
> On 09/29/2018 06:51 PM, Stephen Frost wrote:
> > Greetings,
> > 
> > * Tomas Vondra (tomas.von...@2ndquadrant.com) wrote:
> >> On 09/29/2018 02:19 PM, Stephen Frost wrote:
> >>> * Tomas Vondra (tomas.von...@2ndquadrant.com) wrote:
>  While looking at the online checksum verification patch (which I guess
>  will get committed before this one), it occurred to me that disabling
>  checksums may need to be more elaborate, to protect against someone
>  using the stale flag value (instead of simply switching to "off"
>  assuming that's fine).
> 
>  The signals etc. seem good enough for our internal stuff, but what if
>  someone uses the flag in a different way? E.g. the online checksum
>  verification runs as an independent process (i.e. not a backend) and
>  reads the control file to find out if the checksums are enabled or not.
>  So if we just switch from "on" to "off" that will break.
> 
>  Of course, we may also say "Don't disable checksums while online
>  verification is running!" but that's not ideal.
> >>>
> >>> I'm not really sure what else we could say here..?  I don't particularly
> >>> see an issue with telling people that if they disable checksums while
> >>> they're running a tool that's checking the checksums that they're going
> >>> to get odd results.
> >>
> >> I don't know, to be honest. I was merely looking at the online
> >> verification patch and realized that if someone disables checksums it
> >> won't notice it (because it only reads the flag once, at the very
> >> beginning) and will likely produce bogus errors.
> >>
> >> Although, maybe it won't - it now uses a checkpoint LSN, so that might
> >> fix it. The checkpoint LSN is read from the same controlfile as the
> >> flag, so we know the checksums were enabled during that checkpoint. Soi
> >> if we ignore failures with a newer LSN, that should do the trick, no?
> >>
> >> So perhaps that's the right "protocol" to handle this?
> > 
> > I certainly don't think we need to do anything more.
> > 
> 
> Not sure I agree. I'm not suggesting we absolutely have to write huge
> amount of code to deal with this issue, but I hope we agree we need to
> at least understand the issue so that we can put warnings into docs.
> 
> FWIW pg_basebackup (in the default "verify checksums") has this issue
> too AFAICS, and it seems rather unfriendly to just start reporting
> checksum errors during backup in that case.
> 
> But as I mentioned, maybe there's no problem at all and using the
> checkpoint LSN deals with it automatically.

Given that this patch has not been developed in a few months, I don't
see why this has an active 2019-01 CF entry? I think we should mark this
as Returned With Feedback.

https://commitfest.postgresql.org/21/1535/

Greetings,

Andres Freund

Re: Connection slots reserved for replication

[Petr Jelinek]

On 31/01/2019 11:25, Oleksii Kliukin wrote:
> 
> 
>> On 25. Jan 2019, at 18:37, Oleksii Kliukin > > wrote:
>>
>> Hello,
>>
>>> On 24. Jan 2019, at 13:47, Kyotaro HORIGUCHI
>>> >> > wrote:
>>
>> Thank you for the review.I took a liberty to address it with v9.
> 
> 
> So, given there are no further feedback or suggestions, can we set it to
> RFC?

I vote yes.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

Re: Connection slots reserved for replication

[Oleksii Kliukin]

> On 25. Jan 2019, at 18:37, Oleksii Kliukin  wrote:
> 
> Hello,
> 
>> On 24. Jan 2019, at 13:47, Kyotaro HORIGUCHI 
>> mailto:horiguchi.kyot...@lab.ntt.co.jp>> 
>> wrote:
> 
> Thank you for the review.I took a liberty to address it with v9.


So, given there are no further feedback or suggestions, can we set it to RFC?

Cheers,
Oleksii

Re: [HACKERS] Transactions involving multiple postgres foreign servers, take 2

[Masahiko Sawada]

On Tue, Jan 29, 2019 at 5:47 PM Ildar Musin  wrote:
>
> Hello,
>
> The patch needs rebase as it doesn't apply to the current master. I applied it
> to the older commit to test it. It worked fine so far.

Thank you for testing the patch!

>
> I found one bug though which would cause resolver to finish by timeout even
> though there are unresolved foreign transactions in the list. The
> `fdw_xact_exists()` function expects database id as the first argument and xid
> as the second. But everywhere it is called arguments specified in the 
> different
> order (xid first, then dbid).  Also function declaration in header doesn't
> match its definition.

Will fix.

>
> There are some other things I found.
> * In `FdwXactResolveAllDanglingTransactions()` variable `n_resolved` is
>   declared as bool but used as integer.
> * In fdwxact.c's module comment there are 
> `FdwXactRegisterForeignTransaction()`
>   and `FdwXactMarkForeignTransactionModified()` functions mentioned that are
>   not there anymore.
> * In documentation (storage.sgml) there is no mention of `pg_fdw_xact`
>   directory.
>
> Couple of stylistic notes.
> * In `FdwXactCtlData struct` there are both camel case and snake case naming
>   used.
> * In `get_fdw_xacts()` `xid != InvalidTransactionId` can be replaced with
>   `TransactionIdIsValid(xid)`.
> * In `generate_fdw_xact_identifier()` the `fx` prefix could be a part of 
> format
>   string instead of being processed by `sprintf` as an extra argument.
>

I'll incorporate them at the next patch set.

> I'll continue looking into the patch. Thanks!

Thanks. Actually I'm updating the patch set, changing API interface as
I proposed before and improving the document and README. I'll submit
the latest patch next week.


--
Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Pluggable Storage - Andres's take

[Amit Khandekar]

Hi,

Attached is a patch that adds some test scenarios for testing the
dependency of various object types on table am. Besides simple tables,
it considers materialized views, partitioned table, foreign table, and
composite types, and verifies that the dependency is created only for
those object types that support table access method.

This patch is based on commit 1bc7e6a4838 in
https://github.com/anarazel/postgres-pluggable-storage

Thanks
-Amit Khandekar


test_tableam_dependency.patch
Description: Binary data

RE: libpq debug log

[Iwata, Aya]

Hi Nagaura-san,

Thank you for your review and advice.

> This merit was very helpful for my use, so I want your proposal function in
> postgres.
Thank you.　

> 1)
> It would be better making the log format the same as the server log format,
> I think.
> Your log format:
>   2019/01/22 04:15:25.496 ...
> Server log format:
>   2019-01-22 04:15:25.496 UTC ...
> There are two differences:
>   One is separator character of date, "/" and "-".
>   The another is standard time information.
Sure. I will change separator character to "-" and add timezone information.

> 2)
> It was difficult for me to understand the first line message in the log file.
> "Max log size is 10B, log min level is LEVEL1"
> Does this mean as follows?
> "The maximum size of this file is 10 Bytes, the parameter 'log min level'
> is set to LEVEL 1."
Yes. The purpose of the line message is to check the value of the set parameter.
I will change it to you suggest.

> 3)
> Under the circumstance that the environment variables "PGLOGDIR" and
> "PGLOGSIZE" are set correctly, the log file will also be created when the
> user connect the server with "psql".
> Does this follow the specification you have thought?
> Is there any option to unset only in that session when you want to connect
> with "psql"?
There are no option to not output log when connected by "psql". 
It is not good to create lots of empty files. I think that the cause of this 
issue is that the initialization location of the new trace log is not good.
I will fix it so that logs are not output when connected to "psql".

> 4)
> Your patch affects the behavior of PQtrace().
> The log of the existing PQtrace() is as follows:
> From backend> "id"
> From backend (#4)> 16387
> From backend (#2)> 1
> From backend (#4)> 23
> ...
> Your patch makes PQtrace() including the following log in addition to the
> above.
> To backend> Msg complete, length 27
> Start sending message to backend:End sending message to backend:PQsendQuery
> end :PQgetResult start :Start receiving message from backend:End receiving
> message from backend:From backend> T ...
Thank you for finding it. I will fix not to affect PQtrace().

Regards,
Aya Iwata