Re: [HACKERS] Variable substitution in psql backtick expansion
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
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
On Mon, Nov 13, 2017 at 7:37 AM, Noah Misch wrote: > On Fri, Nov 03, 2017 at 11:10:14AM +0000, 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
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
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
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
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
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
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?
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
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
On Fri, Nov 10, 2017 at 2:32 AM, Fabrízio de Royes Mello wrote: > On Thu, Nov 9, 2017 at 12:09 AM, Michael Paquier > 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
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()
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
On Thu, Nov 9, 2017 at 2:42 AM, Fabrízio de Royes Mello
wrote:
> On Wed, Nov 8, 2017 at 12:47 AM, Michael Paquier
> 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
On Thu, Nov 9, 2017 at 6:05 AM, Tom Lane wrote: > Michael Paquier writes: >> On Tue, Sep 26, 2017 at 11:42 AM, Vaishnavi Prabakaran >> 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
On Thu, Nov 9, 2017 at 1:46 AM, Peter Eisentraut 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(&btree->itemptr, iptr) > 0) ? TRUE : >> FALSE; >> + return (ginCompareItemPointers(&btree->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
On Thu, Nov 9, 2017 at 1:03 AM, Peter Eisentraut wrote: > On 11/7/17 19:58, Michael Paquier wrote: >> 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. > > 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
On Tue, Nov 7, 2017 at 9:58 PM, Fabrízio de Royes Mello
wrote:
> On Tue, Nov 7, 2017 at 1:15 AM, Michael Paquier
> wrote:
>> On Sun, Nov 5, 2017 at 3:14 AM, Fabrízio de Royes Mello
>> 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
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
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
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
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
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
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
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
On Wed, Nov 8, 2017 at 2:23 AM, Simon Riggs wrote: > On 31 October 2017 at 12:01, Michael Paquier > 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()
On Wed, Nov 8, 2017 at 2:26 AM, Jacob Champion wrote: > On Mon, Nov 6, 2017 at 6:18 PM, Michael Paquier > 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
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
On Sun, Nov 5, 2017 at 3:14 AM, Fabrízio de Royes Mello wrote: > On Sat, Nov 4, 2017 at 1:23 AM, Michael Paquier > wrote: >> 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. >> > > 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()
On Tue, Nov 7, 2017 at 7:27 AM, Asim Praveen wrote: > On Mon, Oct 2, 2017 at 6:48 PM, Michael Paquier > 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
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
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
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
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
On Fri, Nov 3, 2017 at 12:07 PM, Michael Paquier 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?
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
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
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
On Wed, Nov 1, 2017 at 12:07 AM, Tsunakawa, Takayuki wrote: > From: Michael Paquier [mailto:michael.paqu...@gmail.com] >> On Tue, Oct 31, 2017 at 6:59 AM, Tsunakawa, Takayuki >> 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
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
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
On Thu, Nov 2, 2017 at 4:47 PM, Peter Eisentraut 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
On Thu, Nov 2, 2017 at 2:05 AM, Peter Eisentraut wrote: > On 11/1/17 10:29, Michael Paquier wrote: >> 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. > > 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
On Wed, Nov 1, 2017 at 9:04 AM, Michael Paquier 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
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
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
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
On Tue, Oct 31, 2017 at 6:59 AM, Tsunakawa, Takayuki 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
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
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
On Mon, Oct 30, 2017 at 2:22 PM, Simon Riggs wrote: > On 25 October 2017 at 00:17, Michael Paquier > 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
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
On Mon, Oct 30, 2017 at 11:07 AM, Alvaro Herrera 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
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
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
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
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
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
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(&btree->itemptr, iptr) > 0) ? TRUE : FALSE; + return (ginCompareItemPointers(&btree->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
On Sun, Oct 29, 2017 at 12:31 AM, Robert Haas wrote: > On Sun, Oct 29, 2017 at 3:42 AM, Michael Paquier > 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
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
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
On Fri, Oct 27, 2017 at 12:03 AM, Michael Paquier wrote: > On Thu, Oct 26, 2017 at 10:46 PM, Robert Haas wrote: >> On Wed, Oct 25, 2017 at 10:03 PM, Michael Paquier >> 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
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
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
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
On Thu, Oct 26, 2017 at 10:46 PM, Robert Haas wrote: > On Wed, Oct 25, 2017 at 10:03 PM, Michael Paquier > 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
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
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
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)?
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
On Thu, Oct 26, 2017 at 5:50 AM, David Steele wrote: > On 10/25/17 10:10 PM, Craig Ringer wrote: >> On 26 October 2017 at 02:50, Michael Paquier >> 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
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
On Wed, Oct 25, 2017 at 5:24 AM, Andrew Dunstan 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
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
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
On Mon, Oct 23, 2017 at 6:50 AM, Michael Paquier 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 ???
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
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
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
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
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
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
On Mon, Oct 23, 2017 at 7:03 AM, Michael Paquier 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
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
On Mon, Oct 23, 2017 at 1:44 AM, Andrew Dunstan 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 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?
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?
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
On Tue, Oct 17, 2017 at 10:40 PM, Eric Radman 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
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
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
On Thu, Oct 12, 2017 at 5:43 PM, Alvaro Herrera wrote:
> Michael Paquier wrote:
>> 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.
>
> +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
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
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)?
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
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?
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
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
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
