Re: [HACKERS] Variable substitution in psql backtick expansion

2017-11-12 Thread Michael Paquier
On Mon, Nov 13, 2017 at 5:21 AM, Tom Lane  wrote:
> Pavel Stehule  writes:
>> [ psql-server-version-2.patch ]
>
> I think this patch should be rejected.  It adds no new functionality;
> you can get the string in question with "select version()".  Moreover,
> you've been able to do that for lo these many years.  Any application
> that tried to depend on this new way of getting the string would fail
> when working with an older server or older psql.  That does not seem
> like a good property for a version check.  Also, because the string
> isn't especially machine-friendly, it's not very clear to me what the
> use-case is for an application to use it at all, rather than the other
> version formats we already provide.

+1 for rejection as version() returns PG_VERSION_STR already. It is
also already possible to define a VERSION variable psqlrc simply with
that:
\set VERSION 'version();'

+-- check consistency of SERVER_VERSION
+-- which is transmitted as GUC "server_version_raw"
+SELECT :'SERVER_VERSION' = VERSION()
+   AND :'SERVER_VERSION' = current_setting('server_version_raw')
+   AND :'SERVER_VERSION' = :'VERSION'
+   AS "SERVER_VERSION is consistent";
Not much enthusiastic with this test when thinking about cross-upgrades.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] PATCH: psql tab completion for SELECT

2017-11-12 Thread Michael Paquier
On Mon, Nov 13, 2017 at 5:13 AM, David Fetter  wrote:
> Please add this to the upcoming (2018-01) commitfest at
> https://commitfest.postgresql.org/

You may want to scan the following thread as well:
https://www.postgresql.org/message-id/1328820579.11241.4.camel%40vanquo.pezone.net
And also you should be careful about things like WITH clauses...
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [bug fix] postgres.exe crashes with access violation on Windows while starting up

2017-11-12 Thread Michael Paquier
On Mon, Nov 13, 2017 at 7:37 AM, Noah Misch <n...@leadboat.com> wrote:
> On Fri, Nov 03, 2017 at 11:10:14AM +, Michael Paquier wrote:
>> I am
>> switching the patch as ready for committer, I definitely agree that
>> you are taking the write approach here.

s/write/right/.

> Committed both patches.

Thanks for double-checking, Noah.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [PATCH] A hook for session start

2017-11-11 Thread Michael Paquier
On Sat, Nov 11, 2017 at 12:49 AM, Fabrízio de Royes Mello
 wrote:
> New version attached.

Thanks.

+++ b/src/test/modules/Makefile
  test_extensions \
+ test_session_hooks \
  test_parser
Better if that's in alphabetical order.

That's a nit though, so I am switching the patch as ready for committer.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [Patch] Log SSL certificate verification errors

2017-11-10 Thread Michael Paquier
On Sat, Nov 11, 2017 at 3:34 AM, Graham Leggett  wrote:
> Currently neither the server side nor the client side SSL certificate verify 
> callback does anything, leading to potential hair-tearing-out moments.
>
> The following patch to master implements logging of all certificate 
> verification failures, as well as (crucially) which certificates failed to 
> verify, and at what depth, so the admin can zoom in straight onto the problem 
> without any guessing.

Could you attach as a file to this thread a patch that can be easily
applied? Using git --format-patch or simply diff is just fine.

Here are also some community guidelines on the matter:
https://wiki.postgresql.org/wiki/Submitting_a_Patch

And if you are looking for feedback, you should register it to the
next commit fest:
https://commitfest.postgresql.org/16/
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] pg_upgrade to clusters with a different WAL segment size

2017-11-10 Thread Michael Paquier
On Sat, Nov 11, 2017 at 12:46 AM, Bossart, Nathan  wrote:
> Allowing changes to the WAL segment size during pg_upgrade seems like a
> nice way to avoid needing a dump and load, so I would like to propose
> adding support for this.  I'd be happy to submit patches for this in the
> next commitfest.

That's a worthy goal.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks

2017-11-10 Thread Michael Paquier
On Sat, Nov 11, 2017 at 12:50 AM, Robert Haas  wrote:
> On Fri, Nov 10, 2017 at 10:34 AM, Tom Lane  wrote:
>> I think as far as that goes, we can just change to "Therefore, by default
>> their use is restricted ...".  Then I suggest adding a  para
>> after that, with wording along the lines of
>>
>> It is possible to GRANT use of server-side lo_import and lo_export to
>> non-superusers, but careful consideration of the security implications
>> is required.  A malicious user of such privileges could easily parlay
>> them into becoming superuser (for example by rewriting server
>> configuration files), or could attack the rest of the server's file
>> system without bothering to obtain database superuser privileges as
>> such.  Access to roles having such privilege must therefore be guarded
>> just as carefully as access to superuser roles.  Nonetheless, if use
>> of server-side lo_import or lo_export is needed for some routine task,
>> it's safer to use a role of this sort than full superuser privilege,
>> as that helps to reduce the risk of damage from accidental errors.
>
> +1.  That seems like great language to me.

+1. Not convinced that mentioning wrappers is worth the complication.
Experienced admins likely already know such matters.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks

2017-11-09 Thread Michael Paquier
On Fri, Nov 10, 2017 at 10:00 AM, Tom Lane  wrote:
> Stephen Frost  writes:
>> I'm guessing no, which essentially means that *we* consider access to
>> lo_import/lo_export to be equivilant to superuser and therefore we're
>> not going to implement anything to try and prevent the user who has
>> access to those functions from becoming superuser.  If we aren't willing
>> to do that, then how can we really say that there's some difference
>> between access to these functions and being a superuser?
>
> We seem to be talking past each other.  Yes, if a user has malicious
> intentions, it's possibly to parlay lo_export into obtaining a superuser
> login (I'm less sure that that's necessarily true for lo_import).
> That does NOT make it "equivalent", except perhaps in the view of someone
> who is only considering blocking malevolent actors.  It does not mean that
> there's no value in preventing a task that needs to run lo_export from
> being able to accidentally destroy any data in the database.  There's a
> range of situations where you are concerned about accidents and errors,
> not malicious intent; but your argument ignores those use-cases.

That will not sound much as a surprise as I spawned the original
thread, but like Robert I understand that getting rid of all superuser
checks is a goal that we are trying to reach to allow admins to have
more flexibility in handling permissions to a subset of objects.
Forcing an admin to give full superuser rights to one user willing to
work only on LOs import and export is a wrong concept.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Fix bloom WAL tap test

2017-11-09 Thread Michael Paquier
On Thu, Nov 9, 2017 at 7:51 PM, Alexander Korotkov
 wrote:
> On Wed, Nov 8, 2017 at 5:46 AM, Masahiko Sawada 
> wrote:
>> > So I think
>> > that you should instead do something like that:
>> >
>> > --- a/contrib/bloom/Makefile
>> > +++ b/contrib/bloom/Makefile
>> > @@ -20,5 +20,12 @@ include $(top_builddir)/src/Makefile.global
>> >  include $(top_srcdir)/contrib/contrib-global.mk
>> >  endif
>> >
>> > +installcheck: wal-installcheck
>> > +
>> > +check: wal-check
>> > +
>> > +wal-installcheck:
>> > +   $(prove_installcheck)
>> > +
>> >  wal-check: temp-install
>> > $(prove_check)
>> >
>> > This works for me as I would expect it should.
>>
>> Looks good to me too.
>
> OK, then so be it :)

Thanks for the new version. This one, as well as the switch to
psql_safe in 
https://www.postgresql.org/message-id/CAPpHfduxgEYF_0BTs-mxGC4=w5sw8rnUbq9BSTp1Wq7=nwr...@mail.gmail.com
are good for a committer lookup IMO.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] libpq connection strings: control over the cipher suites?

2017-11-09 Thread Michael Paquier
On Fri, Nov 10, 2017 at 2:53 AM, Joe Conway  wrote:
> On 11/09/2017 03:27 AM, Graham Leggett wrote:
>> Is there a parameter or mechanism for setting the required ssl cipher list 
>> from the client side?
>
> I don't believe so. That is controlled by ssl_ciphers, which requires a
> restart in order to change.
>
> https://www.postgresql.org/docs/10/static/runtime-config-connection.html#GUC-SSL-CIPHERS

Since commit de41869 present in v10, SSL parameters can be reloaded.
On libpq there is only an API to have a look at what are the ciphers
set by the server via PQsslAttribute().
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks

2017-11-09 Thread Michael Paquier
On Fri, Nov 10, 2017 at 7:05 AM, Tom Lane  wrote:
> I did miss the need to fix the docs, and am happy to put in some strong
> wording about the security hazards of these functions while fixing the
> docs.  But I do not think that leaving them with hardwired superuser
> checks is an improvement over being able to control them with GRANT.

Sorry about that. lobj.sgml indeed mentions superusers. Do you need a patch?
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [PATCH] A hook for session start

2017-11-09 Thread Michael Paquier
On Fri, Nov 10, 2017 at 2:32 AM, Fabrízio de Royes Mello
<fabriziome...@gmail.com> wrote:
> On Thu, Nov 9, 2017 at 12:09 AM, Michael Paquier <michael.paqu...@gmail.com>
> wrote:
>> +++ b/src/test/modules/test_session_hooks/session_hooks.conf
>> @@ -0,0 +1 @@
>> +shared_preload_libraries = 'test_session_hooks'
>> Don't you think that this should be a GUC? My previous comment
>> outlined that. I won't fight hard on that point in any case, don't
>> worry. I just want to make things clear :)
>
> Ooops... my fault... fixed!
>
> Thanks again!!

+/* GUC variables */
+static char *session_hook_username = NULL;
This causes the module to crash if test_session_hooks.username is not
set. I would recommend just assigning a default value, say "postgres".
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] (spelling) Ensure header of postgresql.auto.conf is consistent

2017-11-09 Thread Michael Paquier
On Thu, Nov 9, 2017 at 6:25 PM, Fabrízio de Royes Mello
 wrote:
> Interesting... IMHO this typo should be backpatched to 9.4 when ALTER SYSTEM
> was introduced.

Yeah, that's harmless.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-11-08 Thread Michael Paquier
On Thu, Nov 9, 2017 at 8:25 AM, Asim Praveen  wrote:
> Indeed, the assertion tripped during WAL replay on the standby.  This was
> caught by TAP tests under src/test/recovery.  The assertion is now fixed so
> that WAL replay is exempt from the check.  Please find the new patch
> attached.  The tests now pass with the fix.  I also manually verified that
> recovery works with "wal_consistency_checking=all".

I still have a bad feeling about that bit... Still, it does not change
the fact that patch 0001 in
https://www.postgresql.org/message-id/CANXE4TccH_VjdKaHc9=KyH0Y7WORqZN+=mH5f=mp0bw3gzx...@mail.gmail.com
needs a committer per the fact that it visibly fixes incorrect backend
code and API contract. So I am switching the CF entry to ready for
committer, but only for 0001.

The other things could always be taken care of later.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [PATCH] A hook for session start

2017-11-08 Thread Michael Paquier
On Thu, Nov 9, 2017 at 2:42 AM, Fabrízio de Royes Mello
<fabriziome...@gmail.com> wrote:
> On Wed, Nov 8, 2017 at 12:47 AM, Michael Paquier <michael.paqu...@gmail.com>
> wrote:
>> - Let's restrict the logging to a role name instead of a database
>> name, and let's parametrize it with a setting in the temporary
>> configuration file. Let's not bother about multiple role support with
>> a list, for the sake of tests and simplicity only defining one role
>> looks fine to me. Comments in the code should be clear about the
>> dependency.
>
> Makes sense and simplify the test code. Fixed.

+   if (!strcmp(username, "regress_sess_hook_usr2"))
+   {
+   const char *dbname;
[...]
+++ b/src/test/modules/test_session_hooks/session_hooks.conf
@@ -0,0 +1 @@
+shared_preload_libraries = 'test_session_hooks'
Don't you think that this should be a GUC? My previous comment
outlined that. I won't fight hard on that point in any case, don't
worry. I just want to make things clear :)
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks

2017-11-08 Thread Michael Paquier
On Thu, Nov 9, 2017 at 6:05 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paqu...@gmail.com> writes:
>> On Tue, Sep 26, 2017 at 11:42 AM, Vaishnavi Prabakaran
>> <vaishnaviprabaka...@gmail.com> wrote:
>>> I moved the cf entry to "ready for committer", and though my vote is for
>>> keeping the existing API behavior with write implying read, I let the
>>> committer decide whether the following behavior change is Ok or not.
>>> "Reading from a large-object descriptor opened with INV_WRITE is NOT
>>> possible"
>
>> Thanks for the review!
>
> After chewing on this some more, I'm inclined to agree that we should
> not change the behavior of the read/write flags.  There's been no
> field requests for a true-write-only mode, so I think we're much more
> likely to get complaints from users whose code we broke than plaudits
> from people who think the change is helpful.

Thanks for the input. Then that's two people favoring no changes.

> I believe it would be easy enough to adjust the patch so that we can
> still have the refactoring benefits; we'd just need the bit that
> translates from external to internal flags to go more like
>
> if (flags & INV_WRITE)
> descflags |= IFS_WRLOCK | IFS_RDLOCK;
> if (flags & INV_READ)
> descflags |= IFS_RDLOCK;
>
> (Preferably with a comment about why it's like this.)

Sure, I have updated the patch with this comment:
+   /*
+* Historically, no difference is made between (INV_WRITE) and
+* (INV_WRITE | INV_READ), the caller being allowed to read the large
+* object descriptor in either case.
+*/
Of course please feel free to reword if you find something better-suited.

> Another idea would be to invent a new external flag bit "INV_WRITE_ONLY",
> so that people who wanted true write-only could get it, without breaking
> backwards-compatible behavior.  But I'm inclined to wait for some field
> demand to show up before adding even that little bit of complication.

Demand that may never show up, and the current behavior on HEAD does
not receive any complains either. I am keeping the patch simple for
now. That's less aspirin needed for everybody.
-- 
Michael


0003-Move-ACL-checks-for-large-objects-when-opening-them.patch
Description: Binary data


0001-Remove-ALLOW_DANGEROUS_LO_FUNCTIONS-for-LO-related-s.patch
Description: Binary data


0002-Replace-superuser-checks-of-large-object-import-expo.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] taking stdbool.h into use

2017-11-08 Thread Michael Paquier
On Thu, Nov 9, 2017 at 1:46 AM, Peter Eisentraut
<peter.eisentr...@2ndquadrant.com> wrote:
> On 10/29/17 08:50, Michael Paquier wrote:
>> I spotted a couple of other things while looking at your patches and
>> the code tree.
>>
>> -   return (ginCompareItemPointers(>itemptr, iptr) > 0) ? TRUE : 
>> FALSE;
>> +   return (ginCompareItemPointers(>itemptr, iptr) > 0) ? true : 
>> false;
>> You could simplify that at the same time by removing such things. The
>> "false : true" pattern is less frequent than the "true : false"
>> pattern.
>
> I have found many more instances like that.  It might be worth
> simplifying a bit, but that sounds like a separate undertaking.

Yeah, I just mentioned one for reference. And I can see 66 instances.
It may be not worth changing either to simplify back-patching.

>> Some comments in the code still mention FALSE or TRUE:
>> - hashsearch.c uses FALSE in some comments.
>> - In execExpr.c, ExecCheck mentions TRUE.
>
> That one is an SQL TRUE, so I left it.

Oops. You are right. I tried to be careful with what was referring to SQL and C.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Exclude pg_internal.init from base backup

2017-11-08 Thread Michael Paquier
On Thu, Nov 9, 2017 at 1:03 AM, Peter Eisentraut
<peter.eisentr...@2ndquadrant.com> wrote:
> On 11/7/17 19:58, Michael Paquier wrote:
>> On Wed, Nov 8, 2017 at 9:50 AM, Haribabu Kommi <kommi.harib...@gmail.com> 
>> wrote:
>>> Thanks for the correction. I was not much aware of SGML markup usage.
>>> While building the documentation, it raises an warning message of "empty
>>> end-tag".
>>> So I just added the end tag. Attached the update patch with the suggested
>>> correction.
>>
>> Ah, I can see the warning as well. Using empty tags is forbidden since
>> c29c5789, which is really recent. Sorry for missing it. Simon got
>> trapped by that as well visibly. Your patch looks good to me.
>
> fixed

Thanks.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [PATCH] A hook for session start

2017-11-07 Thread Michael Paquier
On Tue, Nov 7, 2017 at 9:58 PM, Fabrízio de Royes Mello
<fabriziome...@gmail.com> wrote:
> On Tue, Nov 7, 2017 at 1:15 AM, Michael Paquier <michael.paqu...@gmail.com>
> wrote:
>> On Sun, Nov 5, 2017 at 3:14 AM, Fabrízio de Royes Mello
>> <fabriziome...@gmail.com> wrote:
>> I was going to to hack something like that. That's interesting for the
>> use case Robert has mentioned.
>>
>> Well, in the case of the end session hook, those variables are passed
>> to the hook by being directly taken from the context from MyProcPort:
>> +   (*session_end_hook) (MyProcPort->database_name,
>> MyProcPort->user_name);
>> In the case of the start hook, those are directly taken from the
>> command outer caller, but similarly MyProcPort is already set, so
>> those are strings available (your patch does so in the end session
>> hook)... Variables in hooks are useful if those are not available
>> within the memory context, and refer to a specific state where the
>> hook is called. For example, take the password hook, this uses the
>> user name and the password because those values are not available
>> within the session context. The same stands for other hooks as well.
>> Keeping the interface minimal helps in readability and maintenance.
>> See for the attached example that can be applied on top of 0003, which
>> makes use of the session context, the set regression tests does not
>> pass but this shows how I think those hooks had better be shaped.
>>
>
> Makes sense... fixed.

Thanks for considering it.

>> +   (*session_end_hook) (MyProcPort->database_name,
>> MyProcPort->user_name);
>> +
>> +   /* Make don't leave any active transactions and/or locks behind */
>> +   AbortOutOfAnyTransaction();
>> +   LockReleaseAll(USER_LOCKMETHOD, true);
>> Let's leave this work to people actually implementing the hook contents.
>>
>
> Fixed.
>
> Attached new version. I unify all three patches in just one because this is
> a small patch and it's most part is test code.

Thanks. This makes sense to me.

+   /* Hook just normal backends */
+   if (session_end_hook && MyBackendId != InvalidBackendId)
+   (*session_end_hook) ();
I have been wondering about the necessity of this restriction.
Couldn't it be useful to just let the people implementing the hook do
any decision-making? Tracking some non-backend shutdowns may be useful
as well for logging purposes.

The patch is beginning to take shape (I really like the test module
you are implementing here!), still needs a bit more work.

+CREATE ROLE session_hook_usr1 LOGIN;
+CREATE ROLE session_hook_usr2 LOGIN;
Roles created during regression tests should be prefixed with regress_.

+   if (prev_session_start_hook)
+   prev_session_start_hook();
+
+   if (session_start_hook_enabled)
+   (void) register_session_hook("START");
Shouldn't both be reversed?

+/* sample sessoin end hook function */
Typo here.

+CREATE DATABASE session_hook_db;
[...]
+   if (!strcmp(dbname, "session_hook_db"))
+   {
Creating a database is usually avoided in sql files as those can be
run on existing servers using installcheck. I am getting that this
restriction is in place so as it is possible to create an initial
connection to the server so as the base table to log the activity is
created. That's a fine assumption to me. Still, I think that the
following changes should be done:
- Let's restrict the logging to a role name instead of a database
name, and let's parametrize it with a setting in the temporary
configuration file. Let's not bother about multiple role support with
a list, for the sake of tests and simplicity only defining one role
looks fine to me. Comments in the code should be clear about the
dependency.
- The GUCs test_session_hooks.start_enabled and
test_session_hooks.end_enabled are actually redundant with the
database restriction (well, role restriction per previous comment), so
let's remove them. Please let's also avoid ALTER SYSTEM calls in tests
as it would impact existing installations with installcheck.

Impossible to tell committer's feeling about this test suite, but my
opinion is to keep it as that's a good template example about what can
be done with those two hooks.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [COMMITTERS] pgsql: Expand empty end tag

2017-11-07 Thread Michael Paquier
On Wed, Nov 8, 2017 at 11:15 AM, Peter Eisentraut  wrote:
> Expand empty end tag

Perhaps you missed this patch?
https://www.postgresql.org/message-id/CAJrrPGdkL8TFk+-VivrW637js0v_KM=ub4pBFy=nf0bpafb...@mail.gmail.com
It seems to me that the information within brackets should not be
inside the filename markup :)
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Fix bloom WAL tap test

2017-11-07 Thread Michael Paquier
On Wed, Nov 8, 2017 at 1:58 AM, Alexander Korotkov
 wrote:
> On Tue, Nov 7, 2017 at 4:34 PM, Masahiko Sawada 
> wrote:
>> I understood the necessity of this patch and reviewed two patches.
>
> Good, thank you.

That's clearly a bug fix.

>> diff --git a/contrib/bloom/Makefile b/contrib/bloom/Makefile
>> index 13bd397..c1566d4 100644
>> --- a/contrib/bloom/Makefile
>> +++ b/contrib/bloom/Makefile
>> @@ -20,5 +20,7 @@ include $(top_builddir)/src/Makefile.global
>>  include $(top_srcdir)/contrib/contrib-global.mk
>>  endif
>>
>> +check: wal-check
>> +
>>  wal-check: temp-install
>> $(prove_check)
>
>
> I've tried this version Makefile.  And I've seen the only difference: when
> tap tests are enabled, this version of Makefile runs tap tests before
> regression tests.  While my version of Makefile runs tap tests after
> regression tests.  That seems like more desirable behavior for me.  But it
> would be sill nice to simplify Makefile.

Why do you care about the order of actions? There is no dependency
between each test and it seems to me that it should remain so to keep
a maximum flexibility. This does not sacrifice coverage as well. In
short, Sawada-san's suggestion looks like a thing to me. One piece
that it is missing though is that installcheck triggers only the
SQL-based tests, and it should also trigger the TAP tests. So I think
that you should instead do something like that:

--- a/contrib/bloom/Makefile
+++ b/contrib/bloom/Makefile
@@ -20,5 +20,12 @@ include $(top_builddir)/src/Makefile.global
 include $(top_srcdir)/contrib/contrib-global.mk
 endif

+installcheck: wal-installcheck
+
+check: wal-check
+
+wal-installcheck:
+   $(prove_installcheck)
+
 wal-check: temp-install
$(prove_check)

This works for me as I would expect it should. Could you update the
patch? That's way more simple than having to replicate again rules
like regresscheck and regressioncheck-install. I am switching the
patch back to "waiting on author" for now.

I can see that src/test/modules/commit_ts is missing the shot as well,
and I think that's the case as well of other test modules like
pg_dump's suite..
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Fix bloom WAL tap test

2017-11-07 Thread Michael Paquier
On Wed, Nov 8, 2017 at 1:49 AM, Alexander Korotkov
 wrote:
> On Tue, Nov 7, 2017 at 4:26 PM, Fabrízio Mello 
> wrote:
>> The patch doesn't apply against master:
>>
>> fabrizio@macanudo:/d/postgresql (master)
>> $ git apply /tmp/wal-check-on-bloom-check.patch
>> error: contrib/bloom/Makefile: already exists in working directory
>
> Apparently I sent patches whose are ok for "patch -p1", but not ok for "git
> apply".
> Sorry for that.   I resubmit both of them.

I would not worry about that as long as there is a way to apply
patches. git apply fails to easily to be honest, while patch -p1 is
more permissive. I use the latter extensively by the way, because it
has the merit to not be a PITA.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Remove duplicate setting in test/recovery/Makefile

2017-11-07 Thread Michael Paquier
On Wed, Nov 8, 2017 at 10:38 AM, Masahiko Sawada  wrote:
> Hi,
>
> I found that EXTRA_INSTALL is doubly set at both top and bottom of the
> src/test/recovery/Makefile. Is it necessary?
>
> Attached patch fixes this.

Indeed, there is some bad overlap between d851bef and 1148e22a. Better
to CC Simon who committed both things.
--
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Exclude pg_internal.init from base backup

2017-11-07 Thread Michael Paquier
On Wed, Nov 8, 2017 at 9:50 AM, Haribabu Kommi  wrote:
> Thanks for the correction. I was not much aware of SGML markup usage.
> While building the documentation, it raises an warning message of "empty
> end-tag".
> So I just added the end tag. Attached the update patch with the suggested
> correction.

Ah, I can see the warning as well. Using empty tags is forbidden since
c29c5789, which is really recent. Sorry for missing it. Simon got
trapped by that as well visibly. Your patch looks good to me.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Exclude pg_internal.init from base backup

2017-11-07 Thread Michael Paquier
On Wed, Nov 8, 2017 at 9:04 AM, Haribabu Kommi  wrote:
> The commit 98267e missed to check the empty SGML tag, attached patch
> fixes the same.


 
- pg_internal.init (found in multiple directories)
+ pg_internal.init (found in multiple directories)
 

What has been committed in 98267ee and what is proposed here both look
incorrect to me. The markup filename ought to be used only with file
names, so "(found in multiple directories)" should not be within it.
Simon's commit is not wrong with the markup usage by the way.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Exclude pg_internal.init from base backup

2017-11-07 Thread Michael Paquier
On Wed, Nov 8, 2017 at 1:42 AM, David Steele  wrote:
> On 11/7/17 11:03 AM, Simon Riggs wrote:
>> On 5 November 2017 at 11:55, Magnus Hagander  wrote:
>>>
>>> So +1 for documenting the difference in how these are handled, as this is
>>> important to know for somebody writing an external tool for it.
>>
>> Changes made, moving to commit the attached patch.
>
> Thanks, Simon!  This was on my to do list today -- glad I checked my
> email first.


+pg_internal.init files can be omitted from the
+backup whenever a file of that name is found.  These files contain
+relation cache data that is always rebuilt when recovering.
+   
Do we want to mention in the docs that the same decision-making is
done for *all* files with matching names, aka the fact that if a file
is listed and found in a sub-folder it is skipped? postmaster.opts or
similar friends are unlikely to be found in anything but the root of
the data folder, still the upthread argument of documenting precisely
what basebackup.c does sounded rather convincing to me.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Remove secondary checkpoint

2017-11-07 Thread Michael Paquier
On Wed, Nov 8, 2017 at 2:23 AM, Simon Riggs <si...@2ndquadrant.com> wrote:
> On 31 October 2017 at 12:01, Michael Paquier <michael.paqu...@gmail.com> 
> wrote:
>> While the mention about a manual checkpoint happening after a timed
>> one will cause a full range of WAL segments to be recycled, it is not
>> actually true that segments of the prior's prior checkpoint are not
>> needed, because with your patch the segments of the prior checkpoint
>> are getting recycled. So it seems to me that based on that the formula
>> ought to use 1.0 instead of 2.0...
>
> I think the argument in the comment is right, in that
> CheckPointDistanceEstimate is better if we use multiple checkpoint
> cycles.

Yes, the theory behind is correct. No argument behind that.

> But the implementation of that is bogus and multiplying by 2.0
> wouldn't make it better if CheckPointDistanceEstimate is wrong.

Yes, this is wrong. My apologies if my words looked confusing. By
reading your message I can see that our thoughts are on the same page.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-11-07 Thread Michael Paquier
On Wed, Nov 8, 2017 at 2:26 AM, Jacob Champion <pchamp...@pivotal.io> wrote:
> On Mon, Nov 6, 2017 at 6:18 PM, Michael Paquier
> <michael.paqu...@gmail.com> wrote:
>> Did you really test WAL replay?
>
> Is there a way to test this other than installcheck-world? The only
> failure we've run into at the moment is in the snapshot-too-old tests.
> Maybe we're not configuring with all the tests enabled?

Not automatically. The simplest method I use in this case is
installcheck with a standby replaying changes behind.

>>> The assertion added caught at least one code path where TestForOldSnapshot
>>> calls PageGetLSN without holding any lock.  The snapshot_too_old test in
>>> "check-world" failed due to the assertion failure.  This needs to be fixed,
>>> see the open question in the opening mail on this thread.
>>
>> Good point. I am looping Kevin Grittner here for his input, as the
>> author and committer of old_snapshot_threshold. Things can be
>> addressed with a separate patch by roughly moving the check on
>> PageGetLSN() to TestForOldSnapshot_impl() and use BufferGetLSNAtomic()
>> instead.
>
> It still doesn't pass the tests, as not all code paths ensure that a
> content lock is held before calling TestForOldSnapshot.
> BufferGetLSNAtomic only adds the spinlock.

I would prefer waiting for more input from Kevin here. This may prove
to be a more invasive change. There are no objections into fixing the
existing callers in index AMs though.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] LDAP URI decoding bugs

2017-11-06 Thread Michael Paquier
On Fri, Nov 3, 2017 at 12:57 PM, Thomas Munro
 wrote:
> 1.  If you set up a pg_hba.conf with a URL that lacks a base DN or
> hostname, hba.c will segfault on startup when it tries to pstrdup a
> null pointer.  Examples: ldapurl="ldap://localhost; and
> ldapurl="ldap://;.
>
> 2.  If we fail to bind but have no binddn configured, we'll pass NULL
> to ereport (snprint?) for %s, which segfaults on some libc
> implementations.  That crash requires more effort to reproduce but you
> can see pretty clearly a few lines above in auth.c that it can be
> NULL.  (I'm surprised Coverity didn't complain about that.  Maybe it
> can't see this code due to macros.)

Good question. Indeed Coverity did not complain here, perhaps because
the compiled build is not using openldap?

> Please see attached.

Oops. So...

-hbaline->ldapserver = pstrdup(urldata->lud_host);
+if (urldata->lud_host)
+hbaline->ldapserver = pstrdup(urldata->lud_host);
This prevents the backend to blow up on ldap://.

-   hbaline->ldapbasedn = pstrdup(urldata->lud_dn);
+   if (urldata->lud_dn)
+   hbaline->ldapbasedn = pstrdup(urldata->lud_dn);
And this prevents the crash on ldap://localhost.

-port->hba->ldapbinddn, port->hba->ldapserver,
+port->hba->ldapbinddn ? port->hba->ldapbinddn : "",
+port->hba->ldapserver,
ldapserver should never be NULL thanks to the check on
MANDATORY_AUTH_ARG in parse_hba_line(), still I would tend to be
maniak and do the same check as for ldapbinddn. That feels safer
thinking long-term.

Please note that I have added as well an entry in the next CF to avoid
that bug falling into oblivion:
https://commitfest.postgresql.org/16/1372/
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [PATCH] A hook for session start

2017-11-06 Thread Michael Paquier
On Sun, Nov 5, 2017 at 3:14 AM, Fabrízio de Royes Mello
<fabriziome...@gmail.com> wrote:
> On Sat, Nov 4, 2017 at 1:23 AM, Michael Paquier <michael.paqu...@gmail.com>
> wrote:
>> On Fri, Nov 3, 2017 at 1:55 PM, Fabrízio de Royes Mello
>> <fabriziome...@gmail.com> wrote:
>> >> Passing the database name and user name does not look much useful to
>> >> me. You can have access to this data already with CurrentUserId and
>> >> MyDatabaseId.
>> >
>> > This way we don't need to convert oid to names inside hook code.
>>
>> Well, arguments of hooks are useful if they are used. Now if I look at
>> the two examples mainly proposed in this thread, be it in your set of
>> patches or the possibility to do some SQL transaction, I see nothing
>> using them. So I'd vote for keeping an interface minimal.
>>
>
> Maybe the attached patch with improved test module can illustrate better the
> feature.

I was going to to hack something like that. That's interesting for the
use case Robert has mentioned.

Well, in the case of the end session hook, those variables are passed
to the hook by being directly taken from the context from MyProcPort:
+   (*session_end_hook) (MyProcPort->database_name,
MyProcPort->user_name);
In the case of the start hook, those are directly taken from the
command outer caller, but similarly MyProcPort is already set, so
those are strings available (your patch does so in the end session
hook)... Variables in hooks are useful if those are not available
within the memory context, and refer to a specific state where the
hook is called. For example, take the password hook, this uses the
user name and the password because those values are not available
within the session context. The same stands for other hooks as well.
Keeping the interface minimal helps in readability and maintenance.
See for the attached example that can be applied on top of 0003, which
makes use of the session context, the set regression tests does not
pass but this shows how I think those hooks had better be shaped.

+   (*session_end_hook) (MyProcPort->database_name, MyProcPort->user_name);
+
+   /* Make don't leave any active transactions and/or locks behind */
+   AbortOutOfAnyTransaction();
+   LockReleaseAll(USER_LOCKMETHOD, true);
Let's leave this work to people actually implementing the hook contents.
-- 
Michael


session_hook_simplify.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-11-06 Thread Michael Paquier
On Tue, Nov 7, 2017 at 7:27 AM, Asim Praveen <aprav...@pivotal.io> wrote:
> On Mon, Oct 2, 2017 at 6:48 PM, Michael Paquier <michael.paqu...@gmail.com>
> wrote:
>> Jacob, here are some ideas to make this thread move on. I would
>> suggest to produce a set of patches that do things incrementally:
>> 1) One patch that changes the calls of PageGetLSN to
>> BufferGetLSNAtomic which are now not appropriate. You have spotted
>> some of them in the btree and gist code, but not all based on my first
>> lookup. There is still one in gistFindCorrectParent and one in btree
>> _bt_split. The monitoring of the other calls (sequence.c and
>> vacuumlazy.c) looked safe. There is another one in XLogRecordAssemble
>> that should be fixed I think.
>
> Thank you for your suggestions.  Please find the first patch attached as
> "0001-...".  We verified both, gistFindCorrectParent and _bt_split and all
> calls to PageGetLSN are made with exclusive lock on the buffer contents
> held.

Cool. Thanks for double-checking. XLogRecordAssemble() is fine after
more lookup at this code, XLogRegisterBuffer already doing some sanity
checks.

>> 2) A second patch that strengthens a bit checks around
>> BufferGetLSNAtomic. One idea would be to use LWLockHeldByMe, as you
>> are originally suggesting.
>> A comment could be as well added in bufpage.h for PageGetLSN to let
>> users know that it should be used carefully, in the vein of what is
>> mentioned in src/backend/access/transam/README.
>
> The second patch "0002-..." does the above.  We have a comment added to
> AssertPageIsLockedForLSN as suggested.

Did you really test WAL replay? This still ignores that PageGetLSN is
as well taken in some code paths, like recovery, where actions on the
page are guaranteed to be serialized, like during recovery, so this
patch would cause the system to blow up. Note that pageinspect,
amcheck and wal_consistency_checking also process on page copies. So
the assertion failure of 0002 would trigger in those cases.

> The assertion added caught at least one code path where TestForOldSnapshot
> calls PageGetLSN without holding any lock.  The snapshot_too_old test in
> "check-world" failed due to the assertion failure.  This needs to be fixed,
> see the open question in the opening mail on this thread.

Good point. I am looping Kevin Grittner here for his input, as the
author and committer of old_snapshot_threshold. Things can be
addressed with a separate patch by roughly moving the check on
PageGetLSN() to TestForOldSnapshot_impl() and use BufferGetLSNAtomic()
instead.

The commit fest has lost view of this entry, and so did I. So I have
added a new entry:
https://commitfest.postgresql.org/16/1371/

BufferGetLSNAtomic() could really use LWLockHeldByMe(). Could you
consider it with an extra patch on top of 0001?

It seems to me that 0001 is good for a committer lookup, that will get
rid of all existing bugs. For 0002, what you are proposing is still
not a good idea for anything using page copies. Here are some
suggestions:
- Implement a PageGetLSNFromCopy, dedicated at working correctly when
working on a page copy. Then switch callers of amcheck, pageinspect
and wal_consistency_checking to use that.
- Implement something like GetLSNFromLockedPage, and switch of
backend's PageGetLSN to that. Performance impact could be seen..
- Have a PageGetLSNSafe, which can be used safely for serialized actions.
It could be an idea to remove PageGetLSN to force a breakage of
extensions calling it, so as they would review any of its calls. Not a
fan of that though.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Early locking option to parallel backup

2017-11-05 Thread Michael Paquier
On Sun, Nov 5, 2017 at 7:17 PM, Lucas  wrote:
> The patch creates a "--lock-early" option which will make pg_dump to issue
> shared locks on all tables on the backup TOC on each parallel worker start.
> That way, the backup has a very small chance of failing. When it does,
> happen in the first few seconds of the backup job. My backup scripts (not
> included here) are aware of that and retries the backup in case of failure.

You should register your patch to the next opened commit fest, which
will begin in January, if you are looking for feedback and review:
https://commitfest.postgresql.org/16/
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [PATCH] A hook for session start

2017-11-03 Thread Michael Paquier
On Fri, Nov 3, 2017 at 1:55 PM, Fabrízio de Royes Mello
 wrote:
>> Passing the database name and user name does not look much useful to
>> me. You can have access to this data already with CurrentUserId and
>> MyDatabaseId.
>
> This way we don't need to convert oid to names inside hook code.

Well, arguments of hooks are useful if they are used. Now if I look at
the two examples mainly proposed in this thread, be it in your set of
patches or the possibility to do some SQL transaction, I see nothing
using them. So I'd vote for keeping an interface minimal.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Exclude pg_internal.init from base backup

2017-11-03 Thread Michael Paquier
On Fri, Nov 3, 2017 at 4:04 PM, Petr Jelinek
 wrote:
> Not specific problem to this patch, but I wonder if it should be made
> more clear that those files (there are couple above of what you added)
> are skipped no matter which directory they reside in.

Agreed, it is a good idea to tell in the docs how this behaves. We
could always change things so as the comparison is based on the full
path like what is done for pg_control, but that does not seem worth
complicating the code.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [PATCH] A hook for session start

2017-11-03 Thread Michael Paquier
On Thu, Nov 2, 2017 at 11:36 PM, Fabrízio de Royes Mello
 wrote:
> On Thu, Nov 2, 2017 at 5:42 AM, Aleksandr Parfenov
>  wrote:
>> Unfortunately, patches 0001 and 0002 don't apply to current master.
>>
>> The new status of this patch is: Waiting on Author
>
> Thanks for your review. Rebased versions attached.

Looking at this thread, there are clearly arguments in favor of having
a session hook after authentication. One use case mentioned by Robert
is inserting data into a table when a user logs in. I can imagine that
something like that could be applied to a session ending.

 /*
+ * Setup handler to session end hook
+ */
+if (IsUnderPostmaster)
+on_proc_exit(do_session_end_hook, 0);
I think that it would be better to place that in ShutdownPostgres.
This way it is possible to take actions before any resource is shut
down.

Passing the database name and user name does not look much useful to
me. You can have access to this data already with CurrentUserId and
MyDatabaseId.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] SQL/JSON in PostgreSQL

2017-11-03 Thread Michael Paquier
On Fri, Nov 3, 2017 at 12:07 PM, Michael Paquier
<michael.paqu...@gmail.com> wrote:
> The patch sent previously does not directly apply on HEAD, and as far
> as I can see the last patch set published on
> https://www.postgresql.org/message-id/2361ae4a-66b1-c6c5-ea6a-84851a1c0...@postgrespro.ru
> has rotten. Could you send a new patch set?
>
> About the patch set, I had a look at the first patch which is not that
> heavy, however it provides zero documentation, close to zero comments,
> but adds more than 500 lines of code. I find that a bit hard to give
> an opinion on, having commit messages associated to each patch would
> be also nice. This way, reviewers can figure what's going out in this
> mess and provide feedback. Making things incremental is welcome as
> well, for example in the first patch I have a hard way finding out why
> timestamps are touched to begin with.

My mistake here, only the first patch adds 8,200 lines of code. This
makes the lack of comments and docs even worse.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Is it time to kill support for very old servers?

2017-11-03 Thread Michael Paquier
On Mon, Oct 16, 2017 at 10:08 PM, Andres Freund  wrote:
> On 2017-10-16 16:59:59 -0400, Peter Eisentraut wrote:
>> On 9/20/17 04:32, Andres Freund wrote:
>> > Here's what I roughly was thinking of.  I don't quite like the name, and
>> > the way the version is specified for libpq (basically just the "raw"
>> > integer).
>>
>> "forced_protocol_version" reads wrong to me.  I think
>> "force_protocol_version" might be better.  Other than that, no issues
>> with this concept.
>
> Yea, I agree. I've read through the patch since, and it struck me as
> odd. Not sure how I came up with it...

Andres, could you update the patch?
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] SQL/JSON in PostgreSQL

2017-11-03 Thread Michael Paquier
On Fri, Nov 3, 2017 at 11:29 AM, Nikita Glukhov  wrote:
> By standard only string literals can be used in JSON path specifications.
> But of course it is possible to allow to use variable jsonpath expressions
> in
> SQL/JSON functions.
>
> Attached patch implements this feature for JSON query functions, JSON_TABLE
> is
> not supported now because it needs some refactoring.
>
> I have pushed this commit to the separate branch because it is not finished
> yet:
> https://github.com/postgrespro/sqljson/tree/sqljson_variable_json_path

The patch sent previously does not directly apply on HEAD, and as far
as I can see the last patch set published on
https://www.postgresql.org/message-id/2361ae4a-66b1-c6c5-ea6a-84851a1c0...@postgrespro.ru
has rotten. Could you send a new patch set?

About the patch set, I had a look at the first patch which is not that
heavy, however it provides zero documentation, close to zero comments,
but adds more than 500 lines of code. I find that a bit hard to give
an opinion on, having commit messages associated to each patch would
be also nice. This way, reviewers can figure what's going out in this
mess and provide feedback. Making things incremental is welcome as
well, for example in the first patch I have a hard way finding out why
timestamps are touched to begin with.

The patch is already marked as "waiting on author" for more than one month.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] postgres_fdw: Add support for INSERT OVERRIDING clause

2017-11-03 Thread Michael Paquier
On Tue, Oct 31, 2017 at 3:45 PM, Peter Eisentraut
 wrote:
> It has been pointed out to me that the command deparsing in postgres_fdw
> does not support the INSERT OVERRIDING clause that was added in PG10.
> Here is a patch that seems to fix that.  I don't know much about this,
> whether anything else needs to be added or whether there should be
> tests.  Perhaps someone more familiar with postgres_fdw details can
> check it.

Trying to insert some data using OVERRIDING SYSTEM VALUE on a foreign
table with a remote relation defined with GENERATED ALWAYS would just
fail:
=# insert into id_always_foreign OVERRIDING system VALUE values (8);
ERROR:  428C9: cannot insert into column "a"
DETAIL:  Column "a" is an identity column defined as GENERATED ALWAYS.
HINT:  Use OVERRIDING SYSTEM VALUE to override.

And that's confusing because there is no actual way to avoid this
error if postgres_fdw is unpatched.

I think that you should add some tests, and make sure that the
documentation of postgres-fdw.sgml mentions that those two clauses are
pushed down.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [bug fix] postgres.exe crashes with access violation on Windows while starting up

2017-11-03 Thread Michael Paquier
On Wed, Nov 1, 2017 at 12:07 AM, Tsunakawa, Takayuki
<tsunakawa.ta...@jp.fujitsu.com> wrote:
> From: Michael Paquier [mailto:michael.paqu...@gmail.com]
>> On Tue, Oct 31, 2017 at 6:59 AM, Tsunakawa, Takayuki
>> <tsunakawa.ta...@jp.fujitsu.com> wrote:
>> > When CurrentMemoryContext is NULL, the message  is logged with
>> ReportEventA().  This is similar to write_console().
>>
>> My point is that as Postgres is running as a service, isn't it wrong to
>> write a message to stderr as a fallback if the memory context is not set?
>> You would lose a message. It seems to me that for an operation that can
>> happen at a low-level like the postmaster startup, we should really use
>> a low-level operation as well.
>
> I'm sorry I may not have been clear.  With this patch, write_eventlog() 
> outputs the message to event log with ReportEventA() when 
> CurrentMemoryContext is NULL.  It doesn't write to stderr.  So the message 
> won't be lost.

Oh, yes. Sorry. I got confused a bit by write_eventlog(), which is
already doing what your patch is adding for write_eventlog(). I am
switching the patch as ready for committer, I definitely agree that
you are taking the write approach here.

I am also adding Noah to get some input on this issue, as he is the
author and committer of 5f538ad0 which has improved the handling of
non-ASCII characters in this code path, and more importantly has
tweaked 43adc7a7 to handle properly transaction contexts in
pgwin32_message_to_UTF16() which is where the palloc calls happen. I
would be the one in the pool of committers who would most likely
commit your patch.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Setting pd_lower in GIN metapage

2017-11-02 Thread Michael Paquier
On Fri, Nov 3, 2017 at 1:10 AM, Amit Kapila  wrote:
> On Fri, Nov 3, 2017 at 2:54 AM, Tom Lane  wrote:
>> I've marked the CF entry closed.  However, I'm not sure if we're quite
>> done with this thread.  Weren't we going to adjust nbtree and hash to
>> be more aggressive about labeling their metapages as REGBUF_STANDARD?
>
> I have already posted the patches [1] for the same in this thread and
> those are reviewed [2][3] as well. I have adjusted the comments as per
> latest commit.   Please find updated patches attached.

Confirmed. Setting those makes sense even if REGBUF_WILL_INIT is set,
at least for page masking.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] SSL and Encryption

2017-11-02 Thread Michael Paquier
On Fri, Nov 3, 2017 at 3:19 AM, Craig Ringer  wrote:
> This is probably off topic for pgsql-hackers.
>
> For password crypto please go read the SCRAM thread and the PostgreSQL
> 10 release notes.

The SCRAM discussion is spread across two threads mainly with hundreds
of emails, which may discourage even the bravest. Here are links to
the important documentation:
https://www.postgresql.org/docs/current/static/auth-methods.html#auth-password
https://www.postgresql.org/docs/10/static/sasl-authentication.html

And PostgreSQL implements SCRAM-SHA-256 following RFCs 7677 and 5802:
https://tools.ietf.org/html/rfc5802
https://tools.ietf.org/html/rfc7677
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Removing wal_keep_segments as default configuration in PostgresNode.pm

2017-11-02 Thread Michael Paquier
On Thu, Nov 2, 2017 at 4:47 PM, Peter Eisentraut
<peter.eisentr...@2ndquadrant.com> wrote:
> On 9/11/17 21:55, Michael Paquier wrote:
>> I tend to think that while all the other parameters make sense to
>> deploy instances that need few resources, wal_keep_segments may cause
>> up to 350MB of WAL segments to be kept in each pg_wal's instance,
>> while max_wal_size is set at 128MB. The only test in the code tree in
>> need of wal_keep_segments is actually pg_rewind, which enforces
>> checkpoints after the rewind to update the source's control file.
>>
>> So, thoughts about the attached that reworks this portion of PostgresNode.pm?
>
> Committed.
>
> Besides the resource usage, it would probably be bad if a
> wal_keep_segments setting papered over problems with replication slots
> for example.

Thanks! I almost forgot this patch.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] pg_basebackup fails on Windows when using tablespace mapping

2017-11-02 Thread Michael Paquier
On Thu, Nov 2, 2017 at 2:05 AM, Peter Eisentraut
<peter.eisentr...@2ndquadrant.com> wrote:
> On 11/1/17 10:29, Michael Paquier wrote:
>> On Wed, Nov 1, 2017 at 2:24 PM, Peter Eisentraut
>> <peter.eisentr...@2ndquadrant.com> wrote:
>>> Committed to master.  I suppose this should be backpatched?
>>
>> Thanks! Yes this should be back-patched.
>
> OK, done for 10, 9.6, and 9.5.
>
> The tablespace mapping feature started in 9.4 (has it been that long
> already?), but the canonicalization was only added in 9.5.

Thank you.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Commit fest 2017-11

2017-11-01 Thread Michael Paquier
On Wed, Nov 1, 2017 at 9:04 AM, Michael Paquier
<michael.paqu...@gmail.com> wrote:
> Anybody willing to take the hat of the commit fest manager? If nobody,
> I am fine to take the hat as default choice this time.

And now it is open. Let's the fest begin.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] pg_basebackup fails on Windows when using tablespace mapping

2017-11-01 Thread Michael Paquier
On Wed, Nov 1, 2017 at 2:24 PM, Peter Eisentraut
 wrote:
> Committed to master.  I suppose this should be backpatched?

Thanks! Yes this should be back-patched.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Commit fest 2017-11

2017-11-01 Thread Michael Paquier
Hi all,

At the moment of writing this email, it is 9PM AoE (Anywhere on Earth)
31st of October. This means that the next commit fest will begin in 3
hours, and that any hackers willing to register patches for this
commit fest have roughly three hours to do so (plus/minus N hours).

This current score is the following:
Needs review: 125.
Waiting on Author: 22.
Ready for Committer: 39.
This represents a total of 186 patches still pending for review, for
the second largest commit fest ever.

Anybody willing to take the hat of the commit fest manager? If nobody,
I am fine to take the hat as default choice this time.

Thanks,
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Remove secondary checkpoint

2017-10-31 Thread Michael Paquier
On Tue, Oct 31, 2017 at 2:00 PM, Simon Riggs  wrote:
> On 30 October 2017 at 18:58, Simon Riggs  wrote:
>> On 30 October 2017 at 15:22, Simon Riggs  wrote:
>>
 You forgot to update this formula in xlog.c:
 distance = (2.0 + CheckPointCompletionTarget) * 
 CheckPointDistanceEstimate;
 /* add 10% for good measure. */
 distance *= 1.10;
 You can guess easily where the mistake is.
>>>
>>> Doh - fixed that before posting, so I must have sent the wrong patch.
>>>
>>> It's the 10%, right? ;-)
>>
>> So, there are 2 locations that mention 2.0 in xlog.c
>>
>> I had already fixed one, which is why I remembered editing it.
>>
>> The other one you mention has a detailed comment above it explaining
>> why it should be 2.0 rather than 1.0, so I left it.
>>
>> Let me know if you still think it should be removed? I can see the
>> argument both ways.
>> Or maybe we need another patch to account for manual checkpoints.
>
> Revised patch to implement this

Here is the comment and the formula:
 * The reason this calculation is done from the prior
checkpoint, not the
 * one that just finished, is that this behaves better if some
checkpoint
 * cycles are abnormally short, like if you perform a manual checkpoint
 * right after a timed one. The manual checkpoint will make
almost a full
 * cycle's worth of WAL segments available for recycling, because the
 * segments from the prior's prior, fully-sized checkpoint cycle are no
 * longer needed. However, the next checkpoint will make only
few segments
 * available for recycling, the ones generated between the timed
 * checkpoint and the manual one right after that. If at the manual
 * checkpoint we only retained enough segments to get us to
the next timed
 * one, and removed the rest, then at the next checkpoint we would not
 * have enough segments around for recycling, to get us to the
checkpoint
 * after that. Basing the calculations on the distance from
the prior redo
 * pointer largely fixes that problem.
 */
distance = (2.0 + CheckPointCompletionTarget) *
CheckPointDistanceEstimate;

While the mention about a manual checkpoint happening after a timed
one will cause a full range of WAL segments to be recycled, it is not
actually true that segments of the prior's prior checkpoint are not
needed, because with your patch the segments of the prior checkpoint
are getting recycled. So it seems to me that based on that the formula
ought to use 1.0 instead of 2.0...
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [bug fix] postgres.exe crashes with access violation on Windows while starting up

2017-10-31 Thread Michael Paquier
On Tue, Oct 31, 2017 at 6:59 AM, Tsunakawa, Takayuki
<tsunakawa.ta...@jp.fujitsu.com> wrote:
> From: pgsql-hackers-ow...@postgresql.org
>> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
>> So you are basically ready to lose any message that could be pushed
>> here if there is no memory context? That does not sound like a good
>> trade-off to me. A static buffer does not look like the best idea
>> either to not truncate message, so couldn't we envisage to just use
>> malloc? pgwin32_message_to_UTF16() is called in two places in elog.c,
>> and there is a full control on the error code paths.
>
> Thank you for reviewing a rare bug fix on Windows that most people wouldn't 
> be interested in.

Yeah, it may take a while until a committer gets interested I am
afraid. See my bug about pg_basebackup on Windows with path names..

> When CurrentMemoryContext is NULL, the message  is logged with 
> ReportEventA().  This is similar to write_console().

My point is that as Postgres is running as a service, isn't it wrong
to write a message to stderr as a fallback if the memory context is
not set? You would lose a message. It seems to me that for an
operation that can happen at a low-level like the postmaster startup,
we should really use a low-level operation as well.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Re: PANIC: invalid index offnum: 186 when processing BRIN indexes in VACUUM

2017-10-31 Thread Michael Paquier
On Tue, Oct 31, 2017 at 4:56 AM, Tom Lane  wrote:
> Alvaro Herrera  writes:
>> Tom Lane wrote:
>>> So: I put the blame on the fact that summarize_range() thinks that
>>> the tuple offset it has for the placeholder tuple is guaranteed to
>>> hold good, even across possibly-long intervals where it's holding
>>> no lock on the containing buffer.
>
>> Yeah, I think this is a pretty reasonable explanation for the problem.
>> I don't understand why it doesn't fail in 9.6.
>
> Yeah, we're still missing an understanding of why we didn't see it
> before; the inadequate locking was surely there before.  I'm guessing
> that somehow the previous behavior of PageIndexDeleteNoCompact managed
> to mask the problem (perhaps only by not throwing an error, which doesn't
> imply that the index state was good afterwards).  But I don't see quite
> how it did that.

Because 24992c6d has added a check on the offset number by using
PageIndexTupleDeleteNoCompact() in brin_doupdate() making checks
tighter, no? I have not tested, and I lack knowledge about the brin
code, but it seems to me that if we had a similar check then things
could likely blow up.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Re: PANIC: invalid index offnum: 186 when processing BRIN indexes in VACUUM

2017-10-30 Thread Michael Paquier
On Mon, Oct 30, 2017 at 9:42 AM, Alvaro Herrera  wrote:
> Tomas Vondra wrote:
>> FWIW I can reproduce this on REL_10_STABLE, but not on REL9_6_STABLE. So
>> it seems to be due to something that changed in the last release.
>
> I've been trying to reproduce it for half an hour with no success (I
> think my laptop is just too slow). I bet this is related to the
> addition of PageIndexTupleOverwrite (commit b1328d78f) though I find it
> more likely that this was not *caused* by that commit but rather just
> made it easier to hit.

b1328d78f is close enough, but per what I see that's actually not
true. I have been able to reproduce the problem, which shows up within
a window of 2-4 minutes. Still sometimes it can take way longer, and I
spotted one test where it took 15 minutes to show up... So, bisecting
with a test that looks for core files for 20 minutes, I am seeing that
the following commit is actually at fault:
commit 24992c6db9fd40f342db1f22747ec9e56483796d
Author: Tom Lane 
Date:   Fri Sep 9 19:00:59 2016 -0400

Rewrite PageIndexDeleteNoCompact into a form that only deletes 1 tuple.

The full generality of deleting an arbitrary number of tuples is no longer
needed, so let's save some code and cycles by replacing the original coding
with an implementation based on PageIndexTupleDelete.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Remove secondary checkpoint

2017-10-30 Thread Michael Paquier
On Mon, Oct 30, 2017 at 2:22 PM, Simon Riggs <si...@2ndquadrant.com> wrote:
> On 25 October 2017 at 00:17, Michael Paquier <michael.paqu...@gmail.com> 
> wrote:
>> -* Delete old log files (those no longer needed even for previous
>> -* checkpoint or the standbys in XLOG streaming).
>> +* Delete old log files and recycle them
>>  */
>> Here that's more "Delete or recycle old log files". Recycling of a WAL
>> segment is its renaming into a newer segment.
>
> Sometimes files are deleted if there are too many.

Sure, but one segment is never deleted and then recycled, which is
what your new comment implies as I understand it.

>> -   checkPointLoc = ControlFile->prevCheckPoint;
>> +   /*
>> +* It appears to be a bug that we used to use
>> prevCheckpoint here
>> +*/
>> +   checkPointLoc = ControlFile->checkPoint;
>> Er, no. This is correct because we expect the prior checkpoint to
>> still be present in the event of a failure detecting the latest
>> checkpoint.
>
> I'm removing "prevCheckPoint", so not sure what you mean.

I mean that there is no actual bug here. And changing the code as you
do is correct, but the comment is not.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] pow support for pgbench

2017-10-30 Thread Michael Paquier
On Mon, Oct 30, 2017 at 11:56 AM, Raúl Marín Rodríguez
 wrote:
> both patches seem complementary. I've rebased my changes on top of that
> patch
> (v14) in https://git.io/vFtnT and everything seems to be working fine.

Attaching patches directly to a thread is a better practice as if
github goes away, any Postgres developers can still have an access to
any code you publish using the public archives on postgresql.org.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] pow support for pgbench

2017-10-30 Thread Michael Paquier
On Mon, Oct 30, 2017 at 11:07 AM, Alvaro Herrera
<alvhe...@alvh.no-ip.org> wrote:
> Michael Paquier wrote:
>
>> Please add this patch to the upcoming commit fest if you would like to
>> get some feedback:
>> https://commitfest.postgresql.org/15/
>>
>> I am adding as well Fabien in CC who worked in getting the internal
>> function infrastructure in the shape it is now (waaay better) with
>> commit 86c43f4.
>
> I think Raúl would do well to review this patch by Fabien
> https://www.postgresql.org/message-id/alpine.DEB.2.20.1710201835390.15170@lancre
> which adds a few functions and operators.

Good idea. pow() is not added by Fabien's patch, but an operator for
pow() could be something to add as well.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] pow support for pgbench

2017-10-30 Thread Michael Paquier
On Fri, Oct 27, 2017 at 4:51 PM, Raúl Marín Rodríguez
 wrote:
> I've written a small patch to add support for pow() in pgbench.

Cool.

> The main reason behind it is that I'm currently using a shell call to do it
> which takes between 2-10 ms that can be a big percentage of the time taken
> by the whole transaction. For example (shortened):
>
> latency average = 11.718 ms
>  - statement latencies in milliseconds:
>  2.834  \setshell POWER2  awk 'BEGIN {p=2^ARGV[1]; print p }'
> :ZOOM_CURRENT
>  8.846  SELECT
> ST_AsBinary(ST_Simplify(ST_SnapToGrid("the_geom_webmercator",:SNAP),
> :SIMPLIFY)) AS geom FROM
>
> I've also updated the related docs and added some tests. Please let me know
> if I'm missing anything.

Please add this patch to the upcoming commit fest if you would like to
get some feedback:
https://commitfest.postgresql.org/15/

I am adding as well Fabien in CC who worked in getting the internal
function infrastructure in the shape it is now (waaay better) with
commit 86c43f4.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] WIP: Restricting pg_rewind to data/wal dirs

2017-10-30 Thread Michael Paquier
On Mon, Oct 30, 2017 at 10:15 AM, Chris Travers
 wrote:
> This also brings up a fairly major concern more generally about control by
> the way.  A lot of cases where pg_rewind is called, the user doesn't
> necessarily have much control on how it is called.  Moreover in many of
> these cases, the user is probably not in a position to understand the
> internals well enough to grasp what to check after.

Likely they are not.

> Right, but there is a use case difference between "I am taking a backup of a
> server" and "I need to get the server into  rejoin the replication as a
> standby."

The intersection of the first and second categories is not empty. Some
take backups and use them to deploy standbys.

> A really good example of where this is a big problem is with replication
> slots.  On a backup I would expect you want replication slots to be copied
> in.

I would actually expect the contrary, and please note that replication
slots are not taken in a base backup, which is what the documentation
says as well:
https://www.postgresql.org/docs/10/static/protocol-replication.html
"pg_dynshmem, pg_notify, pg_replslot, pg_serial, pg_snapshots,
pg_stat_tmp, and pg_subtrans are copied as empty directories (even if
they are symbolic links)."

Some code I have with 9.6's pg_bsaebackup removes manually replication
slots as this logic is new in 10 ;)

>> The pattern that base backups now use is an exclude list. In the
>> future I would rather see base backups and pg_rewind using the same
>> infrastructure for both things:
>> - pg_rewind should use the replication protocol with BASE_BACKUP.
>> Having it rely on root access now is crazy.
>> - BASE_BACKUP should have an option where it is possible to exclude
>> custom paths.
>> What you are proposing here would make both diverge more, which is in
>> my opinion not a good approach.
>
> How does rep mgr or other programs using pg_rewind know what to exclude?

Good question. Answers could come from folks such as David Steele
(pgBackRest) or Marco (barman) whom I am attaching in CC.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] WIP: Restricting pg_rewind to data/wal dirs

2017-10-30 Thread Michael Paquier
On Mon, Oct 30, 2017 at 9:43 AM, Chris Travers  wrote:
> Are there any cases right now where you have features added by extensions 
> that write to directories which are required for a rewind?

In some of the stuff I maintain, I actually have one case now of a
configuration file included with include_if_exists by postgresql.conf
and is expected to be generated by a component that my code doing the
rewind has no direct access on... I can control how pg_rewind kicks
in, but I think that you would actually break silently the current
code, which is scary especially if I don't control the code where
pg_rewind is called when Postgres 11 gets integrated into the product
I am thinking about, and even more scary if there is no way to include
something.

> The problem with an exclude list is that we cannot safely exclude
> configuration files or logs (because these could be named anything), right?

You have the exact same problem with base backups now, and any
configuration files created by extensions are a per-case problem...
The pattern that base backups now use is an exclude list. In the
future I would rather see base backups and pg_rewind using the same
infrastructure for both things:
- pg_rewind should use the replication protocol with BASE_BACKUP.
Having it rely on root access now is crazy.
- BASE_BACKUP should have an option where it is possible to exclude
custom paths.
What you are proposing here would make both diverge more, which is in
my opinion not a good approach.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] WIP: Restricting pg_rewind to data/wal dirs

2017-10-30 Thread Michael Paquier
On Sat, Oct 28, 2017 at 4:22 AM, Chris Travers  wrote:
> The Solution:
> The solution is a whitelist of directories specified which are the only ones
> which are synchronised.  The relevant part of this patch is:
>
> +/* List of directories to synchronize:
> + * base data dirs (and ablespaces)
> + * wal/transaction data
> + * and that is it.
> + *
> + * This array is null-terminated to make
> + * it easy to expand
> + */
>
> +const char *rewind_dirs[] = {
> +"base",
> +"global",
> +"pg_commit_ts",
> +"pg_logical",
> +"pg_multixact",
> +"pg_serial",
> +"pg_subtrans",
> +"pg_tblspc",
> +"pg_twophase",
> +"pg_wal",
> +"pg_xact",
> +NULL
> +};

The problem with a white list, which is one reason why I do not favor
it, is in the case where a feature adds in the data folder a new path
for its data, particularly in the case where this is critical for a
base backup. If this folder is not added in this list, then a rewind
may be silently corrupted as well. There are also a set of directories
in this list that we do not care about, pg_serial being one.
pg_subtrans is a second, as it gets zeroed on startup.

And if you think about it, pg_rewind is actually the *same* processing
as a base backup taken using the replication protocol. So instead of
this patch I would recommend using excludeFiles and excludeDirContents
by moving this list to a common header where frontend and backend can
refer to. Note that basebackup.h includes replnodes.h, so my thoughts
is that you should split the current header with something like
basebackup_internal.h which is backend-only, and have pg_rewind fetch
the list of directories to automatically ignore as those ones. You can
also simplify a bit the code of pg_rewind a bit so as things like
postmaster.opts. On top of that, there is visibly no need to tweak the
SQL query fetching the directory list (which is useful for debugging
as shaped now actually), but just change process_source_file so as its
content is not added in the file map.

Then there is a second case where you do not want a set of folders to
be included, but those can be useful by default, an example here being
pg_wal where one might want to have an empty path to begin with. A
third case is a set of folders that you do not care about, but depends
on the deployment, being here "log" for example. For those you could
just add an --exclude-path option which simply piles up paths into a
linked list when called multiple times. This could happen with a
second patch.

> From there we iterate over this array for directory-based approaches in
> copy_fetch.c and with one query per directory in libpq_fetch.c.  This also
> means shifting from the basic interface from PQexec to PQexecParams.  It
> would be possible to move to binary formats too, but this was not done
> currently in order to simplify code review (that could be a separate
> independent patch at a later time).

-res = PQexec(conn, sql);
+for (p = 0; rewind_dirs[p] != NULL; ++p)
+{
+const char *paths[1];
+paths[0] = rewind_dirs[p];
+res = PQexecParams(conn, sql,  1, NULL, paths, NULL, NULL, 0);
Calling multiple times the query to list all directories is messy IMO.
And this is N-costly processing if there are many files to look at,
say many relation files.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Reading timeline from pg_control on replication slave

2017-10-30 Thread Michael Paquier
On Mon, Oct 30, 2017 at 2:48 AM, Craig Ringer  wrote:
> (I'd quite like ThisTimeLineID to go away as a global. It's messy and
> confusing, and I'd much rather it be fetched when needed).

Yeah, I agree. My take on the matter is that we could just use the
status data of the control file which is in shared memory as the only
writers to it are the checkpointer and the startup processes.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] taking stdbool.h into use

2017-10-29 Thread Michael Paquier
On Thu, Oct 26, 2017 at 9:25 AM, Peter Eisentraut
 wrote:
> Here is an updated patch set.  This is just a rebase of the previous
> set, no substantial changes.  Based on the discussion so far, I'm
> proposing that 0001 through 0007 could be ready to commit after review,
> whereas the remaining two need more work at some later time.

I had a look at this patch series. Patches 1, 2 (macos headers indeed
show that NSUNLINKMODULE_OPTION_NONE is set to 0x0), 3 to 7 look fine
to me.

I spotted a couple of other things while looking at your patches and
the code tree.

-   return (ginCompareItemPointers(>itemptr, iptr) > 0) ? TRUE : FALSE;
+   return (ginCompareItemPointers(>itemptr, iptr) > 0) ? true : false;
You could simplify that at the same time by removing such things. The
"false : true" pattern is less frequent than the "true : false"
pattern.

Some comments in the code still mention FALSE or TRUE:
- hashsearch.c uses FALSE in some comments.
- In execExpr.c, ExecCheck mentions TRUE.
- AggStateIsShared mentions TRUE and FALSE.
- In arrayfuncs.c, ReadArrayStr, CopyArrayEls and ReadArrayBinary use TRUE.

0009 looks like a good idea. It would make the code less confusing for
the reader to mention HAVE_STDBOOL_H in the #endif of c.h that you are
complicating to make the end of the block clear. I am lacking energy
for 0008, so I have no opinions to offer. Sorry :p
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Implementing pg_receivewal --no-sync

2017-10-29 Thread Michael Paquier
On Sun, Oct 29, 2017 at 12:31 AM, Robert Haas <robertmh...@gmail.com> wrote:
> On Sun, Oct 29, 2017 at 3:42 AM, Michael Paquier
> <michael.paqu...@gmail.com> wrote:
>> Okay. Here is an updated patch incorporating those comments.
>
> Committed with a little wordsmithing on the documentation.

Thanks all.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] taking stdbool.h into use

2017-10-28 Thread Michael Paquier
On Thu, Oct 26, 2017 at 5:41 PM, Tom Lane  wrote:
> While warnings for this would be lovely, I don't see how we can expect to
> get any.  This is perfectly correct C code no matter whether isprimary
> is C99 bool or is typedef'd to char ... you just end up with different
> values of isprimary, should the RHS produce something other than 1/0.
> The compiler has no way to know that assigning, say, 4 in the char
> variable case is not quite your intent.  Maybe you could hope for a
> warning if the bit value were far enough left to actually not fit into
> "char", but otherwise there's nothing wrong.

This reminded me of
https://www.postgresql.org/message-id/20160212144735.7zkg5527i3un3254%40alap3.anarazel.de
which has caused commit af4472bc when using stdbool.h for MSVC
2013/2015 builds. So I would really assume that there are places where
we could see warnings.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [bug fix] postgres.exe crashes with access violation on Windows while starting up

2017-10-28 Thread Michael Paquier
On Thu, Oct 26, 2017 at 7:10 PM, Tsunakawa, Takayuki
 wrote:
> FIX
> ==
>
> Add the check "CurrentMemoryContext != NULL" in write_eventlog() as in 
> write_console().

 * Also verify that we are not on our way into error recursion
trouble due
 * to error messages thrown deep inside pgwin32_message_to_UTF16().
 */
if (!in_error_recursion_trouble() &&
+   CurrentMemoryContext != NULL &&
GetMessageEncoding() != GetACPEncoding())
{
So you are basically ready to lose any message that could be pushed
here if there is no memory context? That does not sound like a good
trade-off to me. A static buffer does not look like the best idea
either to not truncate message, so couldn't we envisage to just use
malloc? pgwin32_message_to_UTF16() is called in two places in elog.c,
and there is a full control on the error code paths.

> NOTE
> ==
>
> The reason is for not outputing the crash dump is a) the crash occurred 
> before installing the Windows exception handler 
> (pgwin32_install_crashdump_handler() call) and b) the effect of the following 
> call in postmaster is inherited in the child process.
>
> /* In case of general protection fault, don't show GUI popup 
> box */
> SetErrorMode(SEM_FAILCRITICALERRORS | SEM_NOGPFAULTERRORBOX);
>
> But I'm not sure in what order we should do 
> pgwin32_install_crashdump_handler(), startup_hacks() and steps therein, 
> MemoryContextInit().  I think that's another patch.

Perhaps. I don't have a final opinion on this matter.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Implementing pg_receivewal --no-sync

2017-10-28 Thread Michael Paquier
On Fri, Oct 27, 2017 at 12:03 AM, Michael Paquier
<michael.paqu...@gmail.com> wrote:
> On Thu, Oct 26, 2017 at 10:46 PM, Robert Haas <robertmh...@gmail.com> wrote:
>> On Wed, Oct 25, 2017 at 10:03 PM, Michael Paquier
>> <michael.paqu...@gmail.com> wrote:
>>> This sentence is actually wrong, a feedback message is never sent with
>>> the feedback message.
>>
>> Eh?
>
> "A feedback message is never sent depending on the status interval".
>
>> I think this looks basically fine, though I'd omit the short option
>> for it.  There are only so many letters in the alphabet, so let's not
>> use them up for developer-convenience options.
>
> No objections to that.

Okay. Here is an updated patch incorporating those comments.
-- 
Michael


pg_receivewal_nosync_v3.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] ALTER COLUMN TYPE vs. domain constraints

2017-10-28 Thread Michael Paquier
On Fri, Oct 27, 2017 at 11:15 AM, Tom Lane  wrote:
> We could consider back-patching the attached to cover this, but
> I'm not entirely sure it's worth the trouble, because I haven't
> thought of any non-silly use-cases in the absence of domains
> over composite.  Comments?

There are no real complaints about the current behavior, aren't there?
So only patching HEAD seems enough to me.

+comment on constraint c1 on domain dcomptype is 'random commentary';
[...]
+alter type comptype alter attribute r type bigint;
You have added a comment on the constraint to make sure that it
remains up on basically this ALTER TYPE. Querying pg_obj_description
would make sure that the comment on the constraint is still here.

+static void
+RebuildDomainConstraintComment(AlteredTableInfo *tab, int pass, Oid objid,
+  List *domname, char *conname)
There is much duplication with RebuildConstraintComment. Why not
grouping both under say RebuildObjectComment()? I would think about
having cmd->objtype and cmd->object passed as arguments, and then
remove rel and domname from the existing arguments.

[nit]
foreach(lcmd, subcmds)
-   ATExecCmd(wqueue, tab, rel, (AlterTableCmd *)
lfirst(lcmd), lockmode);
+   ATExecCmd(wqueue, tab, rel,
+ castNode(AlterTableCmd, lfirst(lcmd)),
+ lockmode);
This does not really belong to this patch.. No objections to group things.
[/nit]
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] MERGE SQL Statement for PG11

2017-10-27 Thread Michael Paquier
On Fri, Oct 27, 2017 at 9:00 AM, Robert Haas  wrote:
> On Fri, Oct 27, 2017 at 4:41 PM, Simon Riggs  wrote:
>> I didn't say it but my intention was to just throw an ERROR if no
>> single unique index can be identified.
>>
>> It could be possible to still run MERGE in that situaton but we would
>> need to take a full table lock at ShareRowExclusive. It's quite likely
>> that such statements would throw duplicate update errors, so I
>> wouldn't be aiming to do anything with that for PG11.
>
> Like Peter, I think taking such a strong lock for a DML statement
> doesn't sound like a very desirable way forward.  It means, for
> example, that you can only have one MERGE in progress on a table at
> the same time, which is quite limiting.  It could easily be the case
> that you have multiple MERGE statements running at once but they touch
> disjoint groups of rows and therefore everything works.  I think the
> code should be able to cope with concurrent changes, if nothing else
> by throwing an ERROR, and then if the user wants to ensure that
> doesn't happen by taking ShareRowExclusiveLock they can do that via an
> explicit LOCK TABLE statement -- or else they can prevent concurrency
> by any other means they see fit.

+1, I would suspect users to run this query in parallel of the same
table for multiple data sets.

Peter has taken some time to explain me a bit his arguments today, and
I agree that it does not sound much appealing to have constraint
limitations for MERGE. Particularly using the existing ON CONFLICT
structure gets the feeling of having twice a grammar for what's
basically the same feature, with pretty much the same restrictions.

By the way, this page sums up nicely the situation about many
implementations of UPSERT taken for all systems:
https://en.wikipedia.org/wiki/Merge_(SQL)
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Reading timeline from pg_control on replication slave

2017-10-27 Thread Michael Paquier
On Fri, Oct 27, 2017 at 1:04 AM, Andrey Borodin  wrote:
> I'm working on backups from replication salve in WAL-G [0]
> Backups used to use result of pg_walfile_name(pg_start_backup(...)). Call to 
> pg_start_backup() works nice, but "pg_walfile_name() cannot be executed 
> during recovery."
> This function has LSN as argument and reads TimeLineId from global state.
> So I made a function[1] that, if on replica, reads timeline from pg_control 
> file and formats WAL file name as is it was produces by pg_wal_filename(lsn).

ThisTimeLineID is not something you can rely on for standby backends
as it is not set during recovery. That's the reason behind
pg_walfile_name disabled during recovery. There are three things
popping on top of my mind that one could think about:
1) Backups cannot be completed when started on a standby in recovery
and when stopped after the standby has been promoted, meaning that its
timeline has changed.
2) After a standby has been promoted, by using pg_start_backup, you
issue a checkpoint which makes sure that the control file gets flushed
with the new information, so when pg_start_backup returns to the
caller you should have the correct timeline number because the outer
function gets evaluated last.
3) Backups taken from cascading standbys, where a direct parent has
been promoted.

1) and 2) are actually not problems per the restrictions I am giving
above, but 3) is. If I recall correctly, when a streaming standby does
a timeline jump, a restart point is not immediately generated, so you
could have the timeline on the control file not updated to the latest
timeline value, meaning that you could have the WAL file name you use
here referring to a previous timeline and not the newest one.

In short, yes, what you are doing is definitely risky in my opinion,
particularly for complex cascading setups.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Implementing pg_receivewal --no-sync

2017-10-27 Thread Michael Paquier
On Thu, Oct 26, 2017 at 10:46 PM, Robert Haas <robertmh...@gmail.com> wrote:
> On Wed, Oct 25, 2017 at 10:03 PM, Michael Paquier
> <michael.paqu...@gmail.com> wrote:
>> This sentence is actually wrong, a feedback message is never sent with
>> the feedback message.
>
> Eh?

"A feedback message is never sent depending on the status interval".

> I think this looks basically fine, though I'd omit the short option
> for it.  There are only so many letters in the alphabet, so let's not
> use them up for developer-convenience options.

No objections to that.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] taking stdbool.h into use

2017-10-26 Thread Michael Paquier
On Thu, Oct 26, 2017 at 3:48 PM, Alvaro Herrera  wrote:
> Right, exactly.  But my point is that with the whole patch series
> applied I didn't get any warnings.

Sorry, I misread your message. You use Linux I suppose, what's your compiler?
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] proposal: custom autovacuum entries

2017-10-26 Thread Michael Paquier
On Thu, Oct 26, 2017 at 1:47 PM, Tom Lane  wrote:
> Jordan Deitch  writes:
>> I would like to remove records from a time series table older than a
>> certain age. My understanding of current standard practice is to configure
>> an external scheduler like cron to perform the deletion.
>> My proposal is to allow clients to register functions on the tail end of
>> the autovacuum sequence.
>
> There's already a mechanism for writing custom background workers.
> I doubt that adding random user-defined stuff to what autovacuum has
> to do is a better idea; that will mostly make autovac scheduling even
> harder than it is now.

I agree with Tom here, there is no point to have more facilities in
autovacuum so as it does extra work at its tail. There may be not even
any need to develop your own background worker. Have you looked at
pg_cron [1]?

[1]: https://github.com/citusdata/pg_cron
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] taking stdbool.h into use

2017-10-26 Thread Michael Paquier
On Thu, Oct 26, 2017 at 10:51 AM, Alvaro Herrera
 wrote:
> I gave this a quick run, to see if my compiler would complain for things
> like this:
>
>boolisprimary = flags & INDEX_CREATE_IS_PRIMARY;
>
> (taken from the first patch at
> https://postgr.es/m/20171023161503.ohkybquxrlech7d7@alvherre.pgsql )
>
> which is assigning a value other than 1/0 to a bool variable without an
> explicit cast.  I thought it would provoke a warning, but it does not.
> Is that expected?  Is my compiler too old/new?

It seems to me that this proves the point of the proposed patch. You
had better use a zero-equality comparison for such bitwise operation,
and so you ought to do that:
boolisprimary = (flags & INDEX_CREATE_IS_PRIMARY) != 0;
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [BUGS] Bug in Physical Replication Slots (at least 9.5)?

2017-10-26 Thread Michael Paquier
On Thu, Oct 26, 2017 at 3:05 AM, Kyotaro HORIGUCHI
 wrote:
> The largest obstacle to do that is that walreceiver is not
> utterly concerned to record internals. In other words, it doesn't
> know what a record is. Teaching that introduces much complexity
> and the complexity slows down the walreceiver.
>
> Addition to that, this "problem" occurs also on replication
> without a slot. The latest patch also help the case.

That's why replication slots have been introduced to begin with. The
WAL receiver gives no guarantee that a segment will be retained or not
based on the beginning of a record. That's sad that the WAL receiver
does not track properly restart LSN and instead just uses the flush
LSN. I am beginning to think that a new message type used to report
the restart LSN when a replication slot is in use would just be a
better and more stable solution. I haven't looked at the
implementation details yet though.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Timeline ID in backup_label file

2017-10-26 Thread Michael Paquier
On Thu, Oct 26, 2017 at 5:50 AM, David Steele <da...@pgmasters.net> wrote:
> On 10/25/17 10:10 PM, Craig Ringer wrote:
>> On 26 October 2017 at 02:50, Michael Paquier <michael.paqu...@gmail.com> 
>> wrote:
>>>
>>> I would find interesting to add at the bottom of the backup_label file
>>> a new field called "START TIMELINE: %d" to put this information in a
>>> more understandable shape. Any opinions?
>>
>> Strong "yes" from me.
>
> +1

Thanks for the feedback. Attached is a patch to achieve so, I have
added as well a STOP TIMELINE field in the backup history file. Note
that START TIMELINE gets automatically into the backup history file.
Added a CF entry as well.
-- 
Michael


backup_label_tli.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Re: [COMMITTERS] pgsql: Process variadic arguments consistently in json functions

2017-10-26 Thread Michael Paquier
On Thu, Oct 26, 2017 at 5:18 AM, Andrew Dunstan  wrote:
> Argh! I see that in your v6 patch and I thought I'd caught all of it but
> apparently not for master and REL_10. I wonder how that happened?

I am fine to take the blame. Likely an M-c pushed in emacs..
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Re: [COMMITTERS] pgsql: Process variadic arguments consistently in json functions

2017-10-25 Thread Michael Paquier
On Wed, Oct 25, 2017 at 5:24 AM, Andrew Dunstan <and...@dunslane.net> wrote:
> Process variadic arguments consistently in json functions
>
> json_build_object and json_build_array and the jsonb equivalents did not
> correctly process explicit VARIADIC arguments. They are modified to use
> the new extract_variadic_args() utility function which abstracts away
> the details of the call method.
>
> Michael Paquier, reviewed by Tom Lane and Dmitry Dolgov.
>
> Backpatch to 9.5 for the jsonb fixes and 9.4 for the json fixes, as
> that's where they originated.

- * Copyright (c) 2014-2017, PostgreSQL Global Development Group
+ * COPYRIGHT (c) 2014-2017, PostgreSQL Global Development Group
Andrew, I have just noticed that this noise diff has crept in. You may
want to fix that.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Implementing pg_receivewal --no-sync

2017-10-25 Thread Michael Paquier
On Tue, Oct 24, 2017 at 11:05 PM, Kuntal Ghosh
 wrote:
> +   
> +By default, pg_receivewal flushes a WAL segment's
> +contents each time a feedback message is sent to the server depending
> +on the interval of time defined by
> +--status-interval.
> IMHO, it's okay to remove the part 'depending on
> the.--status-interval'.

This sentence is actually wrong, a feedback message is never sent with
the feedback message. You need to use either --synchronous or --slot
for that, and the docs are already clear on the matter.

> +This option causes
> +pg_receivewal to not issue such flushes waiting,
> Did you mean 'to not issue such flush waitings'?

By reading again the patch, "waiting" should not be here. I have
reworded the documentation completely anyway. Hopefully it is more
simple now.

> + [ 'pg_receivewal', '-D', $stream_dir, '--synchronous', '--no-sync' ],
> + 'failure if --synchronous specified without --no-sync');
> s/without/with

Right.
-- 
Michael


pg_receivewal_nosync_v2.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Timeline ID in backup_label file

2017-10-25 Thread Michael Paquier
Hi all,

Lately, in order to extract some information from a backup_label file
in python I have found myself doing something like the following to
get a timeline number (feel free to reuse that code, especially the
regex pattern):
pattern = re.compile('^START WAL
LOCATION:.*([0-9A-F]+\/[0-9A-F]+).*\(file ([0-9A-F]+)\)')
if pattern.match(backup_lable_line):
current_lsn = m.group(1)
current_segment = m.group(2)
current_tli = str(int(current_segment[:8], 16))

That's more or less similar to what read_backup_label()@xlog.c does
using sscanf(), still I keep wondering why we need to go through this
much complication to get the origin timeline number of a base backup,
information which is IMO as important as the start LSN when working on
backup methods.

I would find interesting to add at the bottom of the backup_label file
a new field called "START TIMELINE: %d" to put this information in a
more understandable shape. Any opinions?
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Re: [BUGS] BUG #14849: jsonb_build_object doesn't like VARIADIC calls very much

2017-10-25 Thread Michael Paquier
On Mon, Oct 23, 2017 at 6:50 AM, Michael Paquier
<michael.paqu...@gmail.com> wrote:
> Okay, attached is what I think a fully implemented patch should look
> like. On top of what Andrew has done, I added and reworked the
> following:
> - removed duplicate error handling.
> - documented the function in funcapi.h and funcapi.c.
> - Added a new section in funcapi.h to outline that this is for support
> of VARIADIC inputs.
> I have added a commit message as well. Hope this helps.

For the sake of the archives, the introduction of
extract_variadic_args is committed with f3c6e8a2, and the JSON fixes
with 18fc4ecf. Thanks Andrew for the commit, and thanks Tom, Andrew
and Dmitry for the reviews.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] CurTransactionContext freed before transaction COMMIT ???

2017-10-25 Thread Michael Paquier
On Wed, Oct 25, 2017 at 7:34 AM, Gaddam Sai Ram
 wrote:
> Thanks for the response,
>
> Can you check if CurTransactionContext is valid at that point?
>
>
> Any Postgres function to check if CurTransactionContext is valid or not?

Assert(MemoryContextIsValid(CurTransactionContext));
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Remove secondary checkpoint

2017-10-24 Thread Michael Paquier
On Tue, Oct 24, 2017 at 7:24 PM, Tsunakawa, Takayuki
 wrote:
> (3)
> Should we change the default value of max_wal_size from 1 GB to a smaller 
> size?  I vote for "no" for performance.

The default has just changed in v10, so changing it again could be
confusing, so I agree with your position.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Remove secondary checkpoint

2017-10-24 Thread Michael Paquier
On Tue, Oct 24, 2017 at 5:58 PM, Tsunakawa, Takayuki
 wrote:
> If the latest checkpoint record is unreadable (the WAL segment/block/record 
> is corrupt?), recovery from the previous checkpoint would also stop at the 
> latest checkpoint.  And we don't need to replay the WAL records between the 
> previous checkpoint and the latest one, because their changes are already 
> persisted when the latest checkpoint was taken.  So, the user should just do 
> pg_resetxlog and start the database server when the recovery cannot find the 
> latest checkpoint record and PANICs?

Not necessarily. If a failure is detected when reading the last
checkpoint, as you say recovery would begin at the checkpoint prior
that and would stop when reading the record of last checkpoint, still
one could use a recovery.conf with restore_command to fetch segments
from a different source, like a static archive, as only the local
segment may be corrupted.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Implementing pg_receivewal --no-sync

2017-10-24 Thread Michael Paquier
Hi all,

After thinking a bit on the subject, I have decided to submit a patch
to do $subject. This makes pg_receivewal more consistent with
pg_basebackup. This option is mainly useful for testing, something
that becomes way more doable since support for --endpos has been
added.

Unsurprisingly, --synchronous and --no-sync are incompatible options.
Thanks,
-- 
Michael


pg_receivewal_nosync.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] per-sesson errors after interrupting CLUSTER pg_attrdef

2017-10-24 Thread Michael Paquier
On Fri, Oct 20, 2017 at 9:01 AM, Justin Pryzby  wrote:
> This was briefly scary but seems to have been limited to my psql session (no
> other errors logged).  Issue with catcache (?)
>
> I realized that the backup job I'd kicked off was precluding the CLUSTER from
> running, but that CLUSTER was still holding lock and stalling everything else
> under the sun.
>
> ts=# \df qci_add*
> ERROR:  could not read block 8 in file "base/16400/999225102": read only 0 of 
> 8192 bytes
> ts=# \dt+ pg_att
>
> ts=# \dt+ pg_attrdef
> ERROR:  could not read block 8 in file "base/16400/999225102": read only 0 of 
> 8192 bytes

Perhaps that's too late, but to which relation is this relfilenode
actually referring to?
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Remove secondary checkpoint

2017-10-24 Thread Michael Paquier
On Mon, Oct 23, 2017 at 11:20 PM, Simon Riggs  wrote:
> Remove the code that maintained two checkpoint's WAL files and all
> associated stuff.
>
> Try to avoid breaking anything else
>
> This
> * reduces disk space requirements on master
> * removes a minor bug in fast failover
> * simplifies code

In short, yeah! I had a close look at the patch and noticed a couple of issues.

+else
 /*
- * The last valid checkpoint record required for a streaming
- * recovery exists in neither standby nor the primary.
+ * We used to attempt to go back to a secondary checkpoint
+ * record here, but only when not in standby_mode. We now
+ * just fail if we can't read the last checkpoint because
+ * this allows us to simplify processing around checkpoints.
  */
 ereport(PANIC,
 (errmsg("could not locate a valid checkpoint record")));
-}
Using brackets in this case is more elegant style IMO. OK, there is
one line, but the comment is long so the block becomes
confusingly-shaped.

 /* Version identifier for this pg_control format */
-#define PG_CONTROL_VERSION1003
+#define PG_CONTROL_VERSION1100
Yes, this had to happen anyway in this release cycle.

backup.sgml describes the following:
to higher segment numbers.  It's assumed that segment files whose
contents precede the checkpoint-before-last are no longer of
interest and can be recycled.
However this is not true anymore with this patch.

The documentation of pg_control_checkpoint() is not updated. func.sgml
lists the tuple fields returned for this function.

You forgot to update pg_control_checkpoint in pg_proc.h. prior_lsn is
still listed in the list of arguments returned. And you need to update
as well the output list of types.

  * whichChkpt identifies the checkpoint (merely for reporting purposes).
- * 1 for "primary", 2 for "secondary", 0 for "other" (backup_label)
+ * 1 for "primary", 0 for "other" (backup_label)
+ * 2 for "secondary" is no longer supported
  */
I would suggest to just remove the reference to "secondary".

-* Delete old log files (those no longer needed even for previous
-* checkpoint or the standbys in XLOG streaming).
+* Delete old log files and recycle them
 */
Here that's more "Delete or recycle old log files". Recycling of a WAL
segment is its renaming into a newer segment.

You forgot to update this formula in xlog.c:
distance = (2.0 + CheckPointCompletionTarget) * CheckPointDistanceEstimate;
/* add 10% for good measure. */
distance *= 1.10;
You can guess easily where the mistake is.

-   checkPointLoc = ControlFile->prevCheckPoint;
+   /*
+* It appears to be a bug that we used to use
prevCheckpoint here
+*/
+   checkPointLoc = ControlFile->checkPoint;
Er, no. This is correct because we expect the prior checkpoint to
still be present in the event of a failure detecting the latest
checkpoint.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Re: [BUGS] BUG #14849: jsonb_build_object doesn't like VARIADIC calls very much

2017-10-23 Thread Michael Paquier
On Mon, Oct 23, 2017 at 7:03 AM, Michael Paquier
<michael.paqu...@gmail.com> wrote:
>> Looks good otherwise.
>
> My set of diffs for funcapi.h are actually that:
>   * funcapi.h
>   *   Definitions for functions which return composite type and/or sets
> + *   or work on VARIADIC inputs.
> [...]
> +/*--
> + * Support to ease writing of functions dealing with VARIADIC inputs
> + *--
> + *
> + * This function extracts a set of argument values, types and NULL markers
> + * for a given input function. This returns a set of data:
> + * - **values includes the set of Datum values extracted.
> + * - **types the data type OID for each element.
> + * - **nulls tracks if an element is NULL.
> + *
> + * convert_unknown set to true enforces conversion of unknown input type
> + * unknown to text.
> + * variadic_start tracks the argument number of the function call where the
> + * VARIADIC set of arguments begins.
> + *
> + * The return result is the number of elements stored. In the event of a
> + * NULL input, then the caller of this function ought to generate a NULL
> + * object as final result, and in this case a result value of -1 is used
> + * to be able to make the difference between an empty array or object.
> + */
> +extern int extract_variadic_args(FunctionCallInfo fcinfo, int variadic_start,
> +bool convert_unknown, Datum **values,
> Oid **types,
> +bool **nulls);
>
> Got this bit as well:
>   * funcapi.c
>   *   Utility and convenience functions for fmgr functions that return
> - *   sets and/or composite types.
> + *   sets and/or composite types, or deal with VARIADIC inputs.
>   *

Okay, attached is what I think a fully implemented patch should look
like. On top of what Andrew has done, I added and reworked the
following:
- removed duplicate error handling.
- documented the function in funcapi.h and funcapi.c.
- Added a new section in funcapi.h to outline that this is for support
of VARIADIC inputs.
I have added a commit message as well. Hope this helps.

format, concat and potentially count_nulls could take advantage of
this new function, though refactoring is left for later. I am fine to
do the legwork on a different thread. Changing count_nulls would also
switch it to a o(n^2),  which is not cool either, so I think that it
could be left out. Still let's discuss that on another thread.
-- 
Michael


json_variadic_v6.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Re: [BUGS] BUG #14849: jsonb_build_object doesn't like VARIADIC calls very much

2017-10-22 Thread Michael Paquier
On Mon, Oct 23, 2017 at 6:11 AM, Tom Lane  wrote:
> This comment is neither correct nor intelligible:
>
> /* important for value, key cannot being NULL */
>
> I'd say just drop it.

Yep.

> The checks for "could not determine data type" errors seem
> rather duplicative, too.

Yep.

> 
> The extern declaration seems to have been dropped in a rather
> random place in the .h file.
> 

funcapi.h/c are nicely documented. I think that more is needed.

> Looks good otherwise.

My set of diffs for funcapi.h are actually that:
  * funcapi.h
  *   Definitions for functions which return composite type and/or sets
+ *   or work on VARIADIC inputs.
[...]
+/*--
+ * Support to ease writing of functions dealing with VARIADIC inputs
+ *--
+ *
+ * This function extracts a set of argument values, types and NULL markers
+ * for a given input function. This returns a set of data:
+ * - **values includes the set of Datum values extracted.
+ * - **types the data type OID for each element.
+ * - **nulls tracks if an element is NULL.
+ *
+ * convert_unknown set to true enforces conversion of unknown input type
+ * unknown to text.
+ * variadic_start tracks the argument number of the function call where the
+ * VARIADIC set of arguments begins.
+ *
+ * The return result is the number of elements stored. In the event of a
+ * NULL input, then the caller of this function ought to generate a NULL
+ * object as final result, and in this case a result value of -1 is used
+ * to be able to make the difference between an empty array or object.
+ */
+extern int extract_variadic_args(FunctionCallInfo fcinfo, int variadic_start,
+bool convert_unknown, Datum **values,
Oid **types,
+bool **nulls);

Got this bit as well:
  * funcapi.c
  *   Utility and convenience functions for fmgr functions that return
- *   sets and/or composite types.
+ *   sets and/or composite types, or deal with VARIADIC inputs.
  *
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Re: [BUGS] BUG #14849: jsonb_build_object doesn't like VARIADIC calls very much

2017-10-22 Thread Michael Paquier
On Mon, Oct 23, 2017 at 1:44 AM, Andrew Dunstan
<andrew.duns...@2ndquadrant.com> wrote:
>
>
> On 10/22/2017 12:11 PM, Andrew Dunstan wrote:
>>
>> On 10/21/2017 07:33 PM, Michael Paquier wrote:
>>> On Sun, Oct 22, 2017 at 1:43 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>>> I don't think collecting all the arguments is particularly special ---
>>>> format() or concat() for instance could possibly use this.  You might
>>>> need an option to say what to do with unknown.
>>> In this case, we could just use a boolean flag to decide if TEXTOID
>>> should be enforced or not.
>> A generic function is going to look a little more complicated than this,
>> though. The functions as coded assume that the function has a single
>> variadic argument. But for it to be useful generically it really needs
>> to be able to work where there are both fixed and variadic arguments (a
>> la printf style functions).
>>
>> I guess a simple way would be to make the caller tell the function where
>> the variadic arguments start, or how many to skip, something like that.

Sorry for the late reply, I was taking a long flight. Yes, that's
actually the conclusion I am reaching when looking at the code by
adding an argument to mark when the variadic arguments start. The
headers of funcapi.h and funcapi.c need also a cleanup.

> here's a patch that works that way, based on Michael's code.

Patch not attached :)
I still have a patch half-cooked, that I can send if necessary, but
you are on it, right?
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] 64-bit queryId?

2017-10-18 Thread Michael Paquier
On Thu, Oct 19, 2017 at 4:12 AM, Robert Haas  wrote:
> On Wed, Oct 18, 2017 at 9:20 AM, Julien Rouhaud  wrote:
>> Sorry for replying so late, but I have a perhaps naive question about
>> the hashtable handling with this new version.
>>
>> IIUC, the shared hash table is now created with HASH_BLOBS instead of
>> HASH_FUNCTION, so since sizeof(pgssHashKey) != sizeof(uint32) the hash
>> table will use tag_hash() to compute the hash key.
>>
>> tag_hash() uses all the bits present in the given struct, so this can
>> be problematic if padding bits are not zeroed, which isn't garanted by
>> C standard for local variable.
>>
>> WIth current pgssHashKey definition, there shouldn't be padding bits,
>> so it should be safe.  But I wonder if adding an explicit memset() of
>> the key in pgss_store() could avoid extension authors to have
>> duplicate entries if they rely on this code, or prevent future issue
>> in the unlikely case of adding other fields to pgssHashKey.
>
> I guess we should probably add additional comment to the definition of
> pgssHashKey warning of the danger.  I'm OK with adding a memset if
> somebody can promise me it will get optimized away by all reasonably
> commonly-used compilers, but I'm not that keen on adding more cycles
> to protect against a hypothetical danger.

A comment is an adapted answer for me too. There is no guarantee that
memset improvements will get committed. They will likely be, but
making a hard promise is difficult.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Re: Is anything preventing us from allowing write to foreign tables from standby?

2017-10-17 Thread Michael Paquier
On Wed, Oct 18, 2017 at 9:14 AM, Craig Ringer  wrote:
> Superficially at least, it sounds like a good idea.

Indeed.

> We should only need a virtual xid when we're working with foreign
> tables since we don't do any local heap changes.
>
> How's it work with savepoints?

That's one thing to worry about.

At least to me, it feels like cheating to allow an INSERT query to
happen for a transaction which is read-only actually read-only because
XactReadOnly is set to true when the transaction starts. I am
wondering if we should extend BEGIN TRANSACTION with a sort of "WRITE
ONLY FOREIGN" mode, which allows read queries as well as write queries
for foreign tables, because we know that those will not generate WAL
locally. This way it would be possible to block as well INSERT queries
happening in a transaction which should be intrinsically read-only.

+ if (rte->relkind == 'f')
+ continue;
Better to use RELKIND_FOREIGN_TABLE here.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [PATCH] Add recovery_min_apply_delay_reconnect recovery option

2017-10-17 Thread Michael Paquier
On Tue, Oct 17, 2017 at 10:40 PM, Eric Radman <ericsh...@eradman.com> wrote:
> On Tue, Oct 17, 2017 at 12:34:17PM +0900, Michael Paquier wrote:
> I thought I had observed cases where the WalReceiver was shut down
> without causing XLogCtl->recoveryWakeupLatch to return. If I'm wrong
> about this then there's no reason to spin every n seconds.

I would expect a patch to not move the timeout calculation within the
loop in recoveryApplyDelay() as you did.

> Which record are you suggesting should be forcibly "read again"?  The
> record identified by XLogCtl->replayEndRecPtr or
> XLogCtl->lastReplayedEndRecPtr?  I'll look more carefully at such an
> approach.

I have not looked at how to do that in details, but as the delay is
applied for commit WAL records, you would need to make the redo loop
look again at this same record once you have switched back to a
streaming state. Something to be careful about is that you should not
apply the same delay multiple times for the same record.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [PATCH] Add recovery_min_apply_delay_reconnect recovery option

2017-10-16 Thread Michael Paquier
On Tue, Oct 17, 2017 at 12:51 AM, Eric Radman  wrote:
> This administrative compromise is necessary because the WalReceiver is
> not resumed after a network interruption until all records are read,
> verified, and applied from the archive on disk.

Taking a step back here... recoveryApplyDelay() uses
XLogCtl->recoveryWakeupLatch which gets set if the WAL receiver has
received new WAL, or if the WAL receiver shuts down properly. So if
the WAL receiver gets down for whatever reason during the loop of
recoveryApplyDelay(), the startup process waits for a record to be
applied maybe for a long time, and as there is no WAL receiver we
actually don't receive any new WAL records. New WAL records would be
received only once WaitForWALToBecomeAvailable() is called, which
happens once the apply delay is done for. If the postmaster dies, then
HandleStartupProcInterrupts() would take care of taking down
immediately the startup process, which is cool.

I see what you are trying to achieve and that seems worth it. It is
indeed a waste to not have a WAL receiver online while waiting for a
delay to be applied. If there is a flacky network between the primary
and a standby, you may end up with a standby way behind its primary,
and that could penalize a primary clean shutdown as the primary waits
for the shutdown checkpoint record to be flushed on the standby.

I think that your way to deal with the problem is messy though. If you
think about it, no parameters are actually needed. What you should try
to achieve is to make recoveryApplyDelay() smarter, by making the wait
to forcibly stop if you detect a failure by getting out of the redo
routine, and then force again the record to be read again. This way,
the startup process would try to start again a new WAL receiver if it
thinks that the source it should read WAL from is a stream. That may
turn to be a patch more complicated than you think though.

Your patch also breaks actually the use case of standbys doing
recovery using only archives and no streaming. In this case
WalRcvStreaming returns false, and recovery_min_apply_delay_reconnect
would be used unconditionally, so you would break a lot of
applications silently.

> Is it possible to verify the archive on disk independently of
> application?  Adding a second delay parameter provides a workaround for
> some use cases without complecting xlog.c.

That's possible, not with core though. The archives are in a location
not controlled by the backend, but by archive_command, which may not
be local to the instance where Postgres is running. You could always
hack your own functions to do this work, here is an example of
something I came up with:
https://github.com/michaelpq/pg_plugins/tree/master/wal_utils
This prototype (use and hack at your own risk), is able to look at the
local contents of an archive. This can be used easily with a
client-side tool to copy a series of segments., or just perform sanity
checks on them.

For those reasons, -1 for the patch as proposed.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] BLK_DONE state in XLogReadBufferForRedoExtended

2017-10-16 Thread Michael Paquier
On Mon, Oct 16, 2017 at 9:50 PM, Amit Kapila  wrote:
> If above analysis is correct, then I think we can say that row state
> in a page will be same during recovery as it was when the original
> operation was performed if the full_page_writes are enabled. I am not
> sure how much this can help in current heap format, but this can help
> in zheap (undo based heap).

If I understood that correctly, that looks like a sane assumption. For
REGBUF_NO_IMAGE you may need to be careful though with undo
operations.

> In zheap, we are writing complete tuple for Delete operation in undo
> so that we can reclaim the corresponding tuple space as soon as the
> deleting transaction is committed.  Now, during recovery, we have to
> generate the complete undo record (which includes the entire tuple)
> and for that ideally, we should write the complete tuple in WAL, but
> instead of that, I think we can regenerate it from the original page.
> This is only applicable when full_page_writes are enabled, otherwise,
> a complete tuple is required in WAL.

Yeah, you should really try to support both modes as well.
Fortunately, it is possible to know if full page writes are enforced
at the moment a record is assembled and inserted, so you could rely on
that.

> I am not sure how much above makes sense to anyone without a detailed
> explanation, but I thought I should give some background on why I
> asked this question.  However, if anybody needs more explanation or
> sees any fault in above understanding, please let me know.

Thanks for clarifying, I was wondering the reason behind the question
as well. It is the second time that I see the word zheap on -hackers,
and the first time was no longer than 2 days ago by Robert.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] replace GrantObjectType with ObjectType

2017-10-16 Thread Michael Paquier
On Thu, Oct 12, 2017 at 5:43 PM, Alvaro Herrera <alvhe...@alvh.no-ip.org> wrote:
> Michael Paquier wrote:
>> On Thu, Oct 12, 2017 at 7:55 AM, Peter Eisentraut
>> <peter.eisentr...@2ndquadrant.com> wrote:
>> > It seems to me that having ACL_OBJECT_* symbols alongside OBJECT_*
>> > symbols is not useful and leads to duplication.  Digging around in the
>> > past suggests that we used to have a lot of these command-specific
>> > symbols but got rid of them over time, except that the ACL stuff was
>> > never touched.  The attached patch accomplishes that.
>
> +1 for this.
>
>> -bool
>> -EventTriggerSupportsGrantObjectType(GrantObjectType objtype)
>> -{
>> -   switch (objtype)
>> -   {
>> -   case ACL_OBJECT_DATABASE:
>> -   case ACL_OBJECT_TABLESPACE:
>> -   /* no support for global objects */
>> -   return false;
>> By removing that, if any GRANT/REVOKE support is added for another
>> object type, then EventTriggerSupportsObjectType would return true by
>> default instead of getting a compilation failure.
>
> Yeah, we've found it useful to remove default: clauses in some switch
> blocks so that we get compile warnings when we forget to add a new type
> (c.f. commit e84c0195980f).  Let's not add any more of those.

Here is an idea: let's keep EventTriggerSupportsGrantObjectType which
instead of ACL_OBJECT_* uses OBJECT_*, but complains with an error if
the object is not supported with GRANT. Not need for a default in this
case.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Still another race condition in recovery TAP tests

2017-10-16 Thread Michael Paquier
On Tue, Oct 17, 2017 at 5:01 AM, Robert Haas  wrote:
> On Fri, Oct 6, 2017 at 5:57 AM, Craig Ringer  wrote:
>>> Fewer people will test as we grow the list of modules they must first 
>>> install.
>> At worst, all we have to do is provide a script
>> that fetches them, from distro repos if possible, and failing that
>> from CPAN.
>>
>> With cpanminus, that's pretty darn simple too.
>
> I think the idea that this is going to work for everybody is not very
> realistic.  I am not going to run some random script to install stuff
> on my system in whatever manner it feels like, because when I do stuff
> like that I usually end up regretting it.  If it's a throw-away VM,
> sure, but not otherwise.

Yeah, having something which is network-dependent to run the full set
of regression tests is not a good idea. I have seen users, at least in
Japan, deploying Postgres on hosts that have no external connection,
just a base OS image built with all packages present. Data center
rules can be very strict.

> We don't even have good documentation of what packages you should
> install on various packaging systems to build the server, let alone
> all of the additional things that may be required depending on build
> options and TAP tests you might want to run.

On top of that, let's not forget that we take the time to improve the
modules what we already have in the code tree. Let's not forget that
;)
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] relkind check in DefineIndex

2017-10-16 Thread Michael Paquier
On Mon, Oct 16, 2017 at 2:56 PM, Amit Langote
 wrote:
> On 2017/10/14 4:32, Robert Haas wrote:
>> On Fri, Oct 13, 2017 at 12:38 PM, Alvaro Herrera
>>  wrote:
>>> The relkind check in DefineIndex has grown into an ugly rats nest of
>>> 'if' statements.  I propose to change it into a switch, as per the
>>> attached.
>>
>> wfm
>
> +1

+1. There is as well CreateTrigger(), analyze_rel() or
ATRewriteCatalogs() that do similar things but those are not using
multiple layers of checks.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [BUGS] Bug in Physical Replication Slots (at least 9.5)?

2017-10-16 Thread Michael Paquier
On Thu, Sep 7, 2017 at 12:33 PM, Kyotaro HORIGUCHI
 wrote:
> At Wed, 6 Sep 2017 12:23:53 -0700, Andres Freund  wrote 
> in <20170906192353.ufp2dq7wm5fd6...@alap3.anarazel.de>
>> I'm not following. All we need to use is the beginning of the relevant
>> records, that's easy enough to keep track of. We don't need to read the
>> WAL or anything.
>
> The beginning is already tracked and nothing more to do.

I have finally allocated time to look at your newly-proposed patch,
sorry for the time it took. Patch 0002 forgot to include sys/stat.h to
allow the debugging tool to compile :)

> The first *problem* was WaitForWALToBecomeAvailable requests the
> beginning of a record, which is not on the page the function has
> been told to fetch. Still tliRecPtr is required to determine the
> TLI to request, it should request RecPtr to be streamed.

[...]

> The rest to do is let XLogPageRead retry other sources
> immediately. To do this I made ValidXLogPageHeader@xlogreader.c
> public (and renamed to XLogReaderValidatePageHeader).
>
> The patch attached fixes the problem and passes recovery
> tests. However, the test for this problem is not added. It needs
> to go to the last page in a segment then put a record continues
> to the next segment, then kill the standby after receiving the
> previous segment but before receiving the whole record.

+typedef struct XLogPageHeaderData *XLogPageHeader;
[...]
+/* Validate a page */
+extern bool XLogReaderValidatePageHeader(XLogReaderState *state,
+   XLogRecPtr recptr, XLogPageHeader hdr);
Instead of exposing XLogPageHeaderData, I would recommend just using
char* and remove this declaration. The comment on top of
XLogReaderValidatePageHeader needs to make clear what caller should
provide.

+if (!XLogReaderValidatePageHeader(xlogreader, targetPagePtr,
+  (XLogPageHeader) readBuf))
+goto next_record_is_invalid;
+
[...]
-ptr = tliRecPtr;
+ptr = RecPtr;
 tli = tliOfPointInHistory(tliRecPtr, expectedTLEs);

 if (curFileTLI > 0 && tli < curFileTLI)
The core of the patch is here (the patch has no comment so it is hard
to understand what's the point of what is being done), and if I
understand that correctly, you allow the receiver to fetch the
portions of a record spawned across multiple segments from different
sources, and I am not sure that we'd want to break that promise.
Shouldn't we instead have the receiver side track the beginning of the
record and send that position for the physical slot's restart_lsn?
This way the receiver would retain WAL segments from the real
beginning of a record. restart_lsn for replication slots is set when
processing the standby message in ProcessStandbyReplyMessage() using
now the flush LSN, so a more correct value should be provided using
that. Andres, what's your take on the matter?
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] pg_control_recovery() return value when not in recovery

2017-10-14 Thread Michael Paquier
On Sat, Oct 14, 2017 at 8:31 AM, Joe Conway  wrote:
> Sorry for the slow response, but thinking back on this now, the idea of
> these functions, in my mind at least, was to provide as close to the
> same output as possible to what pg_controldata outputs. So:
>
> # pg_controldata
> ...
> Minimum recovery ending location: 0/0
> Min recovery ending loc's timeline:   0
> Backup start location:0/0
> Backup end location:  0/0
> End-of-backup record required:no
> ...
>
> So if we make a change here, do we also change pg_controldata?

For a lot of folks on this list, it is clear that things like
InvalidXLogRecPtr map to 0/0, but what of end-users? Couldn't we
consider marking those fields as "undefined" for example. "invalid"
would mean that the state of the cluster is incorrect, so I am not
sure if that is most adapted.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] show precise repos version for dev builds?

2017-10-13 Thread Michael Paquier
On Sat, Oct 14, 2017 at 4:47 AM, Robert Haas  wrote:
> Mmph.  I understand the desire to identify the exact commit used for a
> build somehow, but something whose output depends on whether or not I
> left a branch lying around locally doesn't seem that great.

Similarly to Peter, I prefer a minimum amount of information so I tend
to just use `git rev-parse --short HEAD` with --extra-version for my
own builds. Looking at the timestamp of the files installed is enough
to know when you worked on them, and when testing a patch and
committing it on a local branch before compiling you can know easily
where you left things off. git branch --contains is also useful to get
from which branch is commit from.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] BLK_DONE state in XLogReadBufferForRedoExtended

2017-10-12 Thread Michael Paquier
On Thu, Oct 12, 2017 at 10:47 PM, Amit Kapila  wrote:
> Today, I was trying to think about cases when we can return BLK_DONE
> in XLogReadBufferForRedoExtended.  One thing that occurred to me is
> that it can happen during the replay of WAL if the full_page_writes is
> off.  Basically, when the full_page_writes is on, then during crash
> recovery, it will always first restore the full page image of page and
> then apply the WAL corresponding to that page, so we will never hit
> the case where the lsn of the page is greater than lsn of WAL record.
>
> Are there other cases for which we can get BLK_DONE state?  Is it
> mentioned somewhere in code/comments which I am missing?

Remember the thread about meta page optimization... Some index AMs use
XLogInitBufferForRedo() to redo their meta pages and those don't have
a FPW so when doing crash recovery you may finish by not replaying a
meta page if its LSN on the page header is newer than what's being
replayed. That's another case where BLK_DONE can be reached, even if
full_page_writes is on.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] replace GrantObjectType with ObjectType

2017-10-12 Thread Michael Paquier
On Thu, Oct 12, 2017 at 7:55 AM, Peter Eisentraut
 wrote:
> It seems to me that having ACL_OBJECT_* symbols alongside OBJECT_*
> symbols is not useful and leads to duplication.  Digging around in the
> past suggests that we used to have a lot of these command-specific
> symbols but got rid of them over time, except that the ACL stuff was
> never touched.  The attached patch accomplishes that.

That's always welcome:
 14 files changed, 203 insertions(+), 254 deletions(-)

This needs an update:
$ git grep GrantObjectType
src/tools/pgindent/typedefs.list:GrantObjectType

-static const char *stringify_grantobjtype(GrantObjectType objtype);
-static const char *stringify_adefprivs_objtype(GrantObjectType objtype);
+static const char *stringify_grantobjtype(ObjectType objtype);
+static const char *stringify_adefprivs_objtype(ObjectType objtype);
stringify_grantobjtype should be renamed to stringify_objtype.

-bool
-EventTriggerSupportsGrantObjectType(GrantObjectType objtype)
-{
-   switch (objtype)
-   {
-   case ACL_OBJECT_DATABASE:
-   case ACL_OBJECT_TABLESPACE:
-   /* no support for global objects */
-   return false;
By removing that, if any GRANT/REVOKE support is added for another
object type, then EventTriggerSupportsObjectType would return true by
default instead of getting a compilation failure. I think that a
comment would be appropriate here:
GrantStmt  *stmt = (GrantStmt *) parsetree;

-   if (EventTriggerSupportsGrantObjectType(stmt->objtype))
+   if (EventTriggerSupportsObjectType(stmt->objtype))
ProcessUtilitySlow(pstate, pstmt, queryString,
Something like, "This checks for more object types than currently
supported by the GRANT statement"... Or at least something to outline
that risk.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


  1   2   3   4   5   6   7   8   9   10   >