Re: [HACKERS] [pgsql-www] Schedule for migration to pglister

[Magnus Hagander]

On Mon, Nov 6, 2017 at 5:00 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Magnus Hagander <mag...@hagander.net> writes:
> > On Mon, Nov 6, 2017 at 4:46 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> >> Hm, around here it's no match -> spam bucket.  But in any case, why
>
> > I think you're quite uncommon in that setup.
>
> Interesting, because "it's not addressed to me (or any list I'm on)"
> is the best single spam filtering rule I know, and has been for a
> decade or two.
>

Oh, I think I misunderstood you. I thought you meant "any list tagged email
that's for a list I don't know what it is". As in "list-id exists but is
unknown".

The way you explain there makes a lot more sense. I think not many people
do that either, mainly since gmail/yahoo/whatnot doesn't make it very easy
to do that. But it does make a lot more sense that way.


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

Re: [HACKERS] [pgsql-www] Schedule for migration to pglister

[Magnus Hagander]

On Mon, Nov 6, 2017 at 4:46 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Magnus Hagander <mag...@hagander.net> writes:
> > On Mon, Nov 6, 2017 at 4:40 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> >> I suggest doing that the other way 'round.  Otherwise, the email
> >> about the change will inevitably go into a lot of peoples' bit
> >> buckets if they haven't adjusted their mail filters yet.
>
> > The argument for doing it after the migration is that the complaints that
> > we have received so far have all been from people where email ends up in
> > the *inbox* after the migration, not the bitbucket. That's the default
> > action in most peoples MUAs when their rules no longer match...
>
> Hm, around here it's no match -> spam bucket.  But in any case, why
>

I think you're quite uncommon in that setup. For obvious reasons, but I've
never heard of anybody other than you doing that :)



> would you not want to send it before so that it would end up where
> they're accustomed to seeing the list's traffic?
>

The experience from the pgadmin lists is that a lot of people have the
lists filtered into folders that they don't check often (or at all). So
they don't notice the migraiton message. But they start noticing once all
the list mail shows up in their inbox instead.

It might well be that we end up getting the other half of people when we do
it this order, but we definitely at a *lot* of people in that first bucket.

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

Re: [HACKERS] [pgsql-www] Schedule for migration to pglister

[Magnus Hagander]

On Mon, Nov 6, 2017 at 4:40 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Stephen Frost <sfr...@snowman.net> writes:
> > Each list will receive an email with a link to the wiki about the
> > migration after the list has been migrated.
>
> I suggest doing that the other way 'round.  Otherwise, the email
> about the change will inevitably go into a lot of peoples' bit
> buckets if they haven't adjusted their mail filters yet.
>

The argument for doing it after the migration is that the complaints that
we have received so far have all been from people where email ends up in
the *inbox* after the migration, not the bitbucket. That's the default
action in most peoples MUAs when their rules no longer match...

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

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

[Magnus Hagander]

On Sat, Nov 4, 2017 at 4:04 AM, Michael Paquier <michael.paqu...@gmail.com>
wrote:

> On Fri, Nov 3, 2017 at 4:04 PM, Petr Jelinek
> <petr.jeli...@2ndquadrant.com> 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.
>

pg_internal.init can, and do, appear in multiple different directories.
pg_control is always in the same place. So they're not the same thing.

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.

It also seems the list in the documentation is not in sync with the code.
AFAICT docs are not mentioning the current_logfile. This seems to be a miss
in 19dc233c32f ?

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

Re: [HACKERS] Minor comment issue in receivelog.c

[Magnus Hagander]

On Thu, Nov 2, 2017 at 5:18 PM, Bernd Helmle <maili...@oopsware.de> wrote:

> Please find a minor comment fix for receivelog.c, HandleCopyStream().
>
> The comments talks about a START_STREAMING command, but i think
> START_REPLICATION is what's meant here.
>

Yeah, it is. Confusingly enough, START_STREAMING is what's returned by
START_REPLICATION.

Applied, thanks.

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

Re: [HACKERS] Comment typo

[Magnus Hagander]

On Mon, Oct 30, 2017 at 3:49 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp>
wrote:

> Here is a patch to fix a typo in a comment in partition.c:
> s/specificiation/specification/.
>

Applied, thanks.

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

Re: [HACKERS] git down

[Magnus Hagander]

On Mon, Oct 30, 2017 at 2:35 AM, Andreas Karlsson <andr...@proxel.se> wrote:

> On 10/27/2017 10:51 PM, Erik Rijkers wrote:
>
>> git.postgresql.org is down/unreachable
>>
>> ( git://git.postgresql.org/git/postgresql.git )
>>
>
> Yes, I noticed this too, but https://git.postgresql.org/git/postgresql.git
> still works fine.
>
> I guess it makes sense to remove unencrypted access, but in that case
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=summary should not
> advertise supporting the git protocol. I have not seen any announcement
> either, but that could just be me not paying enough attention.


We definitely still support the unencrypted git protocol. I do recommend
using https, but that doesn't mean git shouldn't work. There seems to have
been some network issues and the git daemon doesn't do a very good job of
handling hung sessions. I've cleaned up for now and it seems to be working
again, and we'll do some more digging into what actually was the root
cause.


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

Re: [HACKERS] stalled post to mailing list - wrong filter?

[Magnus Hagander]

On Sun, Oct 22, 2017 at 9:02 AM, Pavel Stehule <pavel.steh...@gmail.com>
wrote:

> Hi
>
> I sent correct mail, that requires the approval - maybe bad filter?
>

There are some pretty restrictive filters in place on the mj2 lists in
order to deal with cases where people send admin requests. This happens now
and then, and will get released from the moderation queue quickly I'm sure
:)

Long term we'll be getting rid of those filters, but they're there for a
while longer.

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

Re: [HACKERS] A handful of typos in allpaths.c

[Magnus Hagander]

On Wed, Oct 18, 2017 at 4:45 AM, David Rowley <david.row...@2ndquadrant.com>
wrote:

> A small patch to fix these is attached.
>

Applied, thanks.

I backpatched the actual error message typo fix. Left the comment alone in
backbranches because it conflicted, so it didn't seem worth it.

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

Re: [HACKERS] Fix a typo in libpq/auth.c

[Magnus Hagander]

On Thu, Oct 19, 2017 at 12:05 PM, Masahiko Sawada <sawada.m...@gmail.com>
wrote:

> Hi,
>
> Attached a patch for $subject.
>
> s/RAIDUS/RADIUS/
>

Applied, thanks.

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

Re: [HACKERS] Determine state of cluster (HA)

[Magnus Hagander]

On Mon, Oct 16, 2017 at 4:39 AM, Craig Ringer <cr...@2ndquadrant.com> wrote:

> On 13 October 2017 at 08:50, Joshua D. Drake <j...@commandprompt.com> wrote:
> > 5.  There is no way to connect to a db node with something akin to
> > SQL-Server's "application intent" flags, to allow a connection to be
> > rejected if we wish it to be a read/write connection.  This helps detect
> the
> > state of the node directly without having to ask any further questions of
> > the node, and makes it easier to "stall" during connection until a proper
> > connection can be made.
>
> That sounds desirable, and a good step toward eventually being able to
> transparently re-route read/write queries from replica to master.
> Which is where I'd like to land up eventually.
>

It also sounds a lot like the connection parameter target_session_attrs,
does it not? We don't reroute active connections based on it, and we're not
smart enough to do anything beyond "try them one by one until you reach the
one with the correct attributes", but the basic functionality is there.
Basically what we already have fulfills what JD is suggesting, but not what
Craig is, if I understand it correctly.

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

Re: [HACKERS] v10 bottom-listed

[Magnus Hagander]

On Fri, Oct 6, 2017 at 3:59 AM, Amit Langote <langote_amit...@lab.ntt.co.jp>
wrote:

> On 2017/10/05 22:28, Erik Rijkers wrote:
> > In the 'ftp' listing, v10 appears at the bottom:
> >   https://www.postgresql.org/ftp/source/
> >
> > With all the other v10* directories at the top, we could get a lot of
> > people installing wrong binaries...
> >
> > Maybe it can be fixed so that it appears at the top.
>
> Still see it at the bottom.  Maybe ping pgsql-www?
>

Turns out there was already quite an ugly hack to deal with this, so I just
made it a bit more ugly. Once the caches expire I believe sorting should be
correct.

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

Re: [HACKERS] search path security issue?

[Magnus Hagander]

On Fri, Oct 6, 2017 at 12:05 AM, Joshua D. Drake <j...@commandprompt.com>
wrote:

> On 10/05/2017 02:54 PM, David G. Johnston wrote:
>
>> On Thu, Oct 5, 2017 at 2:37 PM, Joshua D. Drake <j...@commandprompt.com
>> <mailto:j...@commandprompt.com>>wrote:
>>
>> I get being able to change my search_path on the fly but it seems
>> odd that as user foo I can change my default search path?
>>
>>
>> Seems down-right thoughtful of us to allow users to change their own
>> defaults instead of forcing them to always change things on-the-fly or bug
>> a DBA to change the default for them.
>>
>
> It seems that if a super user changes the search path with ALTER
> USER/ROLE, then the user itself should not (assuming not an elevated
> privilege) should not be able to change it. Again, I get being able to do
> it with SET but a normal user shouldn't be able to reset a super user
> determined setting.
>

This is in no way specific to search_path.

It would be a nice feature to have in general, like a "basic guc
permissions" thing. At least allowing a superuser to prevent exactly this.
You could argue the same thing for example for memory parameters and such.
We have no permissions at all when it comes to userset gucs today -- and of
course, if something should be added about this, it should be done in a way
that works for all the userset variables, not just search_path.

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

Re: [HACKERS] list of credits for release notes

[Magnus Hagander]

On Wed, Oct 4, 2017 at 8:51 AM, Laurenz Albe <laurenz.a...@cybertec.at>
wrote:

> Peter Eisentraut wrote:
> > At the PGCon Developer Meeting it was agreed[0] to add a list of credits
> > to the release notes, including everyone who was mentioned in a commit
> > message.  I have now completed that list.
> >
> > Attached is the proposed documentation commit as well as the raw list.
>
> > The list is sorted using COLLATE "en-x-icu".
>
> It would be awesome if the list could be sorted by last name,
> as name lists traditionally are, but maybe that's too much to ask.
>

Whether that's traditionally or not very much depends on which part of the
world you are in, I believe. Let's try to avoid going down that rabbithole
:)

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

Re: [HACKERS] Possible SSL improvements for a newcomer to tackle

[Magnus Hagander]

On Tue, Oct 3, 2017 at 3:51 PM, Stephen Frost <sfr...@snowman.net> wrote:

> Tom,
>
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
> > Magnus Hagander <mag...@hagander.net> writes:
> > > On Tue, Oct 3, 2017 at 6:33 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> > >> I'm not an SSL expert, so insert appropriate grain of salt, but AIUI
> the
> > >> question is what are you going to verify against?
> >
> > > One way to do it would be to default to the "system global certificate
> > > store", which is what most other SSL apps do. For example on a typical
> > > debian/ubuntu, that'd be the store in /etc/ssl/certs/ca-
> certificates.crt.
> > > Exactly where to find them would be distribution-specific though, and
> we
> > > would need to actually add support for a second certificate store. But
> that
> > > would probably be a useful feature in itself.
> >
> > Maybe.  The impression I have is that it's very common for installations
> > to use a locally-run CA to generate server and client certs.  I would not
> > expect them to put such certs into /etc/ssl/certs.  But I suppose there
> > might be cases where you would actually pay for a universally-valid cert
> > for a DB server ...
>
> In many larger enterprises, they actually deploy systems with their own
> CA installed into the system global certificate store (possibly removing
> certain other CAs from that set too, and distributing their own version
> of the relevant package that maintains the CA set).
>

I think this is also something that's seen more now than it used to be.
Back when we initially did the SSL support, few people actually did this.
That's not just for this scenario of course -- larger enterprises are much
more likely to *have* proper PKI management today, and if they do then it
will include the Linux boxes.

Bottom line: things in this area has change greatly in the past 10-15 years.

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

Re: [HACKERS] Possible SSL improvements for a newcomer to tackle

[Magnus Hagander]

On Tue, Oct 3, 2017 at 6:33 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Zeus Kronion <zkron...@gmail.com> writes:
> > 2) I was surprised to learn the following from the docs:
>
> >> By default, PostgreSQL will not perform any verification of the server
> >> certificate.
>
> > Is there a technical reason to perform no verification by default?
> Wouldn't
> > a safer default be desirable?
>
> I'm not an SSL expert, so insert appropriate grain of salt, but AIUI the
> question is what are you going to verify against?  You need some local
> notion of which are your trusted root certificates before you can verify
> anything.  So to default to verification would be to default to failing to
> connect at all until user has created a ~/.postgresql/root.crt file with
> valid, relevant entries.  That seems like a nonstarter.
>

Yes, you need something to verify against.

One way to do it would be to default to the "system global certificate
store", which is what most other SSL apps do. For example on a typical
debian/ubuntu, that'd be the store in /etc/ssl/certs/ca-certificates.crt.
Exactly where to find them would be distribution-specific though, and we
would need to actually add support for a second certificate store. But that
would probably be a useful feature in itself.



> It's possible that we could adopt some policy like "if the root.crt file
> exists then default to verify" ... but that seems messy and unreliable,
> so I'm not sure it would really add any security.
>

No that's horrible. If it's unreliable, it doesn't provide any actual
benefit. We have a history of that in our default being prefer instead of
allow, but we definitely shouldn't make that situation even worse.

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

Re: [HACKERS] 64-bit queryId?

[Magnus Hagander]

On Mon, Oct 2, 2017 at 1:22 AM, Robert Haas <robertmh...@gmail.com> wrote:

> On Sun, Oct 1, 2017 at 3:48 PM, Greg Stark <st...@mit.edu> wrote:
> > Well these kinds of monitoring systems tend to be used by operations
> > people who are a lot more practical and a lot less worried about
> > theoretical concerns like that.
>
> +1, well said.
>

+1 as well. I think these people would be perfectly find by it changing
across a version upgrade as long as they know that's the deal. *Most* of
the time there is no version upgrade going on, so it would work fine that
time.

Most operations people already deal with a lot of such parameters changing
around. I'm sure most of them would be more than happy with an improvement,
even if it's not mathematically perfect.

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

Re: [HACKERS] Enhancements to passwordcheck

[Magnus Hagander]

On Fri, Sep 29, 2017 at 3:17 PM, Alvaro Herrera <alvhe...@alvh.no-ip.org>
wrote:

> Michael Paquier wrote:
>
> > Client commands may be run on a trusted network as well, let's not
> > forget that.
>
> I've never seen this "trusted network" thing of which you speak.  Is it
> nice?
>

I believe it's usually referred to as "localhost"?

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

Re: [HACKERS] Built-in plugin for logical decoding output

[Magnus Hagander]

On Tue, Sep 26, 2017 at 2:16 PM, Alvaro Hernandez <a...@ongres.com> wrote:

>
>
> On 26/09/17 12:57, Petr Jelinek wrote:
>
>> On 26/09/17 09:26, Alvaro Hernandez wrote:
>>
>>> On 26/09/17 10:03, Craig Ringer wrote:
>>>
>>>> On 26 September 2017 at 14:08, Alvaro Hernandez <a...@ongres.com
>>>> <mailto:a...@ongres.com>> wrote:
>>>>  - If you stick to in-core plugins, then you need to support at
>>>>  least three different output formats if you want to support 9.4+:
>>>>  test_decoding (and pray it works!), pgoutput, and the "new"
>>>>  in-core plugin that was proposed at the beginning of this thread,
>>>>  if that would see the light.
>>>>
>>>>
>>>> The only practical way will IMO be to have whatever new plugin it also
>>>> have an out-of-core version maintained for older Pg versions, where it
>>>> can be installed.
>>>>
>>>>  But only in-core plugins help for general-purpose solutions.
>>>>
>>>>
>>>> I still don't agree there. If there's enough need/interest/adoption
>>>> you can get cloud vendors on board, they'll feel the customer
>>>> pressure. It's not our job to create that pressure and do their work
>>>> for them.
>>>>
>>>  Don't want to get into a loop, but as I said before it's
>>> chicken-and-egg. But nobody is asking core to do their work. As much as
>>> I love it, I think logical decoding is a bit half-baked until there is a
>>> single, quality, in-core plugin, as it discourages its usage, because of
>>> the reasons I stated.
>>>
>>> Well, in that case it's all good as PG10 has that.
>>
>>
> Even though it's not fully documented, I agree this could fulfill this
> gap for 10+ (I assume this plugin will be maintained onwards, at least to
> support logical replication).
>
> But what about earlier versions? Any chance it could be backported
> down to 9.4? If that would be acceptable, I could probably help/do that...


The likelihood is zero if you mean backported into core of earlier versions.

If you mean backported as a standalone extension that could be installed on
a previous version, probably. I'm not sure if it relies on any internals
not present before that would make it harder, but it would probably at
least be possible.

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

Re: [HACKERS] Built-in plugin for logical decoding output

[Magnus Hagander]

On Tue, Sep 26, 2017 at 7:42 AM, Alvaro Hernandez <a...@ongres.com> wrote:

>
>
> On 25/09/17 22:13, Magnus Hagander wrote:
>
> On Mon, Sep 25, 2017 at 8:20 PM, Alvaro Hernandez <a...@ongres.com> wrote:
>
>>
>>
>> On 25/09/17 20:18, Andres Freund wrote:
>>
>>> On 2017-09-24 13:36:56 +0300, Alvaro Hernandez wrote:
>>>
>>>>  However, if DMS uses it for what I'd call production use, I assume
>>>> it is
>>>> actually production quality. I bet they do enough testing, and don't
>>>> ship
>>>> software to potentially millions of customers if it doesn't work well.
>>>> So...
>>>> first, I'd consider this a a sign of robustness.
>>>>
>>> You've been in software for how long? ... ;)  There's quite mixed
>>> experiences with DMS.
>>>
>>
>> Actually long enough to understand that if someone "big" calls it
>> production quality, we should not be pickier and assume it is --whether it
>> is or not. People will accept it as such, and that's good enough.
>>
>
> Historically the fact that we have been pickier than many of the "someone
> big":s is exactly how we ended up with the codebase and relative stability
> we have today.
>
> Just because someone is big doesn't mean they know what's right. In fact,
> more often than not the opposite turns out to be true.
>
>
>
> Note that I'm not here supporting test_decoding. I'm just saying is
> all what is available in-core for 9.4-9.6, and it seems someone with
> potentially a lot of users tested it and is using it in its own solution.
> Ask me if I would like an in-core, well tested, performant, with an easy to
> parse format, and efficient, for 9.4-9.6? My answer would be an immediate
> 'yes'. But since this is not going to happen, test_decoding is good that is
> good enough, lucky us, because otherwise there would not be a good solution
> on this front.
>

I am not saying we shouldn't have that. I am saying that the argument "if
someone big calls it production quality, we should not be pickier and
assume it is" is incorrect.

And yes, I have used test_decoding in production multiple times. And yes,
there are good reasons why it's called *test* decoding, and should only be
used in production in fairly simple cases :)


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

Re: [HACKERS] Built-in plugin for logical decoding output

[Magnus Hagander]

On Mon, Sep 25, 2017 at 8:20 PM, Alvaro Hernandez <a...@ongres.com> wrote:

>
>
> On 25/09/17 20:18, Andres Freund wrote:
>
>> On 2017-09-24 13:36:56 +0300, Alvaro Hernandez wrote:
>>
>>>  However, if DMS uses it for what I'd call production use, I assume
>>> it is
>>> actually production quality. I bet they do enough testing, and don't ship
>>> software to potentially millions of customers if it doesn't work well.
>>> So...
>>> first, I'd consider this a a sign of robustness.
>>>
>> You've been in software for how long? ... ;)  There's quite mixed
>> experiences with DMS.
>>
>
> Actually long enough to understand that if someone "big" calls it
> production quality, we should not be pickier and assume it is --whether it
> is or not. People will accept it as such, and that's good enough.
>

Historically the fact that we have been pickier than many of the "someone
big":s is exactly how we ended up with the codebase and relative stability
we have today.

Just because someone is big doesn't mean they know what's right. In fact,
more often than not the opposite turns out to be true.

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

Re: [HACKERS] Reading backup label file for checkpoint and redo location during crash recovery

[Magnus Hagander]

On Mon, Sep 25, 2017 at 7:43 PM, Stephen Frost <sfr...@snowman.net> wrote:

> Greetings Satya,
>
> * Satyanarayana Narlapuram (satyanarayana.narlapu...@microsoft.com) wrote:
> > During crash recovery, last checkpoint record information is obtained
> from the backup label if present, instead of getting it from the control
> file. This behavior is causing PostgreSQL database cluster not to come up
> until the backup label file is deleted (as the error message says).
> >
> > if (checkPoint.redo < checkPointLoc)
> >   {
> >  if (!ReadRecord(xlogreader,
> checkPoint.redo, LOG, false))
> > ereport(FATAL,
> >   (errmsg("could not
> find redo location referenced by checkpoint record"),
> >   errhint("If you are
> not restoring from a backup, try removing the file \"%s/backup_label\".",
> DataDir)));
> >   }
> >
> > If we are recovering from a dump file, reading from the backup label
> files makes sense as the control file could be archived after a few
> checkpoints. But this is not the case for crash recovery, and is always
> safe to read the checkpoint record information from the control file.
> > Is this behavior kept this way as there is no clear way to distinguish
> between the recovery from the dump and the regular crash recovery?
>
> This is why the exclusive backup method has been deprecated in PG10 in
> favor of the non-exclusive backup method, which avoids this by not
> creating a backup label file (it's up to the backup software to store
> the necessary information and create the file for use during recovery).
>


Actally, it was deprecated already in 9.6, not just 10.


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

Re: [HACKERS] Reporting query on crash even if completed

[Magnus Hagander]

On Mon, Sep 18, 2017 at 6:12 PM, Robert Haas <robertmh...@gmail.com> wrote:

> On Mon, Sep 18, 2017 at 9:47 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> > Now, for pg_stat_activity part of the argument why this wouldn't be
> > confusing was that you could also see the "state" field.  Maybe we
> > should try to shoehorn equivalent info into the crash log entry?
>
> Yeah, I think so.  Really, I think this is an inadvertency, and thus a
>

Yeah, I agree. That was nothing I recall thinking about at the time, so
strictly speaking it's a bug.



> bug.  But instead of just not showing the query when the backend is
> idle, I'd change the display for that case to:
>
> DETAIL: Failed process was idle; last query was: %s
>
> Or something like that.  I guess we'd need another case for a backend
> that crashed without ever running a query.
>

+1, this is a better solution.

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

Re: [HACKERS] pg_basebackup behavior on non-existent slot

[Magnus Hagander]

On Tue, Sep 12, 2017 at 7:35 PM, Jeff Janes <jeff.ja...@gmail.com> wrote:

> On Wed, Sep 6, 2017 at 2:50 AM, Alvaro Herrera <alvhe...@alvh.no-ip.org>
> wrote:
>
>> Magnus Hagander wrote:
>> > On Mon, Sep 4, 2017 at 3:21 PM, Jeff Janes <jeff.ja...@gmail.com>
>> wrote:
>>
>> > > Should the parent process of pg_basebackup be made to respond to
>> SIGCHLD?
>> > > Or call waitpid(bgchild, , WNOHANG) in some strategic loop?
>> >
>> > I think it's ok to just call waitpid() -- we don't need to react super
>> > quickly, but we should react.
>>
>> Hmm, not sure about that ... in the normal case (slotname is correct)
>> you'd be doing thousands of useless waitpid() system calls during the
>> whole operation, no?  I think it'd be better to have a SIGCHLD handler
>> that sets a flag (just once), which can be quickly checked without
>> accessing kernel space.
>>
>
> If we don't want polling by waitpid, then my next thought would be to move
> the data copy into another process, then have the main process do nothing
> but wait for the first child to exit.  If the first to exit is the WAL
> receiver, then we must have an error and the data receiver can be killed.
> I don't know how to translate that to Windows, however.
>
>
Well, we could do something similar -- run the main process and the
streamer in separate threads on windows and have a main thread wait on
both. The main thread would have to be in charge of cleanup as well of
course. But I think that's likely going to be more complicated than using
non blocking libpq APIs.


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

Re: [HACKERS] Supporting huge pages on Windows

[Magnus Hagander]

On Wed, Sep 13, 2017 at 3:41 AM, Tsunakawa, Takayuki <
tsunakawa.ta...@jp.fujitsu.com> wrote:

> Hi Thomas, Magnus
>
> From: pgsql-hackers-ow...@postgresql.org
> > [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Thomas Munro
> > Since it only conflicts with c7b8998e because of pgindent whitespace
> > movement, I applied it with "patch -p1 --ignore-whitespace" and created
> > a new patch.  See attached.
>
> Thanks, Thomas.  I've added your name in the CF entry so that your name
> will also be listed on the release note, because my patch is originally
> based on your initial try.  Please remove your name just in case you mind
> it.  BTW, your auto-reviewer looks very convenient.  Thank you again for
> your great work.
>
> Magnus, it would be grateful if you could review and commit the patch
> while your memory is relatively fresh.
>
> I've been in a situation which keeps me from doing development recently,
> but I think I can gradually rejoin the community activity soon.
>
>
Hi!

It's my plan to get to this patch during this commitfest. I've been
travelling for open and some 24/7 work so far, but hope to get CFing soon.



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

Re: [HACKERS] pg_basebackup behavior on non-existent slot

[Magnus Hagander]

On Wed, Sep 6, 2017 at 11:50 AM, Alvaro Herrera <alvhe...@alvh.no-ip.org>
wrote:

> Magnus Hagander wrote:
> > On Mon, Sep 4, 2017 at 3:21 PM, Jeff Janes <jeff.ja...@gmail.com> wrote:
>
> > > Should the parent process of pg_basebackup be made to respond to
> SIGCHLD?
> > > Or call waitpid(bgchild, , WNOHANG) in some strategic loop?
> >
> > I think it's ok to just call waitpid() -- we don't need to react super
> > quickly, but we should react.
>
> Hmm, not sure about that ... in the normal case (slotname is correct)
> you'd be doing thousands of useless waitpid() system calls during the
> whole operation, no?  I think it'd be better to have a SIGCHLD handler
> that sets a flag (just once), which can be quickly checked without
> accessing kernel space.
>

Good point.

So the question is what to do for Windows. I'd rather not have to bring in
the whole extra thread and socket emulation stuff into pg_basebackup if it
can be avoided. But I guess we could code up something Windows-specific in
just that one (since it's threaded and not processed on Windows, it's
easier than the backend). I think that means we'd have to rewrite it to use
the async libpq apis, don't you?

The other option would be to just kill the process from the child thread.
Since the're threads we can do that. However, that will leave us in a
position where we can't clean up from the error (as in remove files/dirs),
not sure that's good?


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

Re: [HACKERS] WIP: Separate log file for extension

[Magnus Hagander]

On Fri, Aug 25, 2017 at 12:12 AM, Antonin Houska <a...@cybertec.at> wrote:

> Attached is a draft patch to allow extension to write log messages to a
> separate file. It introduces a concept of a "log stream". The extension's
> shared library gets its stream assigned by calling this function from
> _PG_init()


> my_stream_id = get_log_stream("my_extension", _log_stream);
>

I like this idea in general.



> Then it's supposed to change some of its attributes
>
> adjust_log_stream_attr(>filename, "my_extension.log");
>

This, however, seems to be wrong.

The logfile name does not belong in the extension, it belongs in the
configuration file. I think the extension should set it's "stream id" or
whatever you want to call it, and then it should be possible to control in
postgresql.conf where that log is sent.

Also, what if this extension is loaded on demand in a session and not via
shared_preload_libraries? It looks like the syslogger only gets the list of
configured streams when it starts?

In short, I think the solution should be more generic, and not "just for
extensions".

I completely missed this thread when I did my quick-wip at
https://www.postgresql.org/message-id/flat/cabuevexztl0gorywm9s4tr_ft3fmjbraxqdxj+bqzjpvmru...@mail.gmail.com#cabuevexztl0gorywm9s4tr_ft3fmjbraxqdxj+bqzjpvmru...@mail.gmail.com
 -- some of the changes made were close enough that I got the two confused
:) Based on the feedback of that one, have you done any performance checks?


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

Re: [HACKERS] log_destination=file

[Magnus Hagander]

On Thu, Sep 7, 2017 at 7:02 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Dmitry Dolgov <9erthali...@gmail.com> writes:
> >> On 31 August 2017 at 14:49, Tom Lane <t...@sss.pgh.pa.us> wrote:
> >> pgbench with log_statement = all would be a pretty easy test case.
>
> > It seems that for this particular workload it was about 20-25% slower.
>
> Ouch.  That seems like rather a large hit :-(.  I actually expected
> it to be close to negligible, but I don't think 20% qualifies.
>

Agreed, that's a lot more than I expected. A few questions though:

1. where was stderr actually sent? To the console, to /dev/null or to a
file?

2. Could you run the same thing (on the same machine) with the logging
collector on and logging to file, without the patch? I'd assume that gives
performance similar to running with the patch, but it would be good to
confirm that.

And thanks for running the benchmark, saved me some time!


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

Re: [HACKERS] GnuTLS support

[Magnus Hagander]

On Thu, Sep 7, 2017 at 2:34 PM, Tomas Vondra <tomas.von...@2ndquadrant.com>
wrote:

> Hi,
>
> On 09/04/2017 04:24 PM, Bruce Momjian wrote:
> > On Fri, Sep  1, 2017 at 12:09:35PM -0400, Robert Haas wrote:
> >> I think that what this shows is that the current set of GUCs is overly
> >> OpenSSL-centric.  We created a set of GUCs that are actually specific
> >> to one particular implementation but named them as if they were
> >> generic.  My idea about this would be to actually rename the existing
> >> GUCs to start with "openssl" rather than "ssl", and then add new GUCs
> >> as needed for other SSL implementations.
> >>
> >> Depending on what we think is best, GUCs for an SSL implementation
> >> other than the one against which we compiled can either not exist at
> >> all, or can exist but be limited to a single value (e.g. "none", as we
> >> currently do when the compile has no SSL support at all).  Also, we
> >> could add a read-only GUC with a name like ssl_library that exposes
> >> the name of the underlying SSL implementation - none, openssl, gnutls,
> >> or whatever.
> >>
> >> I think if we go the route of insisting that every SSL implementation
> >> has to use the existing GUCs, we're just trying to shove a square peg
> >> into a round hole, and there's no real benefit for users.  If the
> >> string that has to be stuffed into ssl_ciphers differs based on which
> >> library was chosen at compile time, then you can't have a uniform
> >> default configuration for all libraries anyway.  I think it'll be
> >> easier to explain and document this if there's separate documentation
> >> for openssl_ciphers, gnutls_ciphers, etc. rather than one giant
> >> documentation section that tries to explain every implementation
> >> separately.
> >
> > I am worried about having 3x version of TLS controls in
> > postgresql.conf, and only one set being active. Perhaps we need to
> > break out the TLS config to separate files or something. Anyway, this
> > needs more thought.
> >
>
> Well, people won't be able to set the inactive options, just like you
> can't set ssl=on when you build without OpenSSL support. But perhaps we
> could simply not include the inactive options into the config file, no?
>

We build the pg_hba.conf file dynamically depending on if we have ipv6
support, IIRC. Maybe we need to implement that type of support into
postgresql.conf as well?

It will still be a mess though -- documentation, and tutorials around and
whatnot, will be dependent on library. But I'm not sure we can really get
around that.

Do we have some examples of how other products that support multiple
libraries do to handle this?


>
> I don't see how breaking the TLS config into separate files improves the
> situation - instead of dealing with GUCs in a single file you'll be
> dealing with the same GUCs in multiple files. No?
>

+1. I don't think splitting them up into different files makes it in any
way better -- if anything, it makes it worse.


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

Re: [HACKERS] pg_basebackup behavior on non-existent slot

[Magnus Hagander]

On Mon, Sep 4, 2017 at 3:21 PM, Jeff Janes <jeff.ja...@gmail.com> wrote:

>
> If I tell pg_basebackup to use a non-existent slot, it immediately reports
> an error.  And then it exits with an error, but only after streaming the
> entire database contents.
>
> If you are doing this interactively and are on the ball, of course, you
> can hit ctrl-C when you see the error message.
>
> I don't know if this is exactly a bug, but it seems rather unfortunate.
>

I think that should qualify as a bug.

In 10 it will automatically create a transient slot in this case, but there
might still be a case where you can provoke this.



> Should the parent process of pg_basebackup be made to respond to SIGCHLD?
> Or call waitpid(bgchild, , WNOHANG) in some strategic loop?
>

I think it's ok to just call waitpid() -- we don't need to react super
quickly, but we should react. And we should then exit the main process with
an error before actually streaming everything.

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

Re: [HACKERS] Release Note changes

[Magnus Hagander]

On Mon, Sep 4, 2017 at 1:11 PM, Robert Haas <robertmh...@gmail.com> wrote:

> On Mon, Sep 4, 2017 at 4:49 AM, Simon Riggs <si...@2ndquadrant.com> wrote:
> > Migration to Version 10
> >
> > "A dump/restore using pg_dumpall, or use of pg_upgrade, is required
> > for those wishing to migrate data from any previous release."
> >
> > This isn't true and is seriously misleading ...
>
> Fair point.
>
> > since pglogical and other
> > proprietary tools exist that were designed specifically to allow this.
> > Suggested additional wording would be...
> >
> > "If upgrading from a 9.4 server or later, external utilities using
> > logical decoding, such as pglogical or proprietary alternatives, can
> > also provide an alternate route, often with lower downtime."
>
> It's not really true that the only alternatives to pglogical are
> proprietary.  Really, one could use any logical replication solution,
> and there have been multiple open source alternatives for a decade.
>
> > Our docs mention pglogical already, so don't see an issue with
> > mentioning it here.
>
> The existing reference is alongside a bunch of other third-party
> tools.  Mentioning it at the very top of our release notes would give
> it a pride of place with which I'm not too comfortable.
>

I agree that singling it out there is probably not the best idea. But a
sentence/paragraph saying that there are third party replication solutions
that can solve the problem, along with linking to the page with the list,
perhaps?

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

Re: [HACKERS] Function to move the position of a replication slot

[Magnus Hagander]

On Sun, Sep 3, 2017 at 12:20 AM, Robert Haas <robertmh...@gmail.com> wrote:

> On Fri, Sep 1, 2017 at 11:30 PM, Peter Eisentraut
> <peter.eisentr...@2ndquadrant.com> wrote:
> > On 8/31/17 08:19, Magnus Hagander wrote:
> >> Rebased. Now named pg_advance_replication_slot. ERROR on logical slots.
> >> Forward only.
> >>
> >> I think that, in the end, covered all the comments?
> >
> > I didn't see any explanation of what this would actually be useful for.
> > I suppose you could skip over some changes you don't want replicated,
> > but how do you find to what position to skip?
> >
> > Logical replication has a similar mechanism, using the function
> > pg_replication_origin_advance().  Any overlap there?  (Maybe the names
> > could be aligned.)
> > (https://www.postgresql.org/docs/devel/static/logical-
> replication-conflicts.html)
>
> I think you can use this to work around the absence of failover slots.
>
>
That was the initial usecase I had for this, yes.

It can also be combined with file-based restoring to keep a "blocker"
preventing removal before a segment has progressed through a workflow for
example.

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

Re: [HACKERS] adding the commit to a patch's thread

[Magnus Hagander]

On Sun, Sep 3, 2017 at 8:55 PM, Daniel Gustafsson <dan...@yesql.se> wrote:

> > On 03 Sep 2017, at 19:33, Magnus Hagander <mag...@hagander.net> wrote:
> >
> > On Sun, Sep 3, 2017 at 12:00 AM, Daniel Gustafsson <dan...@yesql.se
> <mailto:dan...@yesql.se>> wrote:
> > > On 01 Sep 2017, at 15:40, Robert Haas <robertmh...@gmail.com  robertmh...@gmail.com>> wrote:
> > >
> > > On Fri, Sep 1, 2017 at 4:26 AM, Alvaro Herrera <
> alvhe...@alvh.no-ip.org <mailto:alvhe...@alvh.no-ip.org>> wrote:
> > >> Erik Rijkers wrote:
> > >>> Would it be possible to change the commitfest a bit and make it
> possible to
> > >>> add the commit (or commit-message, or hash) to the thread in the
> > >>> commitfest-app.
> > >>
> > >> +1 to add one or more commit hashes to CF entry metadata.
> > >>
> > >> (Back-filling for old entries welcome)
> > >
> > > Couldn't the CF app scrape the commit messages for references to
> > > threads, and if the commit message points to a thread with exactly 1
> > > patch record, associate the commit to that patch?
> >
> > Since there is a Gitlink field in the patch metadata, we could start
> simple
> > with setting that to link to the commit on pg gitweb?  While adding the
> thread
> > of the commit from -committers would be great as well, this gets us off
> the
> > ground quickly.
> >
> > The original idea behind the gitlink field was to link to a git
> repo/branch representing the patch, for people who preferred to publish
> full branches for those that want it.
> >
> > This has been done for a grand total of 43 patches throughout (out of a
> total of 1231).
> >
> > Not sure if that's enough to say "let's not repurpose it”?
>
> My thinking was that it wasn’t really repurposing, since the commit is
> quite
> representative of the patch and comma separated list could house both
> (especially since the former usecase is quite rate).  Looking at the code
> however, the Gitlink is a UrlField which I believe won’t support that so
> it’s
> dead in the water either way.
>

I think that would definitely be repurposing it since it's not used for
that in the past :)


That leaves us back at parsing/scraping -committers and adding the
> mailthread.
> For this CF I can add the commits manually, since attaching a thread isn’t
> limited to threads in -hackers.  This way we have more time to figure out
> an
> automated way for the next CF.  I’ve added the commit for my patch here as
> an
> example:
>
> https://commitfest.postgresql.org/14/1245/


That's a good start.

We can also add a separate field that's just a comma separated list of
commit hashes that auto-links to the git server if we want. *That* would be
a trivial addition. Actually auto-populating it would probably be a lot
less so, but for a manually managed one it should be fairly easy.

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

Re: [HACKERS] adding the commit to a patch's thread

[Magnus Hagander]

On Sun, Sep 3, 2017 at 12:00 AM, Daniel Gustafsson <dan...@yesql.se> wrote:

> > On 01 Sep 2017, at 15:40, Robert Haas <robertmh...@gmail.com> wrote:
> >
> > On Fri, Sep 1, 2017 at 4:26 AM, Alvaro Herrera <alvhe...@alvh.no-ip.org>
> wrote:
> >> Erik Rijkers wrote:
> >>> Would it be possible to change the commitfest a bit and make it
> possible to
> >>> add the commit (or commit-message, or hash) to the thread in the
> >>> commitfest-app.
> >>
> >> +1 to add one or more commit hashes to CF entry metadata.
> >>
> >> (Back-filling for old entries welcome)
> >
> > Couldn't the CF app scrape the commit messages for references to
> > threads, and if the commit message points to a thread with exactly 1
> > patch record, associate the commit to that patch?
>
> Since there is a Gitlink field in the patch metadata, we could start simple
> with setting that to link to the commit on pg gitweb?  While adding the
> thread
> of the commit from -committers would be great as well, this gets us off the
> ground quickly.
>

The original idea behind the gitlink field was to link to a git repo/branch
representing the patch, for people who preferred to publish full branches
for those that want it.

This has been done for a grand total of 43 patches throughout (out of a
total of 1231).

Not sure if that's enough to say "let's not repurpose it"?

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

Re: [HACKERS] log_destination=file

[Magnus Hagander]

On Fri, Sep 1, 2017 at 6:44 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Magnus Hagander <mag...@hagander.net> writes:
> > On Fri, Sep 1, 2017 at 6:00 PM, Greg Stark <st...@mit.edu> wrote:
> >> So what happens now with these messages? My understanding is that
> >> they're missing from the CSV logs and are simply inserted into the
> >> text logs without any log_line_prefix? The logging collector doesn't
> >> recognize these messages and reformat them for the CSV logs does it?
>
> > Yeah, that's pretty much it.
>
> > Fixing that is also on my plan, but for later :)
>
> Keep in mind that you've got to be really, really conservative about
> adding functionality in the logging collector process.  If it ever
> crashes, we have problems.  If memory serves, the postmaster is able
> to restart it, but we cannot restore the original stdout/stderr
> destination, which is problematic if that's where its output had
> been going.  So it's pretty close to being as mission-critical as
> the postmaster itself.
>

Yeah. That's one of the reasons I'm not trying to do it all in one batch.

But yeah, the postmaster restarts it just fine. And with the WIP patch I
posted earlier, the message from the postmaster that it did gets lost if
you are logging to stderr. It does end up in the CSV file though. And I'm
sure there are plenty of other corner cases around that one to consider.

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

Re: [HACKERS] log_destination=file

[Magnus Hagander]

On Fri, Sep 1, 2017 at 6:00 PM, Greg Stark <st...@mit.edu> wrote:

> On 31 August 2017 at 13:49, Tom Lane <t...@sss.pgh.pa.us> wrote:
> > Magnus Hagander <mag...@hagander.net> writes:
> >> On Thu, Aug 31, 2017 at 2:34 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> > Yes, it's pretty important, because of assorted stuff not-under-our-
> > control that doesn't know about ereport and will just print to stderr
> > anyway.  Some examples: dynamic linker can't-resolve-symbol failure
> > messages, glibc malloc corruption error messages, just about any external
> > module in plperl or plpython.  I don't find this to be negotiable.
>
> So what happens now with these messages? My understanding is that
> they're missing from the CSV logs and are simply inserted into the
> text logs without any log_line_prefix? The logging collector doesn't
> recognize these messages and reformat them for the CSV logs does it?
>

Yeah, that's pretty much it.

Fixing that is also on my plan, but for later :)



> I'm actually asking because I'm more concerned with JSON logs or
> msgpack logs. Currently these are supported with an emit_log_hook but
> they can't capture these non-ereport logs either.
>
> Also the CSV and emit_log_hook based logs don't have any convenient
> way to turn them on and off and control the location and filename of
> the logs. It would be nice if we could have something like
>
> log_destinations='stderr=text,syslog=json,postgresql-%Y-%m-%
> d_%H%M%S.csv=csv'
>

That could definitely be an interesting improvement as well. Decoupling the
format from the destination would make a lot of sense. (Arguments can
certainly be made for example for csv-to-syslog, when you have a custom
syslog server)



> >> Are you actually asking for a benchmark of if logging gets slower?
> >
> > Yes.
>
> Personally I don't think it's "performance" so much as operational
> issues that are more concerning. For all we know there are people out
> there who tried to use the logging collector and found it didn't work
> well on some system -- perhaps it interacted with systemd or something
> else on the system -- and they switched back to just using stderr. I
> don't know how to flush these users out though if there are any. Just
> making this change early in a release cycle is the best we can do.
>

The most common use of stderr I think is debian/ubuntu which uses that and
then pg_ctl based redirect. Or at least used to (I remember being annoyed
by that at least once..)

AFAIK all other major platforms already use the logging collector in the
standard packages.

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

Re: [HACKERS] log_destination=file

[Magnus Hagander]

On Thu, Aug 31, 2017 at 2:34 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Magnus Hagander <mag...@hagander.net> writes:
> > My understanding is that the main reason for this is that we cannot
> change
> > logging_collector without restarting postmaster, whereas we can change
> > log_destination.
>
> Right, because the decision whether to redirect stdout/stderr can't
> be changed on the fly.
>

Right.

We could of course also say we only care about things generated by our
ereport framework, in which case we don't need to redirect stderr and can
just use a "regular pipe". But IIRC that's functionality we also
specifically wanted (though I don't think I've ever needed it myself,
presumably others have).




> > My suggestion is we work around this by just always starting the logging
> > collector, even if we're not planning to use it.
>
> Umm
>
> > Do people see an actual problem with that? I agree it's an extra round of
> > indirection there, but is that a problem? It would also cause one more
> > backgorund process to always be running, but again, is that really a
> > problem? The overhead is not exactly large.
>
> You just made three assertions about "this isn't a problem" without
> providing any evidence in support of any of them.  Maybe with some
> measurements we could have a real discussion.
>

Well, not entirely. The first two are questions "is this really a problem".

The overhead of an extra process certainly isn't much, I'm wiling to say
that as an assertion.

The other two, less so, that's more question.

Are you actually asking for a benchmark of if logging gets slower? If so,
could you suggest a workload to make an actual benchmark of it (where
logging would be high enough that it could be come a bottleneck -- and not
writing the log data to disk, but the actual logging). I'm not sure what a
good one would be.

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

Re: [HACKERS] Function to move the position of a replication slot

[Magnus Hagander]

On Thu, Aug 17, 2017 at 2:19 AM, Craig Ringer <cr...@2ndquadrant.com> wrote:

> On 17 August 2017 at 07:30, Michael Paquier <michael.paqu...@gmail.com>
> wrote:
>
>>
>> Definitely agreed on that. Any move function would need to check if
>> the WAL position given by caller is already newer than what's
>> available in the local pg_wal (minimum of all other slots), with a
>> shared lock that would need to be taken by xlog.c when recycling past
>> segments. A forward function works on a single entry, which should be
>> disabled at the moment of the update. It looks dangerous to me to do
>> such an operation if there is a consumer of a slot currently on it.
>>
>>
> pg_advance_replication_slot(...)
>
> ERROR's on logical slot, for now. Physical slots only.
>
> Forward-only.
>
> Future work to allow it to use the logical decoding infrastructure to
> fast-forward a slot by reading only catalog change information and
> invalidations, either via a dummy output plugin that filters out all xacts,
> or by lower level use of the decoding code.
>
> Reasonable?
>
>
PFA an updated and rebased patch.

Rebased. Now named pg_advance_replication_slot. ERROR on logical slots.
Forward only.

I think that, in the end, covered all the comments?

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ <http://www.hagander.net/>
 Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 641b3b8f4e..452559f260 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -19030,6 +19030,24 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
 be called when connected to the same database the slot was created on.

   
+  
+   
+
+ pg_advance_replication_slot
+
+pg_advance_replication_slot(slot_name name, position pg_lsn)
+   
+   
+bool
+   
+   
+Advances the current restart position of a physical
+replication slot named slot_name.
+The slot will not be moved backwards, and it will not be
+moved beyond the current insert location. Logical replication
+slots cannot be moved. Returns true if the position was changed.
+   
+  
 
   

diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index d4cbd83bde..0e27e739bf 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -177,6 +177,73 @@ pg_drop_replication_slot(PG_FUNCTION_ARGS)
 }
 
 /*
+ * SQL function for moving the position in a replication slot.
+ */
+Datum
+pg_advance_replication_slot(PG_FUNCTION_ARGS)
+{
+	Name		slotname = PG_GETARG_NAME(0);
+	XLogRecPtr	moveto = PG_GETARG_LSN(1);
+	bool		changed = false;
+	bool 		backwards = false;
+
+	Assert(!MyReplicationSlot);
+
+	check_permissions();
+
+	if (XLogRecPtrIsInvalid(moveto))
+		ereport(ERROR,
+(errmsg("Invalid target xlog position ")));
+
+	/* Temporarily acquire the slot so we "own" it */
+	ReplicationSlotAcquire(NameStr(*slotname), true);
+
+	/* Only physical slots can be moved */
+	if (MyReplicationSlot->data.database != InvalidOid)
+	{
+		ReplicationSlotRelease();
+		ereport(ERROR,
+(errmsg("Only physical replication slots can be advanced.")));
+	}
+
+	if (moveto > GetXLogWriteRecPtr())
+		/* Can't move past current position, so truncate there */
+		moveto = GetXLogWriteRecPtr();
+
+	/* Now adjust it */
+	SpinLockAcquire(>mutex);
+	if (MyReplicationSlot->data.restart_lsn != moveto)
+	{
+		/* Never move backwards, because bad things can happen */
+		if (MyReplicationSlot->data.restart_lsn > moveto)
+			backwards = true;
+		else
+		{
+			MyReplicationSlot->data.restart_lsn = moveto;
+			changed = true;
+		}
+	}
+	SpinLockRelease(>mutex);
+
+	if (backwards)
+		ereport(WARNING,
+(errmsg("Not moving replication slot backwards!")));
+
+
+	if (changed)
+	{
+		ReplicationSlotMarkDirty();
+		ReplicationSlotsComputeRequiredLSN();
+		ReplicationSlotSave();
+	}
+
+	ReplicationSlotRelease();
+
+	PG_RETURN_BOOL(changed);
+}
+
+
+/*
  * pg_get_replication_slots - SQL SRF showing active replication slots.
  */
 Datum
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 8b33b4e0ea..3495447cf9 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -5293,6 +5293,8 @@ DATA(insert OID = 3779 (  pg_create_physical_replication_slot PGNSP PGUID 12 1 0
 DESCR("create a physical replication slot");
 DATA(insert OID = 3780 (  pg_drop_replication_slot PGNSP PGUID 12 1 0 0 0 f f f f t f v u 1 0 2278 "19" _null_ _null_ _null_ _null_ _null_ pg_drop_replication_slot _null_ _null_ _null_ ));
 DESCR("drop a replication slot");
+DATA(insert OID = 3998 (  pg_

[HACKERS] log_destination=file

[Magnus Hagander]

Attached is a very much VIP (AKA rough draft) for $subject.

Right now we have two parameters controlling logging destination, and they
work in interesting combinations:

log_destination=stderr, logging_collector=off -> log to stderr (makes sense)
log_destination=stderr, logging_collector=on  -> log to file (*highly*
illogical for most users, to set stderr when they want file)
log_destination=csvlog, logging_collector=on -> log to csvfile (makes sense)
log_destination=csvlog, logging_collector=off -> fail (ugh)

(beyond that we also have syslog and eventlog, but those are different and
not touched by this patch)

My understanding is that the main reason for this is that we cannot change
logging_collector without restarting postmaster, whereas we can change
log_destination.

My suggestion is we work around this by just always starting the logging
collector, even if we're not planning to use it. Then we'd just have:

log_destination = stderr -> log to stderr
log_destination = file -> log to file
log_destination = csvlog -> log to csv file

The main difference here is that log_destination=stderr would also go
through the logging collector which would then assemble it and write it out
to it's own stderr.

Do people see an actual problem with that? I agree it's an extra round of
indirection there, but is that a problem? It would also cause one more
backgorund process to always be running, but again, is that really a
problem? The overhead is not exactly large.

It would make configuration a lot more logical for logging. It would also
make it possible to switch between all logging configurations without
restarting.

The attached WIP is mostly working for the simple cases. It fails
completely if the syslogger is restarted at this point, simply because I
haven't figured out how to pass the original stderr down to the syslogger.
I'm sure there are also many other smaller issues around it, but I wanted
to get the discussion done before I spend the time to go through those.

Thoughts?

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ <http://www.hagander.net/>
 Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 95180b2ef5..7c4eb5743f 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -1764,7 +1764,7 @@ ServerLoop(void)
 		}
 
 		/* If we have lost the log collector, try to start a new one */
-		if (SysLoggerPID == 0 && Logging_collector)
+		if (SysLoggerPID == 0)
 			SysLoggerPID = SysLogger_Start();
 
 		/*
diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c
index 3255b42c7d..15fc606d24 100644
--- a/src/backend/postmaster/syslogger.c
+++ b/src/backend/postmaster/syslogger.c
@@ -57,10 +57,8 @@
 
 
 /*
- * GUC parameters.  Logging_collector cannot be changed after postmaster
- * start, but the rest can change at SIGHUP.
+ * GUC parameters. Can change at SIGHUP.
  */
-bool		Logging_collector = false;
 int			Log_RotationAge = HOURS_PER_DAY * MINS_PER_HOUR;
 int			Log_RotationSize = 10 * 1024;
 char	   *Log_directory = NULL;
@@ -182,6 +180,10 @@ SysLoggerMain(int argc, char *argv[])
 	 * assumes that all interesting messages generated in the syslogger will
 	 * come through elog.c and will be sent to write_syslogger_file.
 	 */
+	/*
+	 * XXX: this does not work when we want to use stderr in the syslogger.
+	 * needs fixing!
+	 */
 	if (redirection_done)
 	{
 		int			fd = open(DEVNULL, O_WRONLY, 0);
@@ -370,10 +372,11 @@ SysLoggerMain(int argc, char *argv[])
 		if (!rotation_requested && Log_RotationSize > 0 && !rotation_disabled)
 		{
 			/* Do a rotation if file is too big */
-			if (ftell(syslogFile) >= Log_RotationSize * 1024L)
+			if (syslogFile != NULL &&
+ftell(syslogFile) >= Log_RotationSize * 1024L)
 			{
 rotation_requested = true;
-size_rotation_for |= LOG_DESTINATION_STDERR;
+size_rotation_for |= LOG_DESTINATION_FILE;
 			}
 			if (csvlogFile != NULL &&
 ftell(csvlogFile) >= Log_RotationSize * 1024L)
@@ -390,7 +393,7 @@ SysLoggerMain(int argc, char *argv[])
 			 * was sent by pg_rotate_logfile.
 			 */
 			if (!time_based_rotation && size_rotation_for == 0)
-size_rotation_for = LOG_DESTINATION_STDERR | LOG_DESTINATION_CSVLOG;
+size_rotation_for = LOG_DESTINATION_FILE | LOG_DESTINATION_CSVLOG;
 			logfile_rotate(time_based_rotation, size_rotation_for);
 		}
 
@@ -522,9 +525,6 @@ SysLogger_Start(void)
 	pid_t		sysloggerPid;
 	char	   *filename;
 
-	if (!Logging_collector)
-		return 0;
-
 	/*
 	 * If first time through, create the pipe which will receive stderr
 	 * output.
@@ -581,7 +581,10 @@ SysLogger_Start(void)
 	first_syslogger_file_time = time(NULL);
 	filename = logfile_getname(first_syslogger_file_time, NULL);
 
-	syslogFile = logfile_open(filename, "a", false);
+	if (Log_destination 

Re: [HACKERS] segment size depending *_wal_size defaults (was increasing the default WAL segment size)

[Magnus Hagander]

On Wed, Aug 30, 2017 at 6:45 AM, Andres Freund <and...@anarazel.de> wrote:

> On 2017-08-30 12:52:26 +0900, Michael Paquier wrote:
> > On Wed, Aug 30, 2017 at 11:20 AM, Peter Eisentraut
> > <peter.eisentr...@2ndquadrant.com> wrote:
> > > On 8/29/17 20:36, Andres Freund wrote:
> > >> So the question is whether we want {max,min}_wal_size be sized in
> > >> multiples of segment sizes or as a proper byte size.  I'm leaning
> > >> towards the latter.
> > >
> > > I'm not sure what the question is or what its impact would be.
> >
> > FWIW, I get the question as: do we want the in-memory values of
> > min/max_wal_size to be calculated in MB (which is now the case) or
> > just bytes. Andres tends for using bytes.
>
> Not quite.  There's essentially two things:
>
> 1) Currently the default for {min,max}_wal_size depends on the segment
>size. Given that the segment size is about to be configurable, that
>seems confusing.
> 2) Currently wal_segment_size is measured in GUC_UNIT_XBLOCKS, which
>requires us to keep two copies of the underlying variable, one in
>XBLOCKS one in bytes. I'd rather just have the byte variant.
>

I'd say we definitely want the "user interface" to be in
bytes(/mbytes/gbytes etc). We used to have that in segments and it was
quite confusing for a lot of new uers, and seemed very silly...

Also agreed that (1) above seems very confusing. Going to using bytes all
the way seems a lot more clear.

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

Re: [HACKERS] Not listed as committer

[Magnus Hagander]

On Fri, Aug 25, 2017 at 3:47 PM, Michael Meskes <mes...@postgresql.org>
wrote:

> All,
>
> I just committed a patch that is listed in the CF and had to put
> somebody (I chose you Bruce :)) in as a committer because I was not
> listed. Do I have to register somewhere?
>

Ha, that's interesting.

Should be fixed now, please try again.


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

Re: [HACKERS] shared memory based stat collector (was: Sharing record typmods between backends)

[Magnus Hagander]

On Mon, Aug 14, 2017 at 5:46 PM, Robert Haas <robertmh...@gmail.com> wrote:

> On Sun, Aug 13, 2017 at 9:59 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> >> b) I think our tendency to dump all stats whenever we crash isn't really
> >>tenable, given how autovacuum etc are tied to them.
> >
> > Eh, maybe.  I don't think crashes are really so common on production
> > systems.  As developers we have an inflated view of their frequency ;-)
>

>From a stats perspective I think the crash problem is the small problem.
The big problem is we nuke them on replication promotion and we nuke them
on PITR. If we solve those two, I'm pretty sure we would also solve the
on-crash more or less in the same thing.



> Without taking a position on the point under debate, AFAIK it wouldn't
> be technically complex either under our current architecture or the
> proposed new one to dump out a new permanent stats file every 10
> minutes or so.  So if there is an issue here I think it might not be
> very hard to fix, whatever else we do.
>

I started working on that patch at some point, I think I have a rotting
branch somewhere. That part was indeed fairly easy.

I got stalled when I feature-crept myself by realizing I wanted it
snapshotted to WAL so it could fix the PITR and replication issues. And
then properly bogged down when I realized that on the standby I'd want
*both* the stats from the standby (while it's running) and the stats from
the master (after switchover).


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

Re: [HACKERS] Patches I'm thinking of pushing shortly

[Magnus Hagander]

On Sun, Aug 13, 2017 at 11:43 PM, Robert Haas <robertmh...@gmail.com> wrote:

> On Sun, Aug 13, 2017 at 5:24 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> >> I'd vote for including this in v10.  There doesn't seem to be any
> >> downside to this: it's a no brainer to avoid our exploding hash table
> >> case when we can see it coming.
> >
> > Anybody else want to vote that way?  For myself it's getting a bit late
> > in the beta process to be including inessential changes, but I'm willing
> > to push it to v10 not just v11 if there's multiple people speaking for
> > that.
>
> I'd vote for waiting until v11.  I think it's too late to be doing
> things that might change good plans into bad ones or visca versa;
> that's a recipe for having to put out 10.1 and 10.2 a little quicker
> than I'd like.
>

+1 for waiting until v11 with it.

We have plenty enough other things that could end up needing a quick
post-release-release, and those are things that have received at least
somem testing...

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

Re: [HACKERS] More flexible LDAP auth search filters?

[Magnus Hagander]

On Mon, Jul 17, 2017 at 6:47 PM, Mark Cave-Ayland <
mark.cave-ayl...@ilande.co.uk> wrote:

> On 17/07/17 13:09, Magnus Hagander wrote:
>
> Hi Magnus,
>
> Great to hear from you! It has definitely been a while...
>

Indeed. You should spend more time on these lists :P



>
> > Generally you find that you will be given the option to set the
> > attribute for the default search filter of the form
> > "(attribute=username)" which defaults to uid for UNIX-type systems
> and
> > sAMAccountName for AD. However there is always the ability to
> specify a
> > custom filter where the user is substituted via e.g. %u to cover all
> the
> > other use-cases.
> >
> > Right, but that's something we do already today. It just defaults to
> > uid, but it's easy to change.
>
> Yes, I think that bit is fine as long as the default can be overridden.
> There's always a choice as to whether the defaults favour a POSIX-based
> LDAP or AD environment but that happens with all installations so it's
> not a big deal.
>

It's easy enough to change.



> > As an example, I don't know if anyone would actually do this with
> > PostgreSQL but I've been asked on multiple occasions to configure
> > software so that users should be allowed to log in with either their
> > email address or username which is easily done with a custom LDAP
> filter
> > like "(|(mail=%u)(uid=%u))".
> >
> >
> > How would that actually work, though? Given the same user in ldap could
> > now potentially match multiple different users in PostgreSQL. Would you
> > then create two accounts for the user, one with the uid as name and one
> > with email as name? Wouldn't that actually cause more issues than it
> solves?
>
> Normally what happens for search+bind is that you execute the custom
> filter with substitutions in order to find the entry for your bind DN,
> but you also request the value of ldapsearchattribute (or equivalent) at
> the same time. Say for example you had an entry like this:
>
> dn: uid=mha,dc=users,dc=hagander,dc=net
> uid: mha
> mail: mag...@hagander.net
>
> Using the filter "(|(mail=%u)(uid=%u))" would locate the same bind DN
> "uid=mha,dc=users,dc=hagander,dc=net" with either one of your uid or
> email address.
>
> If the bind is successful then the current user identity should be set
> to the value of the ldapsearchattribute retrieved from the bind DN
> entry, which with the default of "uid" would be "mha". Hence you end up
> with the same user role "mha" regardless of whether a uid or email
> address was entered.
>

Right. This is the part that doesn't work for PostgreSQL. Because we have
already specified the username -- it goes in the startup packet in order to
pick the correct row from pg_hba.conf.

I guess in theory we could treat it like Kerberos or another one of the
systems where we get the username from an external entity. But then you'd
still have to specify the mapping again in pg_ident.conf, and it would be a
pretty strong break of backwards compatibility.



> In terms of matching multiple users, all LDAP authentication routines
> I've seen will fail if more than one DN matches the search filter, so
> this nicely handles the cases where either the custom filter is
> incorrect or more than one entry is accidentally matched in the directory.
>

So do we, in the current implementation. But it's a lot less likely to
happen in the current implementation, since we do a single equals lookup.

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

Re: [HACKERS] More flexible LDAP auth search filters?

[Magnus Hagander]

On Mon, Jul 17, 2017 at 1:23 AM, Stephen Frost <sfr...@snowman.net> wrote:

>
> * Magnus Hagander (mag...@hagander.net) wrote:
> > On Sun, Jul 16, 2017 at 11:05 PM, Stephen Frost <sfr...@snowman.net>
> wrote:
> > > I'd suggest that we try to understand why Kerberos couldn't be used in
> > > that environment.  I suspect in at least some cases what users would
> > > like is the ability to do Kerberos auth but then have LDAP checked to
> > > see if a given user (who has now auth'd through Kerberos) is allowed to
> > > connect.  We don't currently have any way to do that, but if we were
> > > looking for things to do, that's what I'd suggest working on rather
> than
> > > adding more to our LDAP auth system and implying by doing so that it's
> > > reasonable to use.
> > >
> > > I find it particularly disappointing to see recommendations for using
> > > LDAP auth, particularly in AD environments, that don't even mention
> > > Kerberos or bother to explain how using LDAP sends the user's PW to the
> > > server in cleartext.
> >
> > You do realize, I'm sure, that there are many LDAP servers out there that
> > are not AD, and that do not come with a Kerberos server attached to
> them...
>
> I am aware that some exist, I've even contributed to their development
> and packaging, but that doesn't make it a good idea to use them for
> authentication.
>

Pretty sure that doesn't include any of the ones I'm talking about, but
sure :)



> > I agree that Kerberos is usually the better choice *if it's available*.
>
> Which is the case in an AD environment..
>

Yes. But we shouldn't force everybody to use AD :P



> > It's several orders of magnitude more complicated to set up though, and
> > there are many environments that have ldap but don't have Kerberos.
>
> Frankly, I simply don't agree with this.
>

Really?

For LDAP auth I don't need to do *anything* in preparation. For Kerberos
auth I need to create an account, set encryption type, export keys, etc.
You can't possibly claim this is the same level of complexity?




> > Refusing to improve LDAP for the users who have no choice seems like a
> very
> > unfriendly thing to do.
>
> I'm fine with improving LDAP in general, but, as I tried to point out,
> having a way to make it easier to integrate PG into an AD environment
> would be better.  It's not my intent to stop this patch but rather to
> point out the issues with LDAP auth that far too frequently are not
> properly understood.
>

A documentation patch for that would certainly be a good place to start.
Perhaps with up to date instructions for how to actually set up Kerberos in
an AD environment including all steps required?



> > (And you can actually reasonably solve the case of
> > kerberos-for-auth-ldap-for-priv by syncing the groups into postgres
> roles)
>
> Yes, but sync'ing roles is a bit of a pain and it'd be nice if we could
> avoid it, or perhaps make it easier.
>

Certainly.

//Magnus

Re: [HACKERS] More flexible LDAP auth search filters?

[Magnus Hagander]

On Sun, Jul 16, 2017 at 7:58 PM, Mark Cave-Ayland <
mark.cave-ayl...@ilande.co.uk> wrote:

> On 16/07/17 00:08, Thomas Munro wrote:
>
> > On Fri, Jul 14, 2017 at 11:04 PM, Magnus Hagander <mag...@hagander.net>
> wrote:
> >> On Thu, Jul 13, 2017 at 9:31 AM, Thomas Munro
> >> <thomas.mu...@enterprisedb.com> wrote:
> >>> A post on planet.postgresql.org today reminded me that a colleague had
> >>> asked me to post this POC patch here for discussion.  It allows custom
> >>> filters with ldapsearchprefix and ldapsearchsuffix.  Another approach
> >>> might be to take a filter pattern with "%USERNAME%" or whatever in it.
> >>> There's an existing precedent for the prefix and suffix approach, but
> >>> on the other hand a pattern approach would allow filters where the
> >>> username is inserted more than once.
> >>
> >>
> >> Do we even need prefix/suffix? If we just make it "ldapsearchpattern",
> then
> >> you could have something like:
> >>
> >> ldapsearchattribute="uid"
> >> ldapsearchfilter="|(memberof=cn=Paris DBA Team)(memberof=cn=Tokyo DBA
> Team)"
> >>
> >> We could then always to substitution of the kind:
> >> (&(attr=)())
> >>
> >> which would in this case give:
> >> (&(uid=mha)(|(memberof=cn=Paris DBA Team)(memberof=cn=Tokyo DBA Team)))
> >>
> >>
> >> Basically we'd always AND together the username lookup with the
> additional
> >> filter.
> >
> > Ok, so we have 3 ideas put forward:
> >
> > 1.  Wrap username with ldapsearchprefix ldapsearchsuffix to build
> > filter (as implemented by POC patch)
> > 2.  Optionally AND ldapsearchfilter with the existing
> > ldapsearchattribute-based filter (Magnus's proposal)
> > 3.  Pattern-based ldapsearchfilter so that %USER% is replaced with
> > username (my other suggestion)
> >
> > The main argument for approach 1 is that it follows the style of the
> > bind-only mode.
> >
> > With idea 2, I wonder if there are some more general kinds of things
> > that people might want to do that that wouldn't be possible because it
> > has to include (attribute=user)... perhaps something involving a
> > substring or other transformation functions (but I'm no LDAP expert,
> > that may not make sense).
> >
> > With idea 3 it would allow "(|(foo=%USER%)(bar=%USER%))", though I
> > don't know if any such multiple-mention filters would ever make sense
> > in a sane LDAP configuration.
> >
> > Any other views from LDAP-users?
>
> I've spent quite a bit of time integrating various bits of
> non-PostgreSQL software to LDAP and in my experience option 3 tends to
> be the standard.
>
> Generally you find that you will be given the option to set the
> attribute for the default search filter of the form
> "(attribute=username)" which defaults to uid for UNIX-type systems and
> sAMAccountName for AD. However there is always the ability to specify a
> custom filter where the user is substituted via e.g. %u to cover all the
> other use-cases.


Right, but that's something we do already today. It just defaults to uid,
but it's easy to change.



>
> As an example, I don't know if anyone would actually do this with
> PostgreSQL but I've been asked on multiple occasions to configure
> software so that users should be allowed to log in with either their
> email address or username which is easily done with a custom LDAP filter
> like "(|(mail=%u)(uid=%u))".
>

How would that actually work, though? Given the same user in ldap could now
potentially match multiple different users in PostgreSQL. Would you then
create two accounts for the user, one with the uid as name and one with
email as name? Wouldn't that actually cause more issues than it solves?

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

Re: [HACKERS] More flexible LDAP auth search filters?

[Magnus Hagander]

On Sun, Jul 16, 2017 at 11:05 PM, Stephen Frost <sfr...@snowman.net> wrote:

> Magnus, all,
>
> * Magnus Hagander (mag...@hagander.net) wrote:
> > (FWIW, a workaround I've applied more than once to this in AD
> environments
> > (where kerberos for one reason or other can't be done, sorry Stephen) is
> to
> > set up a RADIUS server and use that one as a "middle man". But it would
> be
> > much better if we could do it natively)
>
> I'd suggest that we try to understand why Kerberos couldn't be used in
> that environment.  I suspect in at least some cases what users would
> like is the ability to do Kerberos auth but then have LDAP checked to
> see if a given user (who has now auth'd through Kerberos) is allowed to
> connect.  We don't currently have any way to do that, but if we were
> looking for things to do, that's what I'd suggest working on rather than
> adding more to our LDAP auth system and implying by doing so that it's
> reasonable to use.
>
> I find it particularly disappointing to see recommendations for using
> LDAP auth, particularly in AD environments, that don't even mention
> Kerberos or bother to explain how using LDAP sends the user's PW to the
> server in cleartext.
>

You do realize, I'm sure, that there are many LDAP servers out there that
are not AD, and that do not come with a Kerberos server attached to them...

I agree that Kerberos is usually the better choice *if it's available*.
It's several orders of magnitude more complicated to set up though, and
there are many environments that have ldap but don't have Kerberos.

Refusing to improve LDAP for the users who have no choice seems like a very
unfriendly thing to do.

(And you can actually reasonably solve the case of
kerberos-for-auth-ldap-for-priv by syncing the groups into postgres roles)

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

Re: [HACKERS] More flexible LDAP auth search filters?

[Magnus Hagander]

On Sun, Jul 16, 2017 at 1:08 AM, Thomas Munro <thomas.mu...@enterprisedb.com
> wrote:

> On Fri, Jul 14, 2017 at 11:04 PM, Magnus Hagander <mag...@hagander.net>
> wrote:
> > On Thu, Jul 13, 2017 at 9:31 AM, Thomas Munro
> > <thomas.mu...@enterprisedb.com> wrote:
> >> A post on planet.postgresql.org today reminded me that a colleague had
> >> asked me to post this POC patch here for discussion.  It allows custom
> >> filters with ldapsearchprefix and ldapsearchsuffix.  Another approach
> >> might be to take a filter pattern with "%USERNAME%" or whatever in it.
> >> There's an existing precedent for the prefix and suffix approach, but
> >> on the other hand a pattern approach would allow filters where the
> >> username is inserted more than once.
> >
> >
> > Do we even need prefix/suffix? If we just make it "ldapsearchpattern",
> then
> > you could have something like:
> >
> > ldapsearchattribute="uid"
> > ldapsearchfilter="|(memberof=cn=Paris DBA Team)(memberof=cn=Tokyo DBA
> Team)"
> >
> > We could then always to substitution of the kind:
> > (&(attr=)())
> >
> > which would in this case give:
> > (&(uid=mha)(|(memberof=cn=Paris DBA Team)(memberof=cn=Tokyo DBA Team)))
> >
> >
> > Basically we'd always AND together the username lookup with the
> additional
> > filter.
>
> Ok, so we have 3 ideas put forward:
>
> 1.  Wrap username with ldapsearchprefix ldapsearchsuffix to build
> filter (as implemented by POC patch)
> 2.  Optionally AND ldapsearchfilter with the existing
> ldapsearchattribute-based filter (Magnus's proposal)
> 3.  Pattern-based ldapsearchfilter so that %USER% is replaced with
> username (my other suggestion)
>
> The main argument for approach 1 is that it follows the style of the
> bind-only mode.
>

Agreed, but I'm not sure it's a good style to follow (and yes, I think I
may be the original author of it..). I'd rank option 3 over option 1.


>
> With idea 2, I wonder if there are some more general kinds of things
> that people might want to do that that wouldn't be possible because it
> has to include (attribute=user)... perhaps something involving a
> substring or other transformation functions (but I'm no LDAP expert,
> that may not make sense).
>

Yeah, that's exactly what I'm wondering about it :)



>
> With idea 3 it would allow "(|(foo=%USER%)(bar=%USER%))", though I
> don't know if any such multiple-mention filters would ever make sense
> in a sane LDAP configuration.
>
> Any other views from LDAP-users?
>
>

+1 for some input from people who directly use it in larger LDAP
environments. If we're going to change how it works, let's make it right
this time!

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

Re: [HACKERS] More flexible LDAP auth search filters?

[Magnus Hagander]

On Thu, Jul 13, 2017 at 9:31 AM, Thomas Munro <thomas.mu...@enterprisedb.com
> wrote:

> Hi hackers,
>
> A customer asked how to use pg_hba.conf LDAP search+bind
> authentication to restrict logins to users in one of a small number of
> groups.  ldapsearchattribute only lets you make filters like
> "(foo=username)", so it couldn't be done.  Is there any reason we
> should allow a more general kind of search filter constructions?
>
> A post on planet.postgresql.org today reminded me that a colleague had
> asked me to post this POC patch here for discussion.  It allows custom
> filters with ldapsearchprefix and ldapsearchsuffix.  Another approach
> might be to take a filter pattern with "%USERNAME%" or whatever in it.
> There's an existing precedent for the prefix and suffix approach, but
> on the other hand a pattern approach would allow filters where the
> username is inserted more than once.
>

Do we even need prefix/suffix? If we just make it "ldapsearchpattern", then
you could have something like:

ldapsearchattribute="uid"
ldapsearchfilter="|(memberof=cn=Paris DBA Team)(memberof=cn=Tokyo DBA Team)"

We could then always to substitution of the kind:
(&(attr=)())

which would in this case give:
(&(uid=mha)(|(memberof=cn=Paris DBA Team)(memberof=cn=Tokyo DBA Team)))


Basically we'd always AND together the username lookup with the additional
filter.


Perhaps there are better ways to organise your LDAP servers so that
> this sort of thing isn't necessary.  I don't know.  Thoughts?
>

I think something along this way is definitely wanted. We can argue the
syntax, but being able to filter like this is definitely useful.

(FWIW, a workaround I've applied more than once to this in AD environments
(where kerberos for one reason or other can't be done, sorry Stephen) is to
set up a RADIUS server and use that one as a "middle man". But it would be
much better if we could do it natively)

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

[HACKERS] Re: retry shm attach for windows (WAS: Re: OK, so culicidae is *still* broken)

[Magnus Hagander]

On Jul 10, 2017 16:08, "Tom Lane"  wrote:

Noah Misch  writes:
> On Mon, Jun 05, 2017 at 09:56:33AM -0400, Tom Lane wrote:
>> Yeah, being able to reproduce the problem reliably enough to say whether
>> it's fixed or not is definitely the sticking point here.  I have some
>> ideas about that: ...

> I tried this procedure without finding a single failure.

Thanks for testing!  But apparently we still lack some critical part
of the recipe for getting failures in the wild.

> I watched the ensuing memory maps, which led me to these interesting
facts:

>   An important result of the ASLR design in Windows Vista is that some
address
>   space layout parameters, such as PEB, stack, and heap locations, are
>   selected once per program execution. Other parameters, such as the
location
>   of the program code, data segment, BSS segment, and libraries, change
only
>   between reboots.
>   ...
>   This offset is selected once per reboot, although we’ve uncovered at
least
>   one other way to cause this offset to be reset without a reboot (see
>   Appendix II).
>   -- http://www.symantec.com/avcenter/reference/Address_
Space_Layout_Randomization.pdf

That is really interesting, though I'm not quite sure what to make of it.
It suggests though that you might need certain types of antivirus products
to be active, to create more variability in the initial process address
space layout than would happen from Windows ASLR alone.

> I recommend pushing your patch so the August back-branch releases have it.
> One can see by inspection that your patch has negligible effect on systems
> healthy today.  I have a reasonable suspicion it will help some systems.
If
> others remain broken after this, that fact will provide a useful clue.

Okay, so that leaves us with a decision to make: push it into beta2, or
wait till after wrap?  I find it pretty scary to push a patch with
portability implications so soon before wrap, but a quick look at the
buildfarm suggests we'd get runs from 3 or 4 Windows members before the
wrap, if I do it quickly.  If we wait, then it will hit the field in
production releases with reasonable buildfarm testing but little more.
That's also pretty scary.

On balance I'm tempted to push it now for beta2, but it's a close call.
Thoughts?


Given the lack of windows testing on non packaged releases I think we
should definitely push this for beta2. That will give it a much better real
world testing on what is still a beta.

If it breaks its a lot better to do it in beta2 (and possibly quickly roll
a beta3) than in production.

/Magnus

Re: [HACKERS] pg_receivewal and messages printed in non-verbose mode

[Magnus Hagander]

On Wednesday, June 14, 2017, Michael Paquier <michael.paqu...@gmail.com>
wrote:

> On Tue, Jun 13, 2017 at 4:50 PM, Craig Ringer <cr...@2ndquadrant.com
> <javascript:;>> wrote:
> > On 13 June 2017 at 14:33, Michael Paquier <michael.paqu...@gmail.com
> <javascript:;>> wrote:
> >> Those come from stop_streaming in pg_receivewal.c. Shouldn't those
> >> messages only show up to the user if --verbose is used? It seems
> >> strange to me that at least the first one is written to the user as
> >> that's not an error after promoting a standby.
> >
> > I agree. At least the first should be --verbose only.
>
> I have been looking at all the code surrounding pg_receivewal and
> pg_recvlogical and those are indeed the two only places where we print
> a message in non-verbose mode even if those are not explicit errors.
> pg_recvlogical does not show up any messages when it is signaled or
> when it receives SIGINT or reaches the end of LSN position. I don't
> think that this is worth complicating the code for, just noticed the
> inconsistency on the way.
>
> Perhaps a committer will care about that. Or not. For now I am just
> adding that in the CF.
>

I agree that this should be fixed.

I wonder if we should actually just remove the second message? AFAICT no
other tools log that information. Is there any particular reason why we
want that logging in pg_receivewal when we don't have it in other tools?

//Magnus



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

Re: [HACKERS] Fix header comment of streamutil.c

[Magnus Hagander]

On Fri, Jul 7, 2017 at 8:31 AM, Masahiko Sawada <sawada.m...@gmail.com>
wrote:

> Hi,
>
> While reading source code, I found that the header comment of
> streamutil.c is not correct. I guess pg_receivelog is a mistake of
> pg_receivexlog and it's an oversight when changing xlog to wal.
>
> Attached patch fixes this.
>

Yeah, both a typo and a missing user :)

Applied, thanks.

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

Re: [HACKERS] pg_stop_backup(wait_for_archive := true) on standby server

[Magnus Hagander]

On Thu, Jun 22, 2017 at 6:22 PM, Masahiko Sawada <sawada.m...@gmail.com>
wrote:

> On Thu, Jun 22, 2017 at 10:36 PM, Magnus Hagander <mag...@hagander.net>
> wrote:
> >
> >
> > On Thu, Jun 22, 2017 at 10:12 AM, Masahiko Sawada <sawada.m...@gmail.com
> >
> > wrote:
> >>
> >> Hi,
> >>
> >> Since an optional second argument wait_for_archive of pg_stop_backup
> >> has been  introduced in PostgreSQL 10 we can choose whether wait for
> >> archiving. But my colleagues found that we can do pg_stop_backup with
> >> wait_for_archive = true on the standby server but it actually doesn't
> >> wait for WAL archiving. Because this behavior is not documented and we
> >> cannot find out it without reading source code it will confuse the
> >> user.
> >>
> >> I think we can raise an error when pg_stop_backup with
> >> wait_for_archive = true is executed on the standby. Attached patch
> >> change it so that.
> >
> >
> > Wouldn't it be better to make it *work*? If you have
> archive_mode=always, it
> > makes sense to want to wait on the standby as well, does it not?
> >
>
> Yes, ideally it will be better to make it wait for WAL archiving on
> standby server when archive_mode=always. But I think it would be for
> PG11 item, and this item is for PG10.
>
>
I'm not sure. I think this can be considered a bug in the implementation
for 10, and as such is "open for fixing". However, it's not a very critical
bug so I doubt it should be a release blocker, but if someone wants to work
on a fix I think we should commit it.

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

Re: [HACKERS] Fix a typo in partition.c

[Magnus Hagander]

On Thu, Jun 22, 2017 at 4:16 AM, Masahiko Sawada <sawada.m...@gmail.com>
wrote:

> On Wed, Jun 21, 2017 at 3:32 AM, Peter Eisentraut
> <peter.eisentr...@2ndquadrant.com> wrote:
> > On 6/19/17 23:02, Masahiko Sawada wrote:
> >> Hi,
> >>
> >> Attached patch for $subject.
> >>
> >> s/opreator/operator/
> >
> > fixed
> >
>
> Thank you!
> I found another one.
>
> s/retrived/retrieved/
>
>
Thanks, applied.


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

Re: [HACKERS] pg_stop_backup(wait_for_archive := true) on standby server

[Magnus Hagander]

On Thu, Jun 22, 2017 at 10:12 AM, Masahiko Sawada <sawada.m...@gmail.com>
wrote:

> Hi,
>
> Since an optional second argument wait_for_archive of pg_stop_backup
> has been  introduced in PostgreSQL 10 we can choose whether wait for
> archiving. But my colleagues found that we can do pg_stop_backup with
> wait_for_archive = true on the standby server but it actually doesn't
> wait for WAL archiving. Because this behavior is not documented and we
> cannot find out it without reading source code it will confuse the
> user.
>
> I think we can raise an error when pg_stop_backup with
> wait_for_archive = true is executed on the standby. Attached patch
> change it so that.
>

Wouldn't it be better to make it *work*? If you have archive_mode=always,
it makes sense to want to wait on the standby as well, does it not?

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

Re: [HACKERS] Typo in drop_publication.sgml

[Magnus Hagander]

On Sun, Jun 18, 2017 at 8:46 AM, Julien Rouhaud <julien.rouh...@dalibo.com>
wrote:

> Patch attached.
>

Applied, thanks.

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

Re: [HACKERS] Incorrect comment in 001_ssltests.pl

[Magnus Hagander]

On Sat, Jun 17, 2017 at 1:53 AM, Michael Paquier <michael.paqu...@gmail.com>
wrote:

> Hi all,
>
> I have noticed the following thing:
> --- a/src/test/ssl/t/001_ssltests.pl
> +++ b/src/test/ssl/t/001_ssltests.pl
> @@ -34,8 +34,6 @@ sub run_test_psql
>  # The first argument is a (part of a) connection string, and it's also
> printed
>  # out as the test case name. It is appended to $common_connstr global
> variable,
>  # which also contains a libpq connection string.
> -#
> -# The second argument is a hostname to connect to.
>  sub test_connect_ok
>  {
> my $connstr = $_[0]
>
> But test_connect_ok and test_connect_fails do not have a second
> argument as the hostname is appended directly into $common_connstr.
>

One could argue that the first sentences should just read "the argument"
once the second one isn't referred, but I can't get too excited about that.
Thus, patch applied as-is - thanks!

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

Re: [HACKERS] Typos in comments

[Magnus Hagander]

On Sat, Jun 17, 2017 at 8:55 AM, Daniel Gustafsson <dan...@yesql.se> wrote:

> Spotted one “paramter” typo and git grep found two more, patch attached
> with
> s/paramter/parameter/ for these.
>

Applied, thanks.

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

Re: [HACKERS] Making server name part of the startup message

[Magnus Hagander]

On Thu, Jun 15, 2017 at 5:57 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Alvaro Herrera <alvhe...@2ndquadrant.com> writes:
> > Tom Lane wrote:
> >> This makes no sense at all.  The client is telling the server what the
> >> server's name is?
>
> > I think for instance you could have one pgbouncer instance (or whatever
> > pooler) pointing to several different servers.  So the client connects
> > to the pooler and indicates which of the servers to connect to.
>
> I should think that in such cases, the end client is exactly not what
> you want to be choosing which server it gets redirected to.  You'd
> be wanting to base that on policies defined at the pooler.  There are
> already plenty of client-supplied attributes you could use as inputs
> for such policies (user name and application name, for instance).
> Why do we need to incur a protocol break to add another one?
>

The normal one to use for pgbonucer today is, well, "database name". You
can then have pgbouncer map different databases to different backend
servers. It's fairly common in my experience to have things like "dbname"
and "dbname-ro" (for example) as different database names with one mapping
to the master and one mapping to a load-balanced set of standbys, and
things like that. ISTM that using the database name is a good choice for
that.

For the original idea in this thread, using something like dbname@server
seems a more logical choice than username@server.

TBH, so maybe I'm misunderstanding the original issue?

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

Re: [HACKERS] Why does logical replication launcher set application_name?

[Magnus Hagander]

On Wed, Jun 7, 2017 at 4:58 AM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 6/6/17 15:58, Robert Haas wrote:
> > The problem with the status quo (after Peter's commit) is that there's
> > now nothing at all to identify the logical replication launcher, apart
> > from the wait_event field, which is likely to be LogicalLauncherMain
> > fairly often if you've got the launcher.  I don't personally see why
> > we can't simply adopt Tom's original proposal of setting the query
> > string to something like "" which, while
> > maybe not as elegant as providing some way to override the
> > backend_type field, would be almost no work and substantially better
> > for v10 than what we've got now.
>
> The decision was made to add background workers to pg_stat_activity, but
> no facility was provided to tell the background workers apart.  Is it
> now the job of every background worker to invent a hack to populate some
> other pg_stat_activity field with some ad hoc information?  What about
> other logical replication worker types, parallel workers, external
> background workers, auto-prewarm?
>
> I think the bgw_type addition that I proposed nearby would solve this
> quite well, but it needs a bit of work.  And arguably, it's too late for
> PG10, but one could also argue that this is a design fault in the
> pg_stat_activity extension that is valid to fix in PG10.
>

+1. I definitely think it would be a bad idea to put in what basically
looks like a workaround into 10, since the new feature was added there. I'd
rather have the fix for pg_stat_activity.

We used to keep our query state as a text field and that was a bad idea for
many reasons. So we moved it to a separate field. Let's not repeat that
mistake here.

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

Re: retry shm attach for windows (WAS: Re: [HACKERS] OK, so culicidae is *still* broken)

[Magnus Hagander]

On Mon, Jun 5, 2017 at 1:35 PM, Amit Kapila <amit.kapil...@gmail.com> wrote:

> On Mon, Jun 5, 2017 at 4:58 PM, Magnus Hagander <mag...@hagander.net>
> wrote:
> > On Mon, Jun 5, 2017 at 1:16 PM, Amit Kapila <amit.kapil...@gmail.com>
> wrote:
> >>
> >> On Mon, Jun 5, 2017 at 9:15 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> >> > Amit Kapila <amit.kapil...@gmail.com> writes:
> >> >
> >> >> I think the same problem can happen during reattach as well.
> >> >> Basically, MapViewOfFileEx can fail to load image at predefined
> >> >> address (UsedShmemSegAddr).
> >> >
> >> > Once we've successfully done the VirtualAllocEx call, that should hold
> >> > until the VirtualFree call in PGSharedMemoryReAttach, allowing the
> >> > immediately-following MapViewOfFileEx call to succeed.  Were that not
> >> > the
> >> > case, we'd have had problems even without ASLR.  We did have problems
> >> > exactly like that before the pgwin32_ReserveSharedMemoryRegion code
> was
> >> > added.
> >> >
> >>
> >> I could not find anything directly in specs which could prove the
> >> theory either way.  However, in one of the StackOverflow discussions,
> >> it has been indicated that MapViewOfFile can opt to load the image at
> >> an address different than the predefined address due to ASLR.
> >>
> >> >  So my feeling about this is that retrying the process creation as
> >> > in my prototype patch ought to be sufficient; if you think it isn't,
> the
> >> > burden of proof is on you.
> >> >
> >>
> >> Sure.  I think it is slightly tricky because specs don't say clearly
> >> how ASLR can impact the behavior of any API and in my last attempt I
> >> could not reproduce the issue.
> >>
> >> I can try to do basic verification with the patch you have proposed,
> >> but I fear, to do the actual tests as suggested by you is difficult as
> >> it is not reproducible on my machine, but I can still try.
> >>
> >>
> >> [1] -
> >> https://stackoverflow.com/questions/9718616/what-does-
> mapviewoffile-return/11233456
> >> Refer below text:
> >>
> >> "Yes, MapViewOfFile returns the virtual memory base address where the
> >> image has been loaded. The value (content) of this address depends on
> >> whether the image has been successfully loaded at its predefined
> >> address (which has been setup by the linker) or whether the image has
> >> been relocated (because the desired, predefined address is already
> >> occupied or because the image has opt-in to support ASLR)."
> >>
> >
> > That statements refers to mapping executables though, like DLL and EXE.
> Not
> > mapping of data segments.
> >
> > It does randomize the entire location of the heap, in which case it might
> > also change. But not for the individual block.
> >
> > But in neither of those cases does it help to retry without restarting
> the
> > process,
> >
>
> Okay, the question here is do we need some handling during reattach
> operation where we do MapViewOfFileEx at the predefined location?
>
>
As long as we restart in a new process, I don't think so. ASLR does not
randomize away our addresses *after we've been given them*. It does it at
process (or thread start), so as long as we get it initially, I think we
are safe.

(Obviously nobody can be 100% sure without seeing the source code for it,
but all references I've seen to the implementation seem to indicate that)

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

Re: retry shm attach for windows (WAS: Re: [HACKERS] OK, so culicidae is *still* broken)

[Magnus Hagander]

On Mon, Jun 5, 2017 at 1:16 PM, Amit Kapila <amit.kapil...@gmail.com> wrote:

> On Mon, Jun 5, 2017 at 9:15 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> > Amit Kapila <amit.kapil...@gmail.com> writes:
> >
> >> I think the same problem can happen during reattach as well.
> >> Basically, MapViewOfFileEx can fail to load image at predefined
> >> address (UsedShmemSegAddr).
> >
> > Once we've successfully done the VirtualAllocEx call, that should hold
> > until the VirtualFree call in PGSharedMemoryReAttach, allowing the
> > immediately-following MapViewOfFileEx call to succeed.  Were that not the
> > case, we'd have had problems even without ASLR.  We did have problems
> > exactly like that before the pgwin32_ReserveSharedMemoryRegion code was
> > added.
> >
>
> I could not find anything directly in specs which could prove the
> theory either way.  However, in one of the StackOverflow discussions,
> it has been indicated that MapViewOfFile can opt to load the image at
> an address different than the predefined address due to ASLR.
>
> >  So my feeling about this is that retrying the process creation as
> > in my prototype patch ought to be sufficient; if you think it isn't, the
> > burden of proof is on you.
> >
>
> Sure.  I think it is slightly tricky because specs don't say clearly
> how ASLR can impact the behavior of any API and in my last attempt I
> could not reproduce the issue.
>
> I can try to do basic verification with the patch you have proposed,
> but I fear, to do the actual tests as suggested by you is difficult as
> it is not reproducible on my machine, but I can still try.
>
>
> [1] - https://stackoverflow.com/questions/9718616/what-does-
> mapviewoffile-return/11233456
> Refer below text:
>
> "Yes, MapViewOfFile returns the virtual memory base address where the
> image has been loaded. The value (content) of this address depends on
> whether the image has been successfully loaded at its predefined
> address (which has been setup by the linker) or whether the image has
> been relocated (because the desired, predefined address is already
> occupied or because the image has opt-in to support ASLR)."
>
>
That statements refers to mapping executables though, like DLL and EXE. Not
mapping of data segments.

It does randomize the entire location of the heap, in which case it might
also change. But not for the individual block.

But in neither of those cases does it help to retry without restarting the
process, because the heap will be mapped into the same place, and any DLLs
loading prior to our code will already have been loaded.

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

Re: [HACKERS] comment fix in attoptcache.c

[Magnus Hagander]

On Fri, Jun 2, 2017 at 11:14 AM, Amit Langote <langote_amit...@lab.ntt.co.jp
> wrote:

> Noticed a comment copy-pasted from spccache.c, which attached patch fixes
> to match the file.
>
>  /*
>   * InitializeAttoptCache
> - *  Initialize the tablespace cache.
> + *  Initialize the attribute options cache.
>   */
>

Applied, thanks.


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

Re: [HACKERS] Fix a typo in predicate.c

[Magnus Hagander]

On Fri, Jun 2, 2017 at 7:32 AM, Masahiko Sawada <sawada.m...@gmail.com>
wrote:

> Hi,
>
> While reading predicate lock source code, I found a comment typo in
> predicate.c file.
> Attached patch fixes it.
>
> s/scrach/scratch/
>

Applied, thanks.

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

Re: [HACKERS] Re: [GENERAL] pg_basebackup error: replication slot "pg_basebackup_2194" already exists

[Magnus Hagander]

On Wed, May 31, 2017 at 8:18 PM, Andres Freund <and...@anarazel.de> wrote:

> On 2017-05-31 18:22:18 +0200, Magnus Hagander wrote:
> > However, the client can't access the pid of the server as it is now,
> > and its the client that has to create the name.
>
> I don't think that's actually true?  Doesn't the wire protocol always
> include the PID, which is then exposed with PQbackendPID()?
>

Oh, you're right. Well, that makes the fix a lot simpler. Will fix in a
minute.


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

[HACKERS] Re: [GENERAL] pg_basebackup error: replication slot "pg_basebackup_2194" already exists

[Magnus Hagander]

On Wed, May 31, 2017 at 12:20 AM, Ludovic Vaugeois-Pepin <
ludovi...@gmail.com> wrote:

> On Tue, May 30, 2017 at 9:32 PM, Magnus Hagander <mag...@hagander.net>
> wrote:
> > On Tue, May 30, 2017 at 9:14 PM, Ludovic Vaugeois-Pepin
> > <ludovi...@gmail.com> wrote:
> >>
> >> I ran into the issue described below with 10.0 beta. The error I got is:
> >>
> >> pg_basebackup: could not create temporary replication slot
> >> "pg_basebackup_2194": ERROR:  replication slot "pg_basebackup_2194"
> >> already exists
> >>
> >> A race condition? Or maybe I am doing something wrong.
> >>
> >>
> >>
> >>
> >>
> >> Release:
> >> Name: postgresql10-server
> >> Version : 10.0
> >> Release : beta1PGDG.rhel7
> >>
> >>
> >> Test Type:
> >> Functional testing of a pacemaker resource agent
> >> (https://github.com/ulodciv/pgha)
> >>
> >>
> >> Test Detail:
> >> During context/environement setup, pg_basebackup is invoked (in
> >> parallel) from multiple virtual machines. The backups are then started
> >> as asynchronously replicated hot standbies.
> >>
> >>
> >> Platform:
> >> Centos 7.3
> >>
> >>
> >> Installation Method:
> >> yum -y install
> >>
> >> https://download.postgresql.org/pub/repos/yum/testing/10/
> redhat/rhel-7-x86_64/pgdg-redhat10-10-1.noarch.rpm
> >> yum -y install postgresql10-server postgresql10-contrib
> >>
> >>
> >> Platform Detail:
> >>
> >>
> >> Test Procedure:
> >>
> >> Have pg_basebackup run simultaneously on multiple hosts against
> >> the same instance eg:
> >>
> >> pg_basebackup -h test4 -p 5432 -D /var/lib/pgsql/10/data -U
> repl1
> >> -Xs
> >>
> >>
> >> Failure?
> >>
> >> E   deploylib.deployer_error.DeployerError:
> >> postgres@test5: got exit status 1 for:
> >> E   pg_basebackup -h test4 -p 5432 -D
> >> /var/lib/pgsql/10/data -U repl1 -Xs
> >> E   stderr: pg_basebackup: could not create temporary
> >> replication slot "pg_basebackup_2194": ERROR:  replication slot
> >> "pg_basebackup_2194" already exists
> >> E   pg_basebackup: child process exited with error 1
> >> E   pg_basebackup: removing data directory
> >> "/var/lib/pgsql/10/data"
> >>
> >>
> >> Test Results:
> >>
> >>
> >> Comments:
> >> This seems to be new with 10. I recently began testing the
> >> pacemaker resource agent against PG 10. I never had (or noticed) this
> >> failure with 9.6.1 and 9.6.2.
> >
> >
> > Hah, that's an interesting failure. In the name of the slot, the 2194
> comes
> > from the pid -- but it's the pid of pg_basebackup.
> >
> > I assume you're not running the two pg_basebackup processes on the same
> > machine? Is it predictable when this happens (meaning that the pid value
> is
> > actually predictable), or do you have to run it a large numbe rof times
> > before it happens?
>
>
> Indeed, I run it from two VMs that were created from the same .ova
> (packaged VM).
> I ran into this once, however I have been running tests on 10.0 for a
> couple of days or so.
>
> My guess is that the two hosts ended up using the same pid when
> running the backup.
>


Moving this one over to -hackers to discuss the fix, as this is clearly an
issue.

Right now, pg_basebackup will use the pid of the *client* process to
generate it's ephemeral slot name. Per this report that seems like it can
definitely be a problem.

One of my first thoughts would be to instead use the pid of the *server* to
do that, as this will be guaranteed to be unique. However, the client can't
access the pid of the server as it is now, and its the client that has to
create the name.

One way to do that would be to include the pid of the walsender backend in
the reply to IDENTIFY_SYSTEM, and then use that. What do people think of
that idea?

Other suggestions?

I will add this to the 10.0 open item lists.

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

Re: [HACKERS] [COMMITTERS] Re: pgsql: Code review focused on new node types added by partitioning supp

[Magnus Hagander]

On Tue, May 30, 2017 at 4:41 AM, Stephen Frost <sfr...@snowman.net> wrote:

> * Tom Lane (t...@sss.pgh.pa.us) wrote:
> > Noah Misch <n...@leadboat.com> writes:
> > > On Mon, May 29, 2017 at 03:20:41AM +, Tom Lane wrote:
> > >> Annotate the fact that somebody added location fields to
> PartitionBoundSpec
> > >> and PartitionRangeDatum but forgot to handle them in
> > >> outfuncs.c/readfuncs.c.  This is fairly harmless for production
> purposes
> > >> (since readfuncs.c would just substitute -1 anyway) but it's still
> bogus.
> > >> It's not worth forcing a post-beta1 initdb just to fix this, but if we
> > >> have another reason to force initdb before 10.0, we should go back and
> > >> clean this up.
> >
> > > +1 for immediately forcing initdb for this, getting it out of the
> way.  We're
> > > already unlikely to reach 10.0 without bumping catversion, but if we
> otherwise
> > > did, releasing 10.0 with a 10beta1 catversion would have negative
> value.
> >
> > I'm not really for doing it that way, but I'm willing to apply the fix
> > if there's consensus for your position.  Anybody else have an opinion?
>
> I tend to agree with Noah on this one.
>
>
>
+1


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

Re: [HACKERS] Fix a typo in execExpr.c

[Magnus Hagander]

On Mon, May 29, 2017 at 4:04 PM, Masahiko Sawada <sawada.m...@gmail.com>
wrote:

> Hi,
>
> Attached for $subject.
>
> s/Expession/Expression/
>

Thanks, applied!

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

Re: retry shm attach for windows (WAS: Re: [HACKERS] OK, so culicidae is *still* broken)

[Magnus Hagander]

On Fri, May 26, 2017 at 8:24 AM, Michael Paquier <michael.paqu...@gmail.com>
wrote:

> On Fri, May 26, 2017 at 8:20 AM, Amit Kapila <amit.kapil...@gmail.com>
> wrote:
> > I think the real question here is, shall we backpatch this fix or we
> > want to do this just in Head or we want to consider it as a new
> > feature for PostgreSQL-11.  I think it should be fixed in Head and the
> > change seems harmless to me, so we should even backpatch it.
>
> The thing is not invasive, so backpatching is a low-risk move. We can
> as well get that into HEAD first, wait a bit for dust to settle on it,
> and then backpatch.
>


I would definitely suggest putting it in HEAD (and thus, v10) for a while
to get some real world exposure before backpatching. But if it does work
out well in the end, then we can certainly consider backpatching it. But
given the difficulty in reliably reproducing the problem etc, I think it's
a good idea to give it some proper real world experience in 10 first.

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

Re: [HACKERS] Fix a typo in hash.c

[Magnus Hagander]

On Mon, May 22, 2017 at 3:36 AM, Masahiko Sawada 
wrote:

> Hi,
>
> Attached patch for $subject.
>
> s/curent/current/
>
>
Applied, thanks!

//Magnus

Re: [HACKERS] Making replication commands case-insensitive

[Magnus Hagander]

On Sat, May 20, 2017 at 2:19 PM, Michael Paquier <michael.paqu...@gmail.com>
wrote:

> Hi all,
>
> $subject has been raised in a recent thread here:
> https://www.postgresql.org/message-id/CAB7nPqTmym5t-
> X6hvMF_P-KRc=ndXtbQCTiU=nhs_jvl7x1...@mail.gmail.com
>
> The idea is to make the replication protocol a bit more flexible, in a
> way similar to what 5c837dd has done, but for repl_scanner.l. This
> will also allow to avoid any problems like what has been fixed in
> aa41bc7 where the SHOW commands used in libpq have to be capitalized.
> Personally, I have pested about the lack of flexibility a couple of
> times when running tests using psql..
>
> I am parking that in the next CF.
>

Given that the protocol really isn't intended for "manual consumption", do
we really want that? Not that it adds a lot of complexity, but still. It
certainly makes the code completely unreadable. And since any program using
it should figure out pretty quickly that it's not working if they us the
wrong casing...

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

Re: [HACKERS] [COMMITTERS] pgsql: Tag refs/tags/REL_10_BETA1 was created

[Magnus Hagander]

On Wed, May 17, 2017 at 2:41 PM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 5/16/17 22:37, Tom Lane wrote:
> > BTW, I now remember having wondered[2] if we should make any other
> changes
> > in version-number formatting while we're at it, like maybe "10beta1"
> > should be "10.beta1".
>
> That's not a naming format I've ever seen.
>
> I think the current format is fine.
>
>
+1. I have also never seen that one, and think the current one is good.


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

Re: [HACKERS] [PATCH v2] Progress command to monitor progression of long running SQL queries

[Magnus Hagander]

On Tue, May 16, 2017 at 7:26 PM, Robert Haas <robertmh...@gmail.com> wrote:

> On Tue, May 16, 2017 at 2:17 AM, Michael Paquier
> <michael.paqu...@gmail.com> wrote:
> > Perhaps DSM? It is not user-friendly to fail sporadically...
>
> Yeah.  I've been thinking we might want to give each backend a
> backend-lifetime DSA that is created on first use.  That could be
> useful for some parallel query stuff and maybe for this as well.
> Other backends could attach to it to read data from it, and it would
> go away on last detach (which would normally be the detach by the
> creating backend, but not if somebody else happens to be reading at
> the moment the backend exits).
>

That seems like a pretty good idea. I've been considering something simliar
for the usecase of being able to view the "collected but not sent yet"
statistics inside a backend (which tables have been accessed, number of
reads etc),and this seems like it could be used for that as well.


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

Re: [HACKERS] Typos in pg_basebackup.c

[Magnus Hagander]

On Mon, May 15, 2017 at 7:02 AM, Michael Paquier <michael.paqu...@gmail.com>
wrote:

> Hi all,
>
> Found $subject while working on the area. A patch is attached.
> Thanks,
>

Applied, thanks.

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

Re: [HACKERS] [patch] Build pgoutput with MSVC

[Magnus Hagander]

On Fri, May 5, 2017 at 11:58 AM, Michael Paquier <michael.paqu...@gmail.com>
wrote:

> On Fri, May 5, 2017 at 6:10 PM, MauMau <maumau...@gmail.com> wrote:
> > The pgoutput is not built with MSVC.  The attached patch fixes this.
> > I confirmed that a few INSERTs were replicated correctly.
> >
> > Should I add this matter in the PostgreSQL 10 Open Items page?
>
> Yes, with Peter as committer and Petr as owner.
>
> +my $pgoutput = $solution->AddProject(
> +'pgoutput', 'dll', '',
> +'src/backend/replication/pgoutput');
> +$pgoutput->AddReference($postgres);
> Yup, that's correct.
>
> You have forgotten to update clean.bat, which should clean up pgoutput.dll.
>

If that's all that's required, I'll just go ahead and commit it right away,
including the clean.bat.

I think the problem with clean.bat isn't cleaning up pgoutput.dll -- that
one goes in a different directory. But it does need to clean up the
win32ver.rc file that gets dropped there automaticaly.

The attached patch itself seems broken (it has some sort of byte order
marker at the beginning, but removing that still breaks with "patch
unexpectedly ends in middle of line patch:  Only garbage was found in
the patch input.". But I can just copy/paste it manually :)

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

Re: [HACKERS] Error message on missing SCRAM authentication with older clients

[Magnus Hagander]

On Fri, May 5, 2017 at 10:19 AM, Aleksander Alekseev <
a.aleks...@postgrespro.ru> wrote:

> Hi Heikki,
>
> > psql: SCRAM authentication requires libpq version 10 or above
>
> Sounds good.
>
>
+1.

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

Re: [HACKERS] password_encryption, default and 'plain' support

[Magnus Hagander]

On Fri, May 5, 2017 at 9:38 AM, Albe Laurenz <laurenz.a...@wien.gv.at>
wrote:

> Tom Lane wrote:
> > Robert Haas <robertmh...@gmail.com> writes:
> >> On Wed, May 3, 2017 at 7:31 AM, Heikki Linnakangas <hlinn...@iki.fi>
> wrote:
> >>> So, I propose that we remove support for password_encryption='plain' in
> >>> PostgreSQL 10. If you try to do that, you'll get an error.
>
> >> I have no idea how widely used that option is.
>
> > Is it possible that there are still client libraries that don't support
> > password encryption at all?  If so, are we willing to break them?
> > I'd say "yes" but it's worth thinking about.
>
> We have one application that has been reduced to "password" authentication
> ever since "crypt" authentication was removed, because they implemented the
> line protocol rather than using libpq and never bothered to move to "md5".
>
> But then, it might be a good idea to break this application, because that
> would force the vendor to implement something that is not a
> blatant security problem.
>

It might. But I'm pretty sure the suggestion does not include removing the
"password" authentication type, that one will still exist. This is just
about password *storage*.


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

Re: [HACKERS] Function to move the position of a replication slot

[Magnus Hagander]

On Thu, May 4, 2017 at 2:42 PM, Craig Ringer <cr...@2ndquadrant.com> wrote:

> On 4 May 2017 at 20:05, Magnus Hagander <mag...@hagander.net> wrote:
> > PFA a patch that adds a new function, pg_move_replication_slot, that
> makes
> > it possible to move the location of a replication slot without actually
> > consuming all the WAL on it.
>
> > This can be useful for example to keep replication slots in sync between
> > different servers in a replication cluster.
>
> It needs a test to ensure it only operates on physical slots. It
> should ERROR on a logical slot, since it has no way of correctly
> advancing the catalog_xmin or finding a reasonable restart_lsn  for
> logical decoding.
>

I guess that makes sense, yeah. I didn't try it with that :)



> I'm still fine with the name, since I plan to add that capability in
> pg11 by running through logical decoding and ReorderBufferSkip()ing
> each xact until we reach the target lsn.
>

Cool.

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

[HACKERS] Function to move the position of a replication slot

[Magnus Hagander]

PFA a patch that adds a new function, pg_move_replication_slot, that makes
it possible to move the location of a replication slot without actually
consuming all the WAL on it.

This can be useful for example to keep replication slots in sync between
different servers in a replication cluster.

(Obviously this is intended for 11, as we're well into the freeze for 10.
Just to be clear. so I'll go add itto the summer commitfest)

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ <http://www.hagander.net/>
 Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index f06d0a9..6b1ff0a 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -18968,6 +18968,24 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
 be called when connected to the same database the slot was created on.

   
+  
+   
+
+ pg_move_replication_slot
+
+pg_move_replication_slot(slot_name name, position pg_lsn)
+   
+   
+bool
+   
+   
+Moves the current restart position of a physical or logical
+replication slot named slot_name.
+The slot will not be moved backwards, and it will not be
+moved beyond the current insert location. Returns true if
+the position was changed.
+   
+  
 
   

diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index 6ee1e68..a9faa10 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -176,6 +176,66 @@ pg_drop_replication_slot(PG_FUNCTION_ARGS)
 }
 
 /*
+ * SQL function for moving the position in a replication slot.
+ */
+Datum
+pg_move_replication_slot(PG_FUNCTION_ARGS)
+{
+	Name		slotname = PG_GETARG_NAME(0);
+	XLogRecPtr	moveto = PG_GETARG_LSN(1);
+	char	   *slotnamestr;
+	bool		changed = false;
+	bool 		backwards = false;
+
+	Assert(!MyReplicationSlot);
+
+	check_permissions();
+
+	if (XLogRecPtrIsInvalid(moveto))
+		ereport(ERROR,
+(errmsg("Invalid target xlog position ")));
+
+	/* Temporarily acquire the slot so we "own" it */
+	ReplicationSlotAcquire(NameStr(*slotname));
+
+	if (moveto > GetXLogWriteRecPtr())
+		/* Can't move past current position, so truncate there */
+		moveto = GetXLogWriteRecPtr();
+
+	/* Now adjust it */
+	SpinLockAcquire(>mutex);
+	if (MyReplicationSlot->data.restart_lsn != moveto)
+	{
+		/* Never move backwards, because bad things can happen */
+		if (MyReplicationSlot->data.restart_lsn > moveto)
+			backwards = true;
+		else
+		{
+			MyReplicationSlot->data.restart_lsn = moveto;
+			changed = true;
+		}
+	}
+	SpinLockRelease(>mutex);
+
+	if (backwards)
+		ereport(WARNING,
+(errmsg("Not moving replication slot backwards!")));
+
+
+	if (changed)
+	{
+		ReplicationSlotMarkDirty();
+		ReplicationSlotsComputeRequiredLSN();
+		ReplicationSlotSave();
+	}
+
+	ReplicationSlotRelease();
+
+	PG_RETURN_BOOL(changed);
+}
+
+
+/*
  * pg_get_replication_slots - SQL SRF showing active replication slots.
  */
 Datum
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 82562ad..04e97ff 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -5289,6 +5289,8 @@ DATA(insert OID = 3779 (  pg_create_physical_replication_slot PGNSP PGUID 12 1 0
 DESCR("create a physical replication slot");
 DATA(insert OID = 3780 (  pg_drop_replication_slot PGNSP PGUID 12 1 0 0 0 f f f f t f v u 1 0 2278 "19" _null_ _null_ _null_ _null_ _null_ pg_drop_replication_slot _null_ _null_ _null_ ));
 DESCR("drop a replication slot");
+DATA(insert OID = 3998 (  pg_move_replication_slot PGNSP PGUID 12 1 0 0 0 f f f f t f v u 2 0 16 "19 3220" _null_ _null_ _null_ _null_ _null_ pg_move_replication_slot _null_ _null_ _null_ ));
+DESCR("move a replication slot position");
 DATA(insert OID = 3781 (  pg_get_replication_slots	PGNSP PGUID 12 1 10 0 0 f f f f f t s s 0 0 2249 "" "{19,19,25,26,16,16,23,28,28,3220,3220}" "{o,o,o,o,o,o,o,o,o,o,o}" "{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn}" _null_ _null_ pg_get_replication_slots _null_ _null_ _null_ ));
 DESCR("information about replication slots currently in use");
 DATA(insert OID = 3786 (  pg_create_logical_replication_slot PGNSP PGUID 12 1 0 0 0 f f f f t f v u 3 0 2249 "19 19 16" "{19,19,16,25,3220}" "{i,i,i,o,o}" "{slot_name,plugin,temporary,slot_name,wal_position}" _null_ _null_ pg_create_logical_replication_slot _null_ _null_ _null_ ));

-- 
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] [PostgreSQL 10] default of hot_standby should be "on"?

[Magnus Hagander]

On Wed, May 3, 2017 at 4:18 PM, Michael Banck <michael.ba...@credativ.de>
wrote:

> Hi,
>
> On Tue, May 02, 2017 at 11:13:58AM +0200, Magnus Hagander wrote:
> > Looks good to me as well. Applied, with only a minor further docs
> addition
> > saying that this is the default also on the high availability page.
>
> I understand this is late, but a colleague alerted me to the following
> behaviour change: If you recover a server with default settings, it is
> our understanding that pg_isready will now return true immediately after
> the consistent state is reached and possibly well before recovery had
> actually ended (depending on the amount of outstanding wal). As hot
> standby works with log shipping, this is independent of the
> recovery.conf settings, i.e. even if standby_mode and primary_conninfo
> are not set. So if one was monitoring recovery like that before and
> expects pg_isready to only return true once the recovery is fully
> complete, this will now have to be adjusted. Also, if the recovered
> server is to be used for transactions, there will now be a window where
> the server accepts connections, but is in read-only mode.
>
> Before, one had the make the concious choice to set hot_standby to get
> the behaviour, now it might be surprising to users, or maybe I'm
> overthinking this?
>
> If that is indeed the case, maybe it should be mentioned more
> prominently in the documentation and/or get highlighted in the release
> notes?
>

Hmm. That's an interesting usecase.

I don't think it's a big enough one to revert this change, but it
definitely makes sense to mention it under incompatible changes.

I wonder if what we really want here, at least long-term, is a flag for
pg_isready that makes it wait for a server to actually go out of
recovery?`Seems that a tool like the one mentioned here would have to do
that -- it can be done now by doing pg_isready first and then psql to check
the status, but it seems like it could be a worthwhile addition?


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

Re: [HACKERS] password_encryption, default and 'plain' support

[Magnus Hagander]

On Wed, May 3, 2017 at 5:52 PM, Robert Haas <robertmh...@gmail.com> wrote:

> On Wed, May 3, 2017 at 7:31 AM, Heikki Linnakangas <hlinn...@iki.fi>
> wrote:
> > In various threads on SCRAM, we've skirted around the question of
> whether we
> > should still allow storing passwords in plaintext. I've avoided
> discussing
> > that in those other threads, because it's been an orthogonal question,
> but
> > it's a good question and we should discuss it.
> >
> > So, I propose that we remove support for password_encryption='plain' in
> > PostgreSQL 10. If you try to do that, you'll get an error.
>
> I have no idea how widely used that option is.
>
> > Another question that's been touched upon but not explicitly discussed,
> is
> > whether we should change the default to "scram-sha-256". I propose that
> we
> > do that as well. If you need to stick to md5, e.g. because you use
> drivers
> > that don't support SCRAM yet, you can change it in postgresql.conf, but
> the
> > majority of installations that use modern clients will be more secure by
> > default.
>
> I think that we should investigate how many connectors have support
> for SCRAM or are likely to do so by the time v10 is released.  A *lot*
> of people are using connectors that are not based on libpq, especially
> JDBC but I think many of the others as well.  If most of those are
> going to support SCRAM by the time v10 comes out, cool, but if not,
> maybe it's wise to hold off for a release before flipping the default.
> Not sure.
>


>From the traffic on the list it sounds like the JDBC people are working on
it already -- hopefully they will have something in time.

It might make sense to ping other "major drivers" people as well -- such as
maybe npgsql. What else?

A good approach might be to change the default now, before beta. Then if
drivers don't change, or if we get a lot of pushback from beta testers, we
change it back before release. But if we don't change it, we will not know
how big the impact would be...

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

Re: [HACKERS] password_encryption, default and 'plain' support

[Magnus Hagander]

On Wed, May 3, 2017 at 2:25 PM, Michael Paquier <michael.paqu...@gmail.com>
wrote:

> On Wed, May 3, 2017 at 8:38 PM, Magnus Hagander <mag...@hagander.net>
> wrote:
> > On Wed, May 3, 2017 at 1:31 PM, Heikki Linnakangas <hlinn...@iki.fi>
> wrote:
> >> In various threads on SCRAM, we've skirted around the question of
> whether
> >> we should still allow storing passwords in plaintext. I've avoided
> >> discussing that in those other threads, because it's been an orthogonal
> >> question, but it's a good question and we should discuss it.
> >>
> >> So, I propose that we remove support for password_encryption='plain' in
> >> PostgreSQL 10. If you try to do that, you'll get an error.
> >
> > Is there any usecase at all for it today?
>
> For developers running applications on top of Postgres?
>

I don't get it. How does password_encryption=plain help them?


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

Re: [HACKERS] Error message on missing SCRAM authentication with older clients

[Magnus Hagander]

On Wed, May 3, 2017 at 10:58 AM, Heikki Linnakangas <hlinnakan...@pivotal.io
> wrote:

> Currently, if you use 9.6 libpq to connect to a v10 server that requires
> SCRAM authentication, you get an error:
>
> psql: authentication method 10 not supported
>
> I'd like to apply this small patch to all the stable branches, to give a
> nicer error message:
>
> psql: SCRAM authentication not supported by this version of libpq
>
> It won't help unless you upgrade to the latest minor version, of course,
> but it's better than nothing. Any objections?


+1, even though it's not strictly speaking a bugfix to go in a backpatch, I
think it will help enough users that it's worth doing. And I can't see how
it could possibly be unsafe...

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

Re: [HACKERS] password_encryption, default and 'plain' support

[Magnus Hagander]

On Wed, May 3, 2017 at 1:31 PM, Heikki Linnakangas <hlinn...@iki.fi> wrote:

> Hi,
>
> In various threads on SCRAM, we've skirted around the question of whether
> we should still allow storing passwords in plaintext. I've avoided
> discussing that in those other threads, because it's been an orthogonal
> question, but it's a good question and we should discuss it.
>
> So, I propose that we remove support for password_encryption='plain' in
> PostgreSQL 10. If you try to do that, you'll get an error.
>

Is there any usecase at all for it today?

+1 for getting rid of it :)



> Another question that's been touched upon but not explicitly discussed, is
> whether we should change the default to "scram-sha-256". I propose that we
> do that as well. If you need to stick to md5, e.g. because you use drivers
> that don't support SCRAM yet, you can change it in postgresql.conf, but the
> majority of installations that use modern clients will be more secure by
> default.


Much as that's going to cause issues for some people, I think it's worth
doing. We should probably put something specific in the release notes
mentioning the error message you get in libpq, and possibly some of the
other most common drivers.

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

Re: [HACKERS] scram and \password

[Magnus Hagander]

On Wed, May 3, 2017 at 10:26 AM, Heikki Linnakangas <hlinn...@iki.fi> wrote:

> On 05/02/2017 07:47 PM, Robert Haas wrote:
>
>> On Tue, May 2, 2017 at 3:42 AM, Heikki Linnakangas <hlinn...@iki.fi>
>> wrote:
>>
>>> There's going to be a default, one way or another. The default is going
>>> to
>>> come from password_encryption, or it's going to be a hard-coded value or
>>> logic based on server-version in PQencryptPasswordConn(). Or it's going
>>> to
>>> be a hard-coded value or logic implemented in every application that uses
>>> PQencryptPasswordConn(). I think looking at password_encryption makes the
>>> most sense. The application is not in a good position to make the
>>> decision,
>>> and forcing the end-user to choose every time they change a password is
>>> too
>>> onerous.
>>>
>>
>> I think there should be no default, and the caller should have to pass
>> the algorithm explicitly.  If they want to determine what default to
>> pass by running 'SHOW password_encryption', that's their choice.
>>
>
> Ok, gotcha. I disagree, I think we should provide a default. Libpq is in a
> better position to make a good choice than most applications.
>
> I've committed the new PQencryptPasswordConn function, with the default
> behavior of doing "show password_encryption", and the changes to use it in
> psql and createuser. This closes the open issue with \password.


If we're basically just telling people to call SHOW manually, we might as
well do it in the default case. I think the wording you put into the docs
there is good, as it tells people exactly what happens and how to reproduce
it locally.

For the security perspective, perhaps we should have a link to the part of
the docs that discusses the different algorithms?

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

Re: [HACKERS] [PostgreSQL 10] default of hot_standby should be "on"?

[Magnus Hagander]

On Fri, Apr 28, 2017 at 3:43 AM, Masahiko Sawada <sawada.m...@gmail.com>
wrote:

> On Thu, Apr 27, 2017 at 11:05 PM, Huong Dangminh
> <huo-dangm...@ys.jp.nec.com> wrote:
> >> >>> I would refrain from doing that, having some parameters listed in
> the
> >> >>> tests makes the intention behind those perl routines clear.
> >> >
> >> > Hmm, you've got a point. But when we changed the default values
> >> > related to replication we dropped some explicitly settings from the
> >> > regression test code.
> >>
> >> Looking at the patch. This is fine:
> >> -  # Change a setting and restart
> >> -  $node->append_conf('postgresql.conf', 'hot_standby = on');
> >> -  $node->restart();
> >>
> >> But not that:
> >>  print $conf "wal_log_hints = on\n";
> >> -print $conf "hot_standby = on\n";
> >>  print $conf "max_connections = 10\n";
> >>
> >> This is a minor point though.
>
> After some thoughts I agree to remain it in the perl code.
>
> >
> > Thanks, I attached the update patch.
>
> So it looks good to me.
>


Looks good to me as well. Applied, with only a minor further docs addition
saying that this is the default also on the high availability page. And per
the comments from Michael, I did not include the change to PostgresNode.pm.

Thanks!

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

Re: [HACKERS] [PostgreSQL 10] default of hot_standby should be "on"?

[Magnus Hagander]

On Wed, Apr 26, 2017 at 1:25 PM, Bruce Momjian <br...@momjian.us> wrote:

> On Wed, Apr 26, 2017 at 07:33:27AM +, Tsunakawa, Takayuki wrote:
> > From: pgsql-hackers-ow...@postgresql.org
> > > [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Masahiko
> Sawada
> > > The idea of changing the default value seems good to me but I'm not
> sure
> > > it's good idea to change the default value now under the circumstances
> where
> > > we're focus on stabilization.
> > > Also we should update the document as well.
> > >
> >
> > We can consider like this: the OP found a usability problem as a result
> of PG 10 development, so we will fix it as a stabilization work.
>
> We did work in Postgres 10 to make replication simpler with better
> defaults.  This would be part of that improvement.
>


+1. I definitely think we should do it, and 10 would be the time to do it.

The failure scenario is that a standby node will no longer work by default
*if* you have changed the master to minimal. But unless you have explicitly
dropped that one, it would work.

So I definitely think we should change that.

I wonder if we should also consider changing the standby error message to
be a WARNING instead of an ERROR. So that if you try to start up a standby
with hot_standby=on but master with wal_level=replica it would turn into a
cold standby.

We should change the default independently of that, I think, but it might
make sense to do both.

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

Re: [HACKERS] scram and \password

[Magnus Hagander]

On Tue, Apr 25, 2017 at 8:29 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Robert Haas <robertmh...@gmail.com> writes:
> > On Tue, Apr 25, 2017 at 11:26 AM, Heikki Linnakangas <hlinn...@iki.fi>
> wrote:
> >> A) Have PQencryptPassword() return an md5 hash.
> >>
> >> B) Have PQencryptPassword() return a SCRAM verifier
> >>
> >> C) Have PQencryptPassword() return a SCRAM verifier if connected to a
> v10
> >> server, and an md5 hash otherwise. This is tricky, because
> PQencryptPassword
> >> doesn't take a PGconn argument. It could behave like PQescapeString()
> does,
> >> and choose md5/scram depending on the server version of the last
> connection
> >> that was established.
>
> > I vote for A - leave PQencryptPassword() as-is, and deprecate it.
> > Tell people to use the new function going forward.
>
> +1.  I never much liked that magic behavior of PQescapeString, and don't
> think we should replicate it elsewhere, so I definitely don't like (C).
> And I don't think we can do (B) because that will break the functionality
> altogether when talking to an older server.  That leaves (A) plus invent
> a new function.
>

+1.


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

Re: [HACKERS] SCRAM authentication, take three

[Magnus Hagander]

On Tue, Apr 18, 2017 at 1:52 PM, Heikki Linnakangas <hlinn...@iki.fi> wrote:

> On 04/14/2017 10:33 PM, Peter Eisentraut wrote:
>
>> On 4/11/17 01:10, Heikki Linnakangas wrote:
>>
>>> That question won't arise in practice. Firstly, if the server can do
>>> scram-sha-256-plus, it presumably can also do scram-sha-512-plus. Unless
>>> there's a change in the way the channel binding works, such that the
>>> scram-sha-512-plus variant needs a newer version of OpenSSL or
>>> something. Secondly, the user's pg_authid row will contain a
>>> SCRAM-SHA-256 or SCRAM-SHA-512 verifier, not both, so that will dictate
>>> which one to use.
>>>
>>
>> Right.  So putting the actual password method in pg_hba.conf isn't going
>> to be useful very often.
>>
>> I think the most practical thing that the user wants in pg_hba.conf is
>> "best password method supported by what is in pg_authid".  This is
>> currently spelled "md5", which is of course pretty weird.  And it will
>> become weirder over time.
>>
>> I think we want to have a new keyword in pg_hba.conf for that, one which
>> does not indicate any particular algorithm or method (so not "scram" or
>> "sasl").
>>
>> We could use "password".  If we think that "md5" can mean md5-or-beyond,
>> then maybe "password" can mean password-or-md5-or-beyond.
>>
>> Or otherwise a completely new word.
>>
>> We also want to give users/admins a way to phase out old methods or set
>> some policy.  We could either make a global GUC setting
>> password_methods='md5 scram-sha-256' and/or make that an option in
>> pg_hba.conf past the method field.
>>
>
> Yeah, that would be reasonable. It can't be called just "password",
> though, because there's no way to implement "password-or-md5-or-scram" in a
> sensible way (see my reply to Simon at [1]). Unless we remove the support
> for what "password" does today altogether, and redefine "password" to mean
> just "md5-or-beyond". Which might not be a bad idea, but that's a separate
> discussion.
>

It is an interesting one though. "password" today is really only useful in
the case of db_user_namespace=on, right? Given the very few people I think
are using that feature, it wouldn't be unreasonable to rename it to
something more closely related to that.

However, that would also leave us in the position to explain "before 10,
avoid using password because it stores in clear text. after 10, we
recommend you use password". Reusing something that's existed before and
not really been secure for something that would be a good choice in the
future seems like a bad idea.

But we can also implement this functionality but under a differet name.
Like just "hashed" or something, which would mean md5-or-scram?



> In any case, I think we would probably still need more fine-grained
> control, too, so we would still need to have "scram-sha-256" as a method
> you can specify directly in pg_hba.conf. So I consider this as a separate,
> new, feature that we can add in the future, if it seems worth the effort.
>

Yes, I think wherever we go we don't want to loose the fine-grained
control. But some people will be happier for not having to use it.

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

Re: [HACKERS] Self-signed certificate instructions

[Magnus Hagander]

On Sat, Apr 15, 2017 at 7:54 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
> > The instructions on how to create a self-signed certificate in s 18.9.3
> > of the docs seem unduly cumbersome.
>
> Yeah, I noticed that they seemed unnecessarily manual.  +1 for
> simplifying.
>

Seems reasonable, +1 for simplifications.

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

Re: [pgsql-www] [HACKERS] Small issue in online devel documentation build

[Magnus Hagander]

On Fri, Apr 14, 2017 at 9:36 PM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 4/14/17 14:45, Magnus Hagander wrote:
> > Attached is a patch that can be applied to pgweb which should fix
> all of
> > this.
> >
> >
> >
> > Applied, thanks.
>
> Oops, one  too many.
>
>
Indeed. Fix pushed.


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

Re: [HACKERS] Wincrypt.h vs wincrypt.h

[Magnus Hagander]

On Fri, Apr 14, 2017 at 9:27 PM, Andrew Dunstan <
andrew.duns...@2ndquadrant.com> wrote:

>
> Commit fe0a0b59 included this line:
>
> #include 
>
> On Windows file names are not case sensitive, so the "W" works fine, but
> as I discovered this morning, if you're cross-compiling on Linux it
> matters very much, and mingw-w64 ships headers with lower case file
> names, in this case "wincrypt.h". Therefore, unless there's an objection
> I propose to lower case that "W".
>

+1. Regardless of the cross compiling, I believe using lowercase is more or
less the convention on Windows for this.

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

Re: [pgsql-www] [HACKERS] Small issue in online devel documentation build

[Magnus Hagander]

On Sat, Apr 8, 2017 at 3:52 AM, Bruce Momjian <br...@momjian.us> wrote:

> On Fri, Mar 24, 2017 at 07:01:46AM +0100, Fabien COELHO wrote:
> >
> > Hello Peter,
> >
> > >I think the fix belongs into the web site CSS, so there is nothing to
> > >commit into PostgreSQL here.
> >
> > Indeed, the changes were only for the "remove nesting" solution.
> >
> > >I will close the commit fest entry, but I have added a section to the
> open
> > >items list so we keep track of it. (https://wiki.postgresql.org/
> wiki/PostgreSQL_10_Open_Items#Documentation_tool_chain)
> >
> > I put forward that the quick workaround a colleague of mine suggested
> (aka
> > something like code code { font-size: 100%; important! }) could also be
> > applied to the web site CSS while waiting for a more definite answer
> which
> > might take some pretty unknown time close to never?
>
> Sorry I am just getting back to this.  Below I am going to cover only
> the problem with the font size of nested  tags, and I am going to
> confirm what most people already figured out.
>
> The basic problem was already posted by Fabien, with an image example.
> The cause of the fonts being too large on Chrome is an interaction of
> Chrome's default font size for different blocks, the JavaScript that is
> meant to fix such mismatches, and the new nested code blocks in the PG
> 10 docs.
>
> First, the JavaScript:
>
> https://github.com/postgres/pgweb/blob/master/media/js/
> monospacefix.js
>
> There is no git history for this file except for its initial checkin in
> 2011, but I am pretty sure I wrote it.  What it does is to create 
> and  blocks, find the font point size, and compute a ratio.  If the
> ratio is not 1, , , and  blocks are adjusted in size to
> match .  The complex part is that the JavaScript conditionally
> injects CSS into the web-page to accomplish this.
>
> The reason the PG 10 docs look fine on Linux Firefox is because the font
> points sizes match so no CSS is injected.  They don't match on Chrome,
> so the CSS is injected.  When the CSS hits double-embedded code blocks,
>  , it makes the font too large because it double-adjusts.
>
> The fix, as Fabien identified, is to conditionally inject additional CSS
> to be _more_ specific than the first CSS and set the font-size to a
> simple '1em' so the first CSS is not called twice.  I don't think
> 'important!' is necessary but it would be good to test this.
>
> Attached is a patch that can be applied to pgweb which should fix all of
> this.
>
>

Applied, thanks.


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

Re: [HACKERS] pg_upgrade vs extension upgrades

[Magnus Hagander]

On Thu, Apr 13, 2017 at 9:13 PM, Bruce Momjian <br...@momjian.us> wrote:

> On Thu, Apr 13, 2017 at 09:02:27PM +0200, Magnus Hagander wrote:
> > On Thu, Apr 13, 2017 at 12:30 AM, Peter Eisentraut <
> > peter.eisentr...@2ndquadrant.com> wrote:
> >
> > On 4/10/17 11:30, Magnus Hagander wrote:
> > > After you've run pg_upgrade, you have to loop through all your
> databases
> > > and do an "ALTER EXTENSION abc UPDATE" once for each extension.
> > >
> > > Is there a reason we shouldn't have pg_upgrade emit a script that
> does
> > > this, similar to how it emits a script to run ANALYZE?
> >
> > Shouldn't pg_dump do this, and perhaps by default?
> >
> >
> > If I restore a dump into another instance, I need to upgrade all my
> > extensions to that installations's versions, no?  That's not
> particular
> > to pg_upgrade.
> >
> > Sure, there's an argument to be made for that.  But pg_dump (or in this
> case,
> > it would more be pg_restore I guess) also doesn't run ANALYZE or
> generate a
> > script to do that, does it? ISTM that we have already decided that
> pg_upgrade
> > has a different requirement on providing those things, whereas pg_dump/
> > pg_restore is more of a low-level tool where people have to figure more
> things
> > out themselves.
>
> Well, pg_upgrade creates ./analyze_new_cluster.sh, but that just
> contains:
>
> "/u/pgsql/bin/vacuumdb" --all --analyze-in-stages
>
> Seems like we should just get rid of ./analyze_new_cluster.sh and tell
> the user to run vacuumdb directly.  I guess I will have to wait for PG
> 11 to do that though.
>
>
Yeah, at this point that probably makes a lot of sense, now that we don't
need the logic in the script anymore.

FWIW, I'm not sure the feature freeze means we can't *remove* a feature?
But I'll defer to others on that.

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

Re: [HACKERS] pg_upgrade vs extension upgrades

[Magnus Hagander]

On Thu, Apr 13, 2017 at 6:06 PM, Robert Haas <robertmh...@gmail.com> wrote:

> On Thu, Apr 13, 2017 at 11:48 AM, Peter Eisentraut
> <peter.eisentr...@2ndquadrant.com> wrote:
> > On 4/12/17 22:56, Bruce Momjian wrote:
> >> Is pg_upgrade the right place for an extension upgrade script?  When
> >> pg_upgrade started creating an incremental-analyze script, people
> >> thought it should be more generic so it was moved to vacuumdb
> >> --analyze-in-stages.  Seems we should do the same thing for the
> >> extension upgrade script.
> >
> > Yeah, many of the things that pg_upgrade would do or suggest after an
> > upgrade could also apply to other upgrade methods, such as by logical
> > replication.  So offering them separately would be good.
>
> And in theory, extension upgrades could happen any time, not just when
> there's a major version upgrade.  You could be using a
> separately-bundled extension, or we could bump an extension version in
> a minor release to fix some issue.
>
> I think we should invent a clever name for a new utility, like
> pg_maintain or something.  It could let you know about (and optionally
> perform) a variety of optional maintenance tasks:
>
> - upgrading extensions
> - reindexing indexes for which the version has been bumped, like hash
> indexes in v10
> - reindexing or dropping indexes marked invalid that were left behind
> by a CIC failure
> - analyzing tables that lack (full?) statistics
> - triggering heap scans to reclaim HEAP_MOVED_* bits, if we have that
> kind of thing someday
>
>
I agree it makes a lot of sense to have a separate tool that can do these
things, and that pg_upgrade can point the user towards it.

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

Re: [HACKERS] pg_upgrade vs extension upgrades

[Magnus Hagander]

On Thu, Apr 13, 2017 at 12:30 AM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 4/10/17 11:30, Magnus Hagander wrote:
> > After you've run pg_upgrade, you have to loop through all your databases
> > and do an "ALTER EXTENSION abc UPDATE" once for each extension.
> >
> > Is there a reason we shouldn't have pg_upgrade emit a script that does
> > this, similar to how it emits a script to run ANALYZE?
>
> Shouldn't pg_dump do this, and perhaps by default?


> If I restore a dump into another instance, I need to upgrade all my
> extensions to that installations's versions, no?  That's not particular
> to pg_upgrade.
>
>
Sure, there's an argument to be made for that.  But pg_dump (or in this
case, it would more be pg_restore I guess) also doesn't run ANALYZE or
generate a script to do that, does it? ISTM that we have already decided
that pg_upgrade has a different requirement on providing those things,
whereas pg_dump/pg_restore is more of a low-level tool where people have to
figure more things out themselves.


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

Re: [HACKERS] Function to control physical replication slot

[Magnus Hagander]

On Thu, Apr 13, 2017 at 2:40 AM, Andres Freund <and...@anarazel.de> wrote:

> On 2017-04-12 20:15:52 -0400, Peter Eisentraut wrote:
> > On 4/11/17 05:15, Magnus Hagander wrote:
> > > Is there a particular reason we don't have a function to *set* the
> > > restart_lsn of a replication slot, other than to drop it and recreate
> it?
> >
> > I suppose there could be lots of problems if the LSN you specify isn't
> > valid.  And it might be hard to determine whether a given LSN is valid.
>
> As long as we're only talking about the LSN of a physical slot (and not
> the xmin) I'm not sure it's that important that it's valid, as long as
> it's not in the future.  But we could otherwise pretty easily assert
> that the new value has to be old_value <= new_value <=
> GetRedoRecPtr()/GetFlushRecPtr().  That should be sufficient for both of
> your use-cases afaics?
>

Yes, I think making that restriction falls well within my requirements --
move it only forward, and not past the end of the current position.

One could argue that a reasonable thing to do when trying to move past the
current position would be to just "truncate" it to the current position,
instead of throwing an error. But that could also be done in userspace
using CASE on the parameter I guess. Not sure which is best. Any opinions
on that?

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

Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.

[Magnus Hagander]

On Wed, Apr 12, 2017 at 2:36 PM, Robert Haas <robertmh...@gmail.com> wrote:

> On Tue, Apr 11, 2017 at 4:05 AM, Magnus Hagander <mag...@hagander.net>
> wrote:
> > On Tue, Apr 11, 2017 at 3:26 AM, Michael Paquier <
> michael.paqu...@gmail.com> wrote:
> >> On Mon, Apr 10, 2017 at 5:47 PM, Magnus Hagander <mag...@hagander.net>
> >> wrote:
> >> > Based on that we seem to agree here, should we add this as an open
> item?
> >> > Clearly if we want to change this, we should do so before 10.
> >>
> >> This really is a new feature, so as the focus is to stabilize things I
> >> think that we should not make the code more complicated because...
> >
> > The part I'm talking about is the potential adjustment of the patch
> that's
> > already committed. That's not a new feature, that's exactly the sort of
> > thing we'd want to adjust before we get to release. Because once
> released we
> > really can't change it.
>
> I don't really agree.  I think if we go and install a GUC_REPORT GUC
> now, we're much less likely to flush out the bugs in the 'show
> transaction_read_only' mechanism.  Also, now that I think about, the
> reason why we settled on 'show transaction_read_only' against
> alternate queries is because there's some ability for the DBA to make
> that return 'false' rather than 'true' even when not in recovery, so
> that if for example you are using logical replication rather than
> physical replication, you have a way to make
> target_session_attrs=read-write still do something useful.  If you add
> an in_hot_standby GUC that's used instead, you lose that.
>
> Now, we can decide what we want to do about that, but I don't see that
> a change in this area *must* go into v10.  Maybe the answer is that
> target_session_attrs grows additional values like 'primary' and
> 'standby' alongside 'read-write' (and Simon's suggested 'read-only').
> Or maybe we have another idea.  But I don't really see the urgency of
> whacking this around right this minute.  There's nothing broken here;
> there's just more stuff people would like to have.  It can be added
> next time around.
>
>
Fair enough, sounds reasonable. I wasn't engaged in the original thread, so
you clearly have thought about this more than I have. I just wanted to make
sure we're not creating something that's going to cause a head-ache for
such a feature in the future.

(And this is why I was specifically asking you if you wanted it on the open
items list or not!)


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

Re: [HACKERS] Reversed sync check in pg_receivewal

[Magnus Hagander]

On Tue, Apr 11, 2017 at 6:51 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Magnus Hagander <mag...@hagander.net> writes:
> > Something like the attached?
>
> Not sure about
>
> + * All methods that have a failure path will set errno on failure.
>
> Given that you've got a getlasterror method, I don't think that's really
> the API invariant is it?  If it were, you'd just have the callers doing
> strerror(errno) for themselves.  I think maybe a more accurate statement
> would be
>

Hmm. You're right, this is what I get for rushing a patch before plane
departure :/


>  * All methods that have a failure return indicator will set state
>  * allowing the getlasterror() method to return a suitable message.
>  * Commonly, errno is this state (or part of it); so callers must take
>  * care not to clobber errno between a failed method call and use of
>  * getlasterror() to retrieve the message.
>


Yeah, that's much better.



> Also:
>
> * the arguments of open_for_write() could stand to be explained
>
> * what's the return value of finish()?
>

Both fixed.


>
> * wouldn't it be better to declare getlasterror() as returning
>   "const char *"?
>

Yeah, probably is. Will do and push.

Thanks!

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

Re: [pgsql-www] [HACKERS] Small issue in online devel documentation build

[Magnus Hagander]

On Tue, Apr 11, 2017 at 4:30 PM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 4/11/17 08:49, Magnus Hagander wrote:
> > At the risk of being proven wrong again, won't this affect  tags in
> > the old documentation as well? And if so, is that something we actually
> > want?
>
> Right.  New patch with more refined selectors attached.
>
> > It does? The output on my laptop for that produces  > summary="Backslash Escape Sequences" border="1"> (for example). It's
> > wrapped in a div with class=table. But the old code had  > border="1" class="CALSTABLE">. And AFAICT, it's the CALSTABLE that the
> > css is actually matching on?
> >
> > Could this be a toolchain version thing? I'm using jessie on my laptop,
> > which is the same one that the website is built with.
>
> Seems so.  Patch for that attached as well.
>
>
Thanks, I've applied all three patches. Your version of the color patch
also seemed slightly more appropriate than Bruce's since it only hit the
docs.css and not the global css.

Once difference I notice is that for example the "note boxes" are no longer
centered, but they do now get the new formatting. Tables now look a lot
better.

I think that only leaves the change to the javascript code that Bruce sent.
Let's see if we can figure out a way to do that one without requiring
javascript, but after that we have covered all listed issues I think?

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