Re: [HACKERS] Showing parallel status in \df+

[Tom Lane]

Pavel Stehule  writes:
> 2016-09-28 16:03 GMT+02:00 Tom Lane :
>> I propose to push my current patch (ie, move PL function
>> source code to \df+ footers), and we can use it in HEAD for awhile
>> and see what we think.  We can alway improve or revert it later.

> I had some objection to format of source code - it should be full source
> code, not just header and body.

That would be redundant with stuff that's in the main part of the \df
display.  I really don't need to see the argument types twice, for instance.

regards, tom lane


-- 
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] Binary I/O for isn extension

[Robert Haas]

On Mon, Aug 22, 2016 at 8:14 AM, Fabien COELHO  wrote:
> Hello Shay,
>> Attached is a new version of the patch, adding an upgrade script and the
>> rest of it. Note that because, as Fabien noted, there's doesn't seem to be
>> a way to add send/receive functions with ALTER TYPE, I did that by
>> updating
>> pg_type directly - hope that's OK.
>
> This patch does not apply anymore, because there as been an update in
> between to mark relevant contrib functions as "parallel".
>
> Could you update the patch?

So, it's been over a month since this request, and there doesn't seem
to be an update to this patch.  The CommitFest is over in 2 days, so
I've marked this "Returned with Feedback".  Shay, please feel free to
resubmit for the next CommitFest.

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


-- 
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] Index Onlys Scan for expressions

[Robert Haas]

On Thu, Sep 8, 2016 at 2:58 PM, Vladimir Sitnikov
 wrote:
> Ildar> Could you please try the patch and tell if it works for you?
>
> I've tested patch6 against recent head. The patch applies with no problems.
>
> The previous case (filter on top of i-o-s) is fixed. Great work.
>
> Here are the test cases and results:
> https://gist.github.com/vlsi/008e18e18b609fcaaec53d9cc210b7e2
>
> However, it looks there are issues when accessing non-indexed columns.
> The error is "ERROR: variable not found in subplan target list"
> The case is 02_case2_fails.sql (see the gist link above)
>
> The essence of the case is "create index on substr(vc, 1, 128)"
> and assume that majority of the rows have length(vc)<128.
> Under that conditions, it would be nice to do index-only-scan
> and filter (like in my previous case), but detect "long" rows
> and do additional recheck for them.

Based on this report, this patch appears to have bugs that would
preclude committing it, so I'm marking it "Returned with Feedback" for
this CommitFest, which is due to end shortly.  Ildar, please feel free
to resubmit once you've updated the patch.

FWIW, I think this is a good effort and hope to see it move forward.

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


-- 
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] pgbench more operators & functions

[Jeevan Ladhe]

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

The patch looks good to me now.
Passing this to committer.

The new status of this patch is: Ready for Committer

-- 
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] Binary I/O for isn extension

[Shay Rojansky]

Sorry about this, I just haven't had a free moment (and it's definitely not
very high priority...)

On Wed, Sep 28, 2016 at 5:04 PM, Robert Haas  wrote:

> On Mon, Aug 22, 2016 at 8:14 AM, Fabien COELHO 
> wrote:
> > Hello Shay,
> >> Attached is a new version of the patch, adding an upgrade script and the
> >> rest of it. Note that because, as Fabien noted, there's doesn't seem to
> be
> >> a way to add send/receive functions with ALTER TYPE, I did that by
> >> updating
> >> pg_type directly - hope that's OK.
> >
> > This patch does not apply anymore, because there as been an update in
> > between to mark relevant contrib functions as "parallel".
> >
> > Could you update the patch?
>
> So, it's been over a month since this request, and there doesn't seem
> to be an update to this patch.  The CommitFest is over in 2 days, so
> I've marked this "Returned with Feedback".  Shay, please feel free to
> resubmit for the next CommitFest.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: [HACKERS] Better tracking of free space during SP-GiST index build

[Robert Haas]

On Thu, Sep 22, 2016 at 1:37 PM, Tom Lane  wrote:
> OK, I'll think about how to do that more efficiently.  The smaller
> incremental improvement isn't surprising, because in this example the
> index would still be 90-something MB if it had no free space at all,
> so there's going to be decreasing returns from any additional work
> to avoid wasted free space.  But if we can do it cheaply, this does
> suggest that using pages in order by free space is of value.

Tom, are you planning to do something about this patch yet this
CommitFest, or leave it until later?

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


-- 
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] Tuplesort merge pre-reading

[Peter Geoghegan]

On Wed, Sep 28, 2016 at 5:04 PM, Heikki Linnakangas  wrote:
>> Not sure that I understand. I agree that each merge pass tends to use
>> roughly the same number of tapes, but the distribution of real runs on
>> tapes is quite unbalanced in earlier merge passes (due to dummy runs).
>> It looks like you're always using batch memory, even for non-final
>> merges. Won't that fail to be in balance much of the time because of
>> the lopsided distribution of runs? Tapes have an uneven amount of real
>> data in earlier merge passes.
>
>
> How does the distribution of the runs on the tapes matter?

The exact details are not really relevant to this discussion (I think
it's confusing that we simply say "Target Fibonacci run counts",
FWIW), but the simple fact that it can be quite uneven is.

This is why I never pursued batch memory for non-final merges. Isn't
that what you're doing here? You're pretty much always setting
"state->batchUsed = true".

>> I'm basically repeating myself here, but: I think it's incorrect that
>> LogicalTapeAssignReadBufferSize() is called so indiscriminately (more
>> generally, it is questionable that it is called in such a high level
>> routine, rather than the start of a specific merge pass -- I said so a
>> couple of times already).
>
>
> You can't release the tape buffer at the end of a pass, because the buffer
> of a tape will already be filled with data from the next run on the same
> tape.

Okay, but can't you just not use batch memory for non-final merges,
per my initial approach? That seems far cleaner.

-- 
Peter Geoghegan


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

Re: [HACKERS] pg_basebackup stream xlog to tar

[Magnus Hagander]

On Sep 28, 2016 19:11, "Robert Haas"  wrote:
>
> On Mon, Sep 5, 2016 at 4:01 AM, Michael Paquier
>  wrote:
> > [ review comments ]
>
> This thread has been sitting idle for more than 3 weeks, so I'm
> marking it "Returned with Feedback" in the CommitFest application.
> Magnus, Michael's latest round of comments seem pretty trivial, so
> perhaps you want to just fix whichever of them seem to you to have
> merit and commit without waiting for the next CommitFest.  Or, you can
> resubmit for the next CommitFest if you think it needs more review.
> But the CommitFest is just about over so it's time to clean out old
> entries, one way or the other.

Yeah, understood. I was planning to get back to it this week, but failed to
find the time. I'll still have some hope about later this week, but most
likely not until the next.

/Magnus

Re: [HACKERS] [PATCH] add option to pg_dumpall to exclude tables from the dump

[Robert Haas]

On Tue, Sep 6, 2016 at 9:37 PM, Gerdan Rezende dos Santos
 wrote:
> After review, I realized that there is a call to the function:
> doShellQuoting (pgdumpopts, OPTARG), which no longer seems to exist ...
> After understand the code, I saw that the call is appendShellString
> (pgdumpopts, OPTARG).
>
> Follow the patches already with the necessary corrections.

This doesn't seem to take into account the discussion between Tom Lane
and Jim Nasby about how this feature should work.

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


-- 
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] PassDownLimitBound for ForeignScan/CustomScan

[Robert Haas]

On Tue, Sep 13, 2016 at 9:07 PM, Kouhei Kaigai  wrote:
> It is because of just my time pressure around the patch submission days.
> I'll try to enhance postgres_fdw as a usage of this run-time optimization.

Time has (pretty much) expired for this CommitFest.  In any case, this
will amount to a whole new patch, not just a rework of the current
one.  So I'm going to mark this "Rejected" in the CommitFest, and I
suggest you start a new thread for the proposed approach if you get a
chance to work on it.

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


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

[HACKERS] CommitFest wrap-up

[Robert Haas]

As some of you probably noticed, I just made a sweep through
everything that was marked "Waiting on Author" in the CommitFest and
hadn't been updated in the last couple of days.  Most of those I
marked as "Returned with Feedback", but some of them got some other
status, one I committed, and a few I just sent a ping of some sort to
the thread.

With that cleanup, things now look like this:

Needs review: 46. Waiting on Author: 21. Ready for Committer: 18.
Committed: 94. Moved to next CF: 1. Rejected: 12. Returned with
Feedback: 27. Total: 219.

There is obviously a good bit of stuff that has been marked "Ready for
Committer"; it would be good if committers could take a look at those
and see if they agree that a commit might be possible without undue
effort.

There is also a lot of stuff that is still in a "Needs Review" state.
I suspect a good amount of that stuff has actually had some review,
and if somebody wants to help, it would be great to go through those
entries and change the status of any of them that are not actually
waiting for review - i.e. if they have been reviewed and are awaiting
an update, mark them as "Waiting on Author".  This will help us
separate the things that still really deserve a look from the stuff
that has already had one.

The things that haven't had any review yet should get a review if
that's at all possible.

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


-- 
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] psql casts aspersions on server reliability

[Petr Jelinek]

On 28/09/16 17:13, David Steele wrote:
> On 9/28/16 10:22 AM, Robert Haas wrote:
>> On Wed, Sep 28, 2016 at 9:14 AM, Tom Lane  wrote:
>>> Robert Haas  writes:
 psql tends to do things like this:
 rhaas=# select * from pg_stat_activity;
 FATAL:  terminating connection due to administrator command
 server closed the connection unexpectedly
 This probably means the server terminated abnormally
 before or while processing the request.
>>>
 Basically everything psql has to say about this is a lie:
>>>
>>> I cannot get terribly excited about this.  What you seem to be proposing
>>> is that psql try to intuit the reason for connection closure from the
>>> last error message it got, but that seems likely to lead to worse lies
>>> than printing a boilerplate message.
>>>
>>> I could go along with just dropping the last sentence ("This probably...")
>>> if the last error we got was FATAL level.  I don't find "unexpectedly"
>>> to be problematic here: from the point of view of psql, and probably
>>> of its user, the shutdown *was* unexpected.
>>
>> I don't care very much whether we try to intuit the reason for
>> connection closure or not; it could be done, but I don't feel that it
>> has to be done.  My bigger point is that currently psql speculates
>> that the reason for *every* connection closure is abnormal server
>> termination, which is actually a very rare event.
>>
>> It may have been common when that message was added.
>> 1a17447be1186fdd36391c58a2a0209f613d89c4 changed the wording this
>> message in 2001, and the original message seems to date to
>> 011ee13131f6fa2f6dbafd3827b70d051cb28f64 in 1996.  And my guess is at
>> that time the server probably did just roll over and die with some
>> regularity.  But today it usually doesn't.  It's neither helpful nor
>> good PR for libpq to guess that the most likely cause of a server
>> disconnection is server unreliability.
>>
>> I have seen actual instances of customers getting upset by this
>> message even though the server had been shut down quite cleanly.  The
>> message got into a logfile and induced minor panic.  Fortunately, I
>> have not seen this happen lately.
> 
> +1 for making this error message less frightening.  I have also had to
> explain it away on occasion.
> 

+1 I've seen this being misleading way too often.

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


-- 
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] hstore: add hstore_length function

[Robert Haas]

On Wed, Jun 8, 2016 at 10:44 AM, Robert Haas  wrote:
> On Mon, Jun 6, 2016 at 7:57 PM, Korbin Hoffman  wrote:
>> With regards to your second point- I've been maintaining consistency
>> with the rest of the hstore module. Hstore's _size is internally
>> stored as a uint, but all uses of HS_COUNT across the feature end up
>> stored in a signed int. I could only find (grep) a few occurrences of
>> PG_RETURN_UINT32 across the entire codebase, and none in the hstore
>> module. If there's strong consensus for change, though, I'm happy to
>> do so.
>
> The PG_RETURN_BLAH macro chosen should match the declared return type
> of that function.  So if your function, for example, returns int4 (or
> integer, which is the same thing), PG_RETURN_INT32 is correct.
>
> There are no built-in SQL datatypes for unsigned integers, which is
> why you did not find many uses of PG_RETURN_UINT32 in the code base.

Since this patch was never updated in response to this review, I am
marking it "Returned with Feedback" in this CommitFest.  If it is
updated, it can be resubmitted to a future CommitFest.

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


-- 
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] Set log_line_prefix and application name in test drivers

[Robert Haas]

On Thu, Sep 15, 2016 at 5:18 PM, Robert Haas  wrote:
> On Sat, Aug 27, 2016 at 3:59 PM, Tom Lane  wrote:
>> Christoph Berg  writes:
>>> I've always been wondering why we don't set a log_line_prefix by
>>> default.
>>
>> I think the odds of getting to something that everyone would agree on
>> are nil, so I'm not excited about getting into that particular
>> bikeshed-painting discussion.  Look at the amount of trouble we're
>> having converging on a default for the regression tests, which are
>> a far narrower use-case than "everybody".
>
> Well, practically anything that includes a PID and the timestamp is
> going to be an improvement over the status quo.  Just because we can't
> all agree on what would be perfect does not mean that we can't do
> better than what we've got now.  +1 for trying.

Is there any chance we can move forward here, or is this effort doomed for now?

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


-- 
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] should xlog_outdesc modify its argument?

[Mark Dilger]

> On Sep 27, 2016, at 11:25 PM, Heikki Linnakangas  wrote:
> 
> On 09/28/2016 02:35 AM, Mark Dilger wrote:
>> The function
>> 
>>  static void xlog_outdesc(StringInfo buf, XLogReaderState *record);
>> 
>> in src/backend/access/transam/xlog.c is called by XLogInsertRecord,
>> and after returning a string describing an XLogRecord, it clears the
>> state data in its XLogReaderState argument.  That mixes the read-only
>> semantics of "give me a string that describes this argument" and the
>> read-write semantics of "clear out the value in this argument".
> 
> I don't see where the "clears the state data" is happening. Can you elaborate?

My apologies.  At the bottom of the function, it calls through the function 
pointer

RmgrTable[rmid].rm_desc(buf, record);

which is set up to call various *_desc functions.  I must have chased through
those function pointers incorrectly, as I can't find the problem now that I am
reviewing all those functions.

Sorry for the noise,

Mark Dilger



-- 
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] Tuplesort merge pre-reading

[Heikki Linnakangas]

On 09/28/2016 07:11 PM, Peter Geoghegan wrote:

On Wed, Sep 28, 2016 at 5:04 PM, Heikki Linnakangas  wrote:

Not sure that I understand. I agree that each merge pass tends to use
roughly the same number of tapes, but the distribution of real runs on
tapes is quite unbalanced in earlier merge passes (due to dummy runs).
It looks like you're always using batch memory, even for non-final
merges. Won't that fail to be in balance much of the time because of
the lopsided distribution of runs? Tapes have an uneven amount of real
data in earlier merge passes.



How does the distribution of the runs on the tapes matter?


The exact details are not really relevant to this discussion (I think
it's confusing that we simply say "Target Fibonacci run counts",
FWIW), but the simple fact that it can be quite uneven is.


Well, I claim that the fact that the distribution of runs is uneven, 
does not matter. Can you explain why you think it does?



This is why I never pursued batch memory for non-final merges. Isn't
that what you're doing here? You're pretty much always setting
"state->batchUsed = true".


Yep. As the patch stands, we wouldn't really need batchUsed, as we know 
that it's always true when merging, and false otherwise. But I kept it, 
as it seems like that might not always be true - we might use batch 
memory when building the initial runs, for example - and because it 
seems nice to have an explicit flag for it, for readability and 
debugging purposes.



I'm basically repeating myself here, but: I think it's incorrect that
LogicalTapeAssignReadBufferSize() is called so indiscriminately (more
generally, it is questionable that it is called in such a high level
routine, rather than the start of a specific merge pass -- I said so a
couple of times already).



You can't release the tape buffer at the end of a pass, because the buffer
of a tape will already be filled with data from the next run on the same
tape.


Okay, but can't you just not use batch memory for non-final merges,
per my initial approach? That seems far cleaner.


Why? I don't see why the final merge should behave differently from the 
non-final ones.


- Heikki



--
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] Change error code for hstore syntax error

[Robert Haas]

On Sun, Sep 4, 2016 at 7:15 PM, Marko Tiikkaja  wrote:
> On 2016-05-09 19:42, Sherrylyn Branchaw wrote:
>>
>> I'm attaching a revised patch; please let me know if there are any other
>> issues before I submit to the commitfest.
>
> I think this is mostly good, but these two should be changed:
>
>   errmsg("unexpected end of string: \"%s\"", state->begin)
>   errmsg("syntax error at position %d: \"%s\"", ...)
>
> Right now, aside from the error code, these two look like they're reporting
> about an error in the SQL statement itself, and not in an input value for a
> type.  I think they should look more like this:
>
>   errmsg("invalid input syntax for type hstore: \"%s\"", string),
>   errdetail("Unexpected end of input.")
>
> If possible, it might also make sense to provide more information than
> "unexpected end of string".  For example: what character were you expecting
> to find, or what were you scanning?  I didn't look too closely what exactly
> could be done here.  I'll leave that part to you.

Since no revised patch has been forthcoming and the CommitFest is due
to end shortly, I've marked this "Returned with Feedback".  Sherrylyn,
please feel free to update the patch and resubmit to the next
CommitFest.

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


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

Re: [HACKERS] Proposal for changes to recovery.conf API

[Robert Haas]

On Tue, Sep 6, 2016 at 10:11 AM, David Steele  wrote:
> On 9/6/16 8:07 AM, Robert Haas wrote:
>> On Wed, Aug 31, 2016 at 9:45 PM, Simon Riggs 
>> wrote:
>>> Related cleanup
>>> * Promotion signal file is now called "promote.trigger" rather than
>>> just "promote"
>>> * Remove user configurable "trigger_file" mechanism - use
>>> "promote.trigger" for all cases
>>
>>
>> I'm in favor of this.  I don't think that it's very hard for authors
>> of backup tools to adapt to this new world, and I don't see that
>> allowing configurability here does anything other than create more
>> cases to worry about.
>
> +1 from a backup tool author.

It's time to wrap up this CommitFest, and this thread doesn't seem to
contain anything that looks like a committable patch.  So, I'm marking
this "Returned with Feedback".  I hope that the fact that there's been
no discussion for the last three weeks doesn't mean this effort is
dead; I would like very much to see it move forward.

Thanks,

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


-- 
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] Sample configuration files

[Robert Haas]

On Thu, Sep 8, 2016 at 8:52 AM, Tom Lane  wrote:
> Vik Fearing  writes:
>> I noticed that this patch has been marked Waiting on Author with no
>> comment.  Peter, what more should I be doing right now while waiting for
>> Martín's review?
>
> FWIW, I agree with the upthread misgivings about whether this is actually
> a useful effort.  Even if we installed the sample config files somewhere
> (something there is not consensus for AFAICT), they would not actually
> *do* anything useful as standalone files.  I suppose you are imagining
> that people would either manually concatenate them onto postgresql.conf
> or insert an include directive for them into postgresql.conf, but neither
> of those things sound pleasant or maintainable.
>
> Moreover, it's not clear why anyone would do that at all in the age of
> ALTER SYSTEM SET.
>
> I suggest that it'd be more fruitful to view this as a documentation
> effort; that is, in each contrib module's SGML documentation file provide
> a standardized section listing all its parameters and their default
> settings.  That would be something that could be copied-and-pasted from
> into either an editor window on postgresql.conf for the old guard, or
> an ALTER SYSTEM SET command for the new.

So, tallying up the votes, one person has spoken in favor of this
(Martín Marqués) and two against it (Tom Lane and Robert Haas).  One
presumes the author is also in favor, so that's a 2-2 tie.  That's not
exactly a consensus against this effort, but it's not a ringing
endorsement, either.  It's hard for me to imagine anything getting
committed here unless some more people think it's a good idea.

So, anyone else have an opinion, pro or con?

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


-- 
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] less expensive pg_buffercache on big shmem

[Robert Haas]

On Tue, Sep 20, 2016 at 7:43 PM, Tomas Vondra
 wrote:
> On 09/02/2016 11:01 AM, Robert Haas wrote:
>>
>> On Fri, Sep 2, 2016 at 8:49 AM, Andres Freund  wrote:
>>>
>>> On 2016-09-02 08:31:42 +0530, Robert Haas wrote:

 I wonder whether we ought to just switch from the consistent method to
 the semiconsistent method and call it good.
>>>
>>>
>>> +1. I think, before long, we're going to have to switch away from having
>>> locks & partitions in the first place. So I don't see a problem relaxing
>>> this. It's not like that consistency really buys you anything...  I'd
>>> even consider not using any locks.
>>
>> I think we certainly want to lock the buffer header, because otherwise
>> we might get a torn read of the buffer tag, which doesn't seem good.
>> But it's not obvious to me that there's any point in taking the lock
>> on the buffer mapping partition; I'm thinking that doesn't really do
>> anything unless we lock them all, and we all seem to agree that's
>> going too far.
>
> +1 from me to only locking the buffer headers. IMHO that's perfectly fine
> for the purpose of this extension.

So, I think we have agreement on the way forward here, but what we
don't have is a committable patch.  I'm willing to commit one before
the end of this CommitFest if somebody produces one RSN; otherwise,
this is going to have to go into the "Returned with Feedback" bucket.

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


-- 
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] Showing parallel status in \df+

[Pavel Stehule]

Hi

2016-09-28 18:57 GMT+02:00 Tom Lane :

> Pavel Stehule  writes:
> > 2016-09-28 16:03 GMT+02:00 Tom Lane :
> >> I propose to push my current patch (ie, move PL function
> >> source code to \df+ footers), and we can use it in HEAD for awhile
> >> and see what we think.  We can alway improve or revert it later.
>
> > I had some objection to format of source code - it should be full source
> > code, not just header and body.
>
> That would be redundant with stuff that's in the main part of the \df
> display.  I really don't need to see the argument types twice, for
> instance.
>

I am sorry, I disagree. Proposed form is hard readable. Is not possible to
simply copy/paste.

I cannot to imagine any use case for proposed format.

Regards

Pavel


>
> regards, tom lane
>

Re: [HACKERS] Tuplesort merge pre-reading

[Peter Geoghegan]

On Wed, Sep 28, 2016 at 5:11 PM, Peter Geoghegan  wrote:
> This is why I never pursued batch memory for non-final merges. Isn't
> that what you're doing here? You're pretty much always setting
> "state->batchUsed = true".

Wait. I guess you feel you have to, since it wouldn't be okay to use
almost no memory per tape on non-final merges.

You're able to throw out so much code here in large part because you
give almost all memory over to logtape.c (e.g., you don't manage each
tape's share of "slots" anymore -- better to give everything to
logtape.c). So, with your patch, you would actually only have one
caller tuple in memory at once per tape for a merge that doesn't use
batch memory (if you made it so that a merge *could* avoid the use of
batch memory, as I suggest).

In summary, under your scheme, the "batchUsed" variable contains a
tautological value, since you cannot sensibly not use batch memory.
(This is even true with !state->tuples callers).

Do I have that right? If so, this seems rather awkward. Hmm.

-- 
Peter Geoghegan


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

Re: [HACKERS] [COMMITTERS] pgsql: pg_ctl: Detect current standby state from pg_control

[Peter Eisentraut]

On 9/28/16 12:44 AM, Michael Paquier wrote:
> On Tue, Sep 27, 2016 at 9:55 AM, Michael Paquier
>  wrote:
>> > Seems overcomplicated to me. How about returning the control file all
>> > the time and let the caller pfree the result? You could then use
>> > crc_ok in pg_ctl.c's get_control_dbstate() to do the decision-making.
> In short I would just go with the attached and call it a day.

Pushed that way.  Thanks!

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


-- 
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] compiler warning read_objtype_from_string()

[Alvaro Herrera]

Peter Eisentraut wrote:
> I'm getting the following compiler warning (using nondefault
> optimization options):
> 
> objectaddress.c: In function 'read_objtype_from_string':
> objectaddress.c:2309:9: error: 'type' may be used uninitialized in this
> function [-Werror=maybe-uninitialized]
>   return type;

Umm.  I think it can only be uninitialized if we fall out of the end of
the array, in which case we're supposed to throw the ERROR and never
return.  Is that not working?

> The comment for the function says
> 
>  * Return ObjectType for the given object type as given by
>  * getObjectTypeDescription; if no valid ObjectType code exists, but it's a
>  * possible output type from getObjectTypeDescription, return -1.
> 
> But the claim that it can return -1 does not seem supported by the code.

Actually, it is -- but the -1 value comes from the ObjectType array.
Perhaps the comment should state that explicitely.

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


-- 
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] identity columns

[Robert Haas]

On Mon, Sep 12, 2016 at 5:02 PM, Peter Eisentraut
 wrote:
> Thank you for this extensive testing.  I will work on getting the bugs
> fixed.

It looks like the patch has not been updated; since the CommitFest is
(hopefully) wrapping up, I am marking this "Returned with Feedback"
for now.

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


-- 
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] Better tracking of free space during SP-GiST index build

[Robert Haas]

On Sun, Sep 25, 2016 at 3:28 PM, Tomas Vondra
 wrote:
> Sure, that would be useful.
>
> I think it would be useful to make repository of such data sets, so that
> patch authors & reviewers can get a reasonable collection of data sets if
> needed, instead of scrambling every time. Opinions?

In theory, great idea.  In practice, I suspect the problem will be
that nobody will know what the use case for a particular data set was
supposed to be, and therefore it'll become a collection of files
nobody knows what to do with.

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


-- 
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] IF (NOT) EXISTS in psql-completion

[Robert Haas]

On Tue, Sep 20, 2016 at 3:50 AM, Michael Paquier
 wrote:
> On Mon, Sep 19, 2016 at 6:11 PM, Pavel Stehule  
> wrote:
>> I am thinking so commit's description should be inside README
>
> Horiguchi-san, your patch has some whitespace issues, you may want to
> get a run with git diff --check. Here are some things I have spotted:
> src/bin/psql/tab-complete.c:1074: trailing whitespace.
> +"MATERIALIZED VIEW",
> src/bin/psql/tab-complete.c:2621: trailing whitespace.
> +   COMPLETE_WITH_QUERY(Query_for_list_of_roles,
>
> This set of patches is making psql tab completion move into a better
> shape, particularly with 0001 that removes the legendary huge if-elif
> and just the routine return immediately in case of a keyword match.
> Things could be a little bit more shortened by for example not doing
> the refactoring of the tab macros because they are just needed in
> tab-complete.c. The other patches introduce further improvements for
> the existing infrastructure, but that's a lot of things just for
> adding IF [NOT] EXISTS to be honest.
>
> Testing a bit, I have noticed that for example trying to after typing
> "create table if", if I attempt to do a tab completion "not exists"
> does not show up. I suspect that the other commands are failing at
> that as well.

This patch hasn't been updated in over a week and we're just about out
of time for this CommitFest, so I've marked it "Returned with
Feedback" for now.  If it gets updated, it can be resubmitted for the
next CommitFest.

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


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

Re: [HACKERS] pg_basebackup stream xlog to tar

[Robert Haas]

On Mon, Sep 5, 2016 at 4:01 AM, Michael Paquier
 wrote:
> [ review comments ]

This thread has been sitting idle for more than 3 weeks, so I'm
marking it "Returned with Feedback" in the CommitFest application.
Magnus, Michael's latest round of comments seem pretty trivial, so
perhaps you want to just fix whichever of them seem to you to have
merit and commit without waiting for the next CommitFest.  Or, you can
resubmit for the next CommitFest if you think it needs more review.
But the CommitFest is just about over so it's time to clean out old
entries, one way or the other.

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


-- 
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] Truncating/vacuuming relations on full tablespaces

[Robert Haas]

On Thu, Sep 8, 2016 at 2:46 AM, Haribabu Kommi  wrote:
> Patch needs rebase, it is failing to apply on latest master.
> And also there are some pending comment fix from Robert.

It's been almost three weeks and this hasn't been updated, so I think
it's pretty clear that it should be marked "Returned with Feedback" at
this point.  I'll go do that.  Asif, if you update the patch, you can
resubmit for the next CommitFest.  Please make sure that all review
comments already given are addressed in your next revision so that
reviewers don't waste time giving you the same comments again.

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


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

[HACKERS] Batches, error handling and transaction in the protocol

[Shay Rojansky]

Hi everyone, I'd appreciate some guidance on an issue that's been raised
with Npgsql, input from other driver writers would be especially helpful.

Npgsql currently supports batching (or pipelining) to avoid roundtrips, and
sends a Sync message only at the end of the batch (so
Parse1/Bind1/Describe1/Execute1/Parse2/Bind2/Describe2/Execute2/Sync). The
reasoning is that if the first statement in the batch fails, the others
shouldn't be processed. This seems to be the standard approach (the
proposed patch for libpq seems to do the same).

At the same time, if the batch doesn't occur within an explicit transaction
(i.e. after BEGIN), it is automatically wrapped in an implicit transaction,
with Sync committing it. This can, for example, provoke deadlocks if two
batches try to update the same rows in reverse order. The problem is that
the user didn't request a transaction in any way - they're just using
batching to avoid roundtrips and their intention is to be in autocommit
mode.

One possible solution for this would be to insert a Sync after every
execute in the batch, rather than a single Sync at the very end. This would
make batches work the same as unbatched statements, and would resolve the
deadlocks. However, behavior in case of error would be problematic:
PostgreSQL would continue executing later messages if earlier ones failed,
Npgsql would have to deal with multiple errors, etc.

More generally speaking, the protocol appears to couple two different
things which may be unrelated. On the one hand, we have a protocol sync
mechanism for error recovery (skip until Sync). One the other hand, we have
an implicit transaction for extended query messages until that same Sync.
It seems valid to want to have error recovery without an implicit
transaction, but this doesn't seem supported by the current protocol (I
could add a note for v4).

Finally, to give more context, a Microsoft developer ran into this while
running ASP.NET benchmarks over Npgsql and its Entity Framework Core ORM
provider. One of EFCore's great new features is that it batches database
updates into a single roundtrip, but this triggered deadlocks. Whereas in
many cases it's OK to tell users to solve the deadlocks by properly
ordering their statements, when an ORM is creating the batch it's a more
difficult proposition.

Thanks for any thoughts or guidance!

Shay

Re: [HACKERS] Better tracking of free space during SP-GiST index build

[Tom Lane]

Robert Haas  writes:
> On Thu, Sep 22, 2016 at 1:37 PM, Tom Lane  wrote:
>> OK, I'll think about how to do that more efficiently.  The smaller
>> incremental improvement isn't surprising, because in this example the
>> index would still be 90-something MB if it had no free space at all,
>> so there's going to be decreasing returns from any additional work
>> to avoid wasted free space.  But if we can do it cheaply, this does
>> suggest that using pages in order by free space is of value.

> Tom, are you planning to do something about this patch yet this
> CommitFest, or leave it until later?

I doubt I will get to it this week, so let's mark it RWF for this fest.

regards, tom lane


-- 
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] asynchronous execution

[Kyotaro HORIGUCHI]

Sorry for delayed response, I'll have enough time from now and
address this.

At Fri, 23 Sep 2016 21:09:03 -0400, Robert Haas  wrote 
in 
> Well, I promised to post this, so here it is.  It's not really working
> all that well at this point, and it's definitely not doing anything
> that interesting, but you can see the outline of what I have in mind.
> Since Kyotaro Horiguchi found that my previous design had a
> system-wide performance impact due to the ExecProcNode changes, I
> decided to take a different approach here: I created an async
> infrastructure where both the requestor and the requestee have to be
> specifically modified to support parallelism, and then modified Append
> and ForeignScan to cooperate using the new interface.  Hopefully that
> means that anything other than those two nodes will suffer no
> performance impact.  Of course, it might have other problems
> 
> Some notes:
> 
> - EvalPlanQual rechecks are broken.
> - EXPLAIN ANALYZE instrumentation is broken.
> - ExecReScanAppend is broken, because the async stuff needs some way
> of canceling an async request and I didn't invent anything like that
> yet.
> - The postgres_fdw changes pretend to be async but aren't actually.
> It's just a demo of (part of) the interface at this point.
> - The postgres_fdw changes also report all pg-fdw paths as
> async-capable, but actually the direct-modify ones aren't, so the
> regression tests fail.
> - Errors in the executor can leak the WaitEventSet.  Probably we need
> to modify ResourceOwners to be able to own WaitEventSets.
> - There are probably other bugs, too.
> 
> Whee!
> 
> Note that I've tried to solve the re-entrancy problems by (1) putting
> all of the event loop's state inside the EState rather than in local
> variables and (2) having the function that is called to report arrival
> of a result be thoroughly different than the function that is used to
> return a tuple to a synchronous caller.
> 
> Comments welcome, if you're feeling brave enough to look at anything
> this half-baked.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

[HACKERS] compiler warning read_objtype_from_string()

[Peter Eisentraut]

I'm getting the following compiler warning (using nondefault
optimization options):

objectaddress.c: In function 'read_objtype_from_string':
objectaddress.c:2309:9: error: 'type' may be used uninitialized in this
function [-Werror=maybe-uninitialized]
  return type;

The comment for the function says

 * Return ObjectType for the given object type as given by
 * getObjectTypeDescription; if no valid ObjectType code exists, but it's a
 * possible output type from getObjectTypeDescription, return -1.

But the claim that it can return -1 does not seem supported by the code.

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


-- 
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] Cache Hash Index meta page.

[Mithun Cy]

On Tue, Sep 27, 2016 at 1:53 AM, Jeff Janes  wrote:
 > I think that this needs to be updated again for v8 of concurrent and v5
of wal

Adding the rebased patch over [1] + [2]

[1] Concurrent Hash index.

[2] Wal for hash index.


-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


cache_hash_index_metapage_onAmit_05_03_with_wall.patch
Description: Binary data

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

Re: [HACKERS] Tuplesort merge pre-reading

[Heikki Linnakangas]

On 09/28/2016 06:05 PM, Peter Geoghegan wrote:

On Thu, Sep 15, 2016 at 9:51 PM, Heikki Linnakangas  wrote:

I don't think it makes much difference in practice, because most merge
passes use all, or almost all, of the available tapes. BTW, I think the
polyphase algorithm prefers to do all the merges that don't use all tapes
upfront, so that the last final merge always uses all the tapes. I'm not
100% sure about that, but that's my understanding of the algorithm, and
that's what I've seen in my testing.


Not sure that I understand. I agree that each merge pass tends to use
roughly the same number of tapes, but the distribution of real runs on
tapes is quite unbalanced in earlier merge passes (due to dummy runs).
It looks like you're always using batch memory, even for non-final
merges. Won't that fail to be in balance much of the time because of
the lopsided distribution of runs? Tapes have an uneven amount of real
data in earlier merge passes.


How does the distribution of the runs on the tapes matter?


+   usedBlocks = 0;
+   for (tapenum = 0; tapenum < state->maxTapes; tapenum++)
+   {
+   int64   numBlocks = blocksPerTape + (tapenum < remainder ? 1 : 0);
+
+   if (numBlocks > MaxAllocSize / BLCKSZ)
+   numBlocks = MaxAllocSize / BLCKSZ;
+   LogicalTapeAssignReadBufferSize(state->tapeset, tapenum,
+   numBlocks * BLCKSZ);
+   usedBlocks += numBlocks;
+   }
+   USEMEM(state, usedBlocks * BLCKSZ);


I'm basically repeating myself here, but: I think it's incorrect that
LogicalTapeAssignReadBufferSize() is called so indiscriminately (more
generally, it is questionable that it is called in such a high level
routine, rather than the start of a specific merge pass -- I said so a
couple of times already).


You can't release the tape buffer at the end of a pass, because the 
buffer of a tape will already be filled with data from the next run on 
the same tape.


- Heikki



--
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] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[David Fetter]

On Thu, Sep 29, 2016 at 11:12:11AM +1300, Thomas Munro wrote:
> On Mon, Sep 26, 2016 at 5:11 PM, Thomas Munro
>  wrote:
> > On Mon, Sep 26, 2016 at 1:18 PM, Thomas Munro
> >  wrote:
> >>
> >> On Mon, Sep 19, 2016 at 4:02 PM, David Fetter  wrote:
> >> >
> >> > [training_wheels_004.patch]
> >>
> >> [review]
> 
> Ping.

Please find attached the next revision.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
diff --git a/contrib/Makefile b/contrib/Makefile
index 25263c0..4bd456f 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -40,6 +40,7 @@ SUBDIRS = \
pgstattuple \
pg_visibility   \
postgres_fdw\
+   require_where   \
seg \
spi \
tablefunc   \
diff --git a/contrib/require_where/Makefile b/contrib/require_where/Makefile
new file mode 100644
index 000..0cf3663
--- /dev/null
+++ b/contrib/require_where/Makefile
@@ -0,0 +1,17 @@
+# contrib/require_where/Makefile
+
+MODULE_big = require_where
+OBJS = require_where.o
+
+PGFILEDESC = 'require_where - require DELETE and/or UPDATE to have a WHERE 
clause'
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS = $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/require_where
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_builddir)/contrib/contrib-global.mk
+endif
diff --git a/contrib/require_where/data/test_require_where.data 
b/contrib/require_where/data/test_require_where.data
new file mode 100644
index 000..d4a29d8
--- /dev/null
+++ b/contrib/require_where/data/test_require_where.data
@@ -0,0 +1,16 @@
+Four
+score
+and
+seven
+years
+ago
+our
+fathers
+brought
+forth
+on
+this
+continent
+a
+new
+nation
diff --git a/contrib/require_where/expected/require_where.out 
b/contrib/require_where/expected/require_where.out
new file mode 100644
index 000..0876e13
--- /dev/null
+++ b/contrib/require_where/expected/require_where.out
@@ -0,0 +1,12 @@
+LOAD
+CREATE TABLE
+COPY 16
+UPDATE 16
+SET
+psql:sql/require_where.sql:17: ERROR:  UPDATE requires a WHERE clause
+HINT:  To update all rows, use "WHERE true" or similar.
+SET
+psql:sql/require_where.sql:21: ERROR:  DELETE requires a WHERE clause
+HINT:  To delete all rows, use "WHERE true" or similar.
+SET
+DELETE 16
diff --git a/contrib/require_where/require_where.c 
b/contrib/require_where/require_where.c
new file mode 100644
index 000..27cbc25
--- /dev/null
+++ b/contrib/require_where/require_where.c
@@ -0,0 +1,92 @@
+/*
+ * --
+ *
+ * require_where.c
+ *
+ * Copyright (C) 2016, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ * contrib/require_where/require_where.c
+ *
+ * --
+ */
+#include "postgres.h"
+
+#include "fmgr.h"
+
+#include "parser/analyze.h"
+
+#include "utils/elog.h"
+#include "utils/guc.h"
+
+PG_MODULE_MAGIC;
+
+void   _PG_init(void);
+void   _PG_fini(void);
+
+static post_parse_analyze_hook_type original_post_parse_analyze_hook = 
NULL;
+static boolrequire_where_delete = false;
+static boolrequire_where_update = false;
+
+static void
+require_where_check(ParseState *pstate, Query *query)
+{
+
+   if (require_where_delete && query->commandType == CMD_DELETE)
+   {
+   Assert(query->jointree != NULL);
+   if (query->jointree->quals == NULL)
+   ereport(ERROR,
+   (errcode(ERRCODE_SYNTAX_ERROR),
+errmsg("DELETE requires a WHERE clause 
when require_where.delete is set to on"),
+errhint("To delete all rows, use 
\"WHERE true\" or similar.")));
+   }
+
+   if (require_where_update && query->commandType == CMD_UPDATE)
+   {
+   Assert(query->jointree != NULL);
+   if (query->jointree->quals == NULL)
+   ereport(ERROR,
+   (errcode(ERRCODE_SYNTAX_ERROR),
+errmsg("UPDATE requires a WHERE clause 
when require_where.delete is set to on"),
+errhint("To update all rows, use 
\"WHERE true\" or similar.")));
+   }
+
+   if (original_post_parse_analyze_hook != NULL)
+   (*original_post_parse_analyze_hook) (pstate, query);
+}
+
+void
+_PG_init(void)
+{
+   DefineCustomBoolVariable("require_where.delete",
+ 

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Haribabu Kommi]

On Thu, Sep 22, 2016 at 3:05 AM, Alvaro Herrera 
wrote:

> Peter Eisentraut wrote:
>
> > How about having the tag not be a column name but a row entry.  So you'd
> > do something like
> >
> > SELECT * FROM pg_stat_sql WHERE tag = 'ALTER VIEW';
> >
> > That way, we don't have to keep updating (and re-debating) this when new
> > command types or subtypes are added.  And queries written for future
> > versions will not fail when run against old servers.
>
> Yeah, good idea.
>

Yes, Having it as a row entry is good.



> Let's also discuss the interface from the stats collector.  Currently we
> have some 20 new SQL functions, all alike, each loading the whole data
> and returning a single counter, and then the view invokes each function
> separately.  That doesn't seem great to me.  How about having a single C
> function that returns the whole thing as a SRF instead, and the view is
> just a single function invocation -- something like pg_lock_status
> filling pg_locks in one go.
>
> Another consideration is that the present patch lumps together all ALTER
> cases in a single counter.  This isn't great, but at the same time we
> don't want to bloat the stat files by having hundreds of counters per
> database, do we?


Currently, The SQL stats is a fixed size counter to track the all the ALTER
cases as single counter. So while sending the stats from the backend to
stats collector at the end of the transaction, the cost is same, because of
it's fixed size. This approach adds overhead to send and read the stats
is minimal.

With the following approach, I feel it is possible to support the counter at
command tag level.

Add a Global and local Hash to keep track of the counters by using the
command tag as the key, this hash table increases dynamically whenever
a new type of SQL command gets executed. The Local Hash data is passed
to stats collector whenever the transaction gets committed.

The problem I am thinking is that, Sending data from Hash and populating
the Hash from stats file for all the command tags adds some overhead.


Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Sample configuration files

[Vik Fearing]

On 09/29/2016 05:55 AM, Michael Paquier wrote:
> On Thu, Sep 29, 2016 at 2:25 AM, Robert Haas  wrote:
>> So, anyone else have an opinion, pro or con?
> 
> Going through this thread, I'd vote -1. This is a documentation effort
> mainly, and installing those files has zero effect if they are not
> loaded via include_if_exists or include in postgresql.conf.

Just the other day, I needed this patch yet again but had to go look up
the documentation instead.

I wonder if it would be a good idea to have a postgresql.conf.d
directory that postgresql.conf would include_dir by default.  These
could then live in there and all I would have had to do is uncomment the
values I wanted.

This patch doesn't do that, of course, but I could easily write a patch
that does.  Would that go over better with the -1ers?
-- 
Vik Fearing  +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


-- 
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] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[David Fetter]

On Thu, Sep 29, 2016 at 11:12:11AM +1300, Thomas Munro wrote:
> On Mon, Sep 26, 2016 at 5:11 PM, Thomas Munro
>  wrote:
> > On Mon, Sep 26, 2016 at 1:18 PM, Thomas Munro
> >  wrote:
> >>
> >> On Mon, Sep 19, 2016 at 4:02 PM, David Fetter  wrote:
> >> >
> >> > [training_wheels_004.patch]
> >>
> >> [review]
> 
> Ping.

I'll have another revision out as soon as I get some more test cases.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
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] compiler warning read_objtype_from_string()

[Tom Lane]

Alvaro Herrera  writes:
> Peter Eisentraut wrote:
>> I'm getting the following compiler warning (using nondefault
>> optimization options):
>> objectaddress.c: In function 'read_objtype_from_string':
>> objectaddress.c:2309:9: error: 'type' may be used uninitialized in this
>> function [-Werror=maybe-uninitialized]
>> return type;

> Umm.  I think it can only be uninitialized if we fall out of the end of
> the array, in which case we're supposed to throw the ERROR and never
> return.  Is that not working?

I do not think you should assume that the compiler is smart enough to
deduce that, nor that all compilers even know ereport(ERROR) doesn't
return.  Personally I don't see the point of the "type" variable at
all, anyway.  I would have written this as

inti;

for (i = 0; i < lengthof(ObjectTypeMap); i++)
{
if (strcmp(ObjectTypeMap[i].tm_name, objtype) == 0)
return ObjectTypeMap[i].tm_type;
}
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 errmsg("unrecognized object type \"%s\"", objtype)));
return -1;/* keep compiler quiet */

regards, tom lane


-- 
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] Set log_line_prefix and application name in test drivers

[Robert Haas]

On Wed, Sep 28, 2016 at 6:07 PM, Alvaro Herrera
 wrote:
> I thought Peter's suggestion for regression test drivers was a good one
> and I see no reason to block that.  Why do you (Tom) object so strongly
> against having a different one on buildfarm than elsewhere?  I'd rather
> have buildfarm adopt the new suggestion than having buildfarm drive the
> new stuff.
>
> Adopting a default prefix is a different question.  For one thing IMHO
> it should not have %a (application name).  Christoph's suggestion
> (Debian's default) seemed good.

Yeah, I like Cristoph's suggestion fine.  It meets my criteria of
"includes timestamp and PID" and overall seems reasonable.   If we
adopted that across the board, it wouldn't be too much different from
what Peter proposed for the regression test.  Just to compare.

Christoph/Debian:
log_line_prefix = '%t [%p-%l] %q%u@%d '
Peter:
log_line_prefix = '%t [%p]: [%l] %qapp=%a '

So Peter's got %p and %l separated by "]: [" whereas Christoph has
them separated only by a dash.  Presumably that's minor.  Then they've
both got %q.  After that, Christoph has %u@%d, which seems reasonable
for an actual system, and Peter's got app=%a, which is better for the
regression tests because the user name will depend on the UNIX
username of the person running the tests.

So how about we adopt both suggestions, except changing Peter's to '%t
[%p-%l] %qapp=%a ' so that they are a bit more similar?  I bet that
would make more people happier than it would make less happy.

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


-- 
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] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[Thomas Munro]

On Mon, Sep 26, 2016 at 5:11 PM, Thomas Munro
 wrote:
> On Mon, Sep 26, 2016 at 1:18 PM, Thomas Munro
>  wrote:
>>
>> On Mon, Sep 19, 2016 at 4:02 PM, David Fetter  wrote:
>> >
>> > [training_wheels_004.patch]
>>
>> [review]

Ping.

-- 
Thomas Munro
http://www.enterprisedb.com


-- 
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] Speed up Clog Access by increasing CLOG buffers

[Tomas Vondra]

On 09/28/2016 05:39 PM, Robert Haas wrote:

On Tue, Sep 27, 2016 at 5:15 PM, Tomas Vondra
 wrote:

So, I got the results from 3.10.101 (only the pgbench data), and it looks
like this:

 3.10.101   1  8 16 32 64128192

 granular-locking2582  18492  33416  49583  53759  53572  51295
 no-content-lock 2580  18666  33860  49976  54382  54012  51549
 group-update2635  18877  33806  49525  54787  54117  51718
 master  2630  18783  33630  49451  54104  53199  50497

So 3.10.101 performs even better tnan 3.2.80 (and much better than 4.5.5),
and there's no sign any of the patches making a difference.


I'm sure that you mentioned this upthread somewhere, but I can't
immediately find it.  What scale factor are you testing here?



300, the same scale factor as Dilip.



It strikes me that the larger the scale factor, the more
CLogControlLock contention we expect to have.  We'll pretty much do
one CLOG access per update, and the more rows there are, the more
chance there is that the next update hits an "old" row that hasn't
been updated in a long time.  So a larger scale factor also
increases the number of active CLOG pages and, presumably therefore,
the amount of CLOG paging activity.

>

So, is 300 too little? I don't think so, because Dilip saw some benefit 
from that. Or what scale factor do we think is needed to reproduce the 
benefit? My machine has 256GB of ram, so I can easily go up to 15000 and 
still keep everything in RAM. But is it worth it?


regards

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


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

Re: [HACKERS] Fix checkpoint skip logic on idle systems by tracking LSN progress

[David Steele]

On 9/28/16 3:35 AM, Michael Paquier wrote:
> On Wed, Sep 28, 2016 at 6:12 AM, David Steele  wrote:
>> I tried the attached patch set and noticed an interesting behavior. With
>> archive_timeout=5 whenever I made a change I would get a WAL segment within
>> a few seconds as expected then another one would follow a few minutes later.
> 
> That's intentional. We may be able to make XLOG_SWITCH records as not
> updating the progress LSN, but I wanted to tackle that as a separate
> patch once we got the basics done correctly, which is still what I
> think this patch is doing. I should have been more precise upthread:
> this patch makes the handling of checkpoint skip logic correct for
> only standby snapshots, not segment switches, and puts the infra to
> handle other things.

OK, I've done functional testing and this patch seems to work as
specified (including the caveat noted above).  Some comments:

* [PATCH 1/3] hs-checkpoints-v12-1

+++ b/src/backend/access/transam/xlog.c
+* Taking a lock is as well necessary to prevent potential torn reads
+* on some platforms.

How about, "Taking a lock is also necessary..."

+   LWLockAcquire([i].l.lock, LW_EXCLUSIVE);

That's a lot of exclusive locks and that would seem to have performance
implications.  It seems to me this is going to be a hard one to
benchmark because the regression (if any) would only be seen under heavy
load on a very large system.

In general I agree with the other comments that this could end up being
a problem.  On the other hand, since the additional locks are only taken
at checkpoint or archive_timeout it may not be that big a deal.

+++ b/src/backend/access/transam/xloginsert.c * Should this record
include the replication origin if one is set up?

Outdated comment from XLogIncludeOrigin().

* [PATCH 2/3] hs-checkpoints-v12-2

+++ b/src/backend/postmaster/checkpointer.c
+   /* OK, it's time to switch */
+   elog(LOG, "Request XLog Switch");

LOG level seems a bit much here, perhaps DEBUG1?

* [PATCH 3/3] hs-checkpoints-v12-3

+* switch segment only when any substantial progress have made 
from
+* reasons will cause last_xlog_switch_lsn stay behind but it 
doesn't

How about, "Switch segment only when substantial progress has been made
after the last segment was switched by a timeout.  Segment switching for
other reasons..."

+++ b/src/backend/access/transam/xlog.c
+   elog(LOG, "Not a forced or shutdown checkpoint: progress_lsn 
%X/%X,
ckpt %X/%X",
+   elog(LOG, "Checkpoint is skipped");
+   elog(LOG, "snapshot taken by checkpoint %X/%X",

Same for the above, seems like it would just be noise for most users.

+++ b/src/backend/postmaster/bgwriter.c
+   elog(LOG, "snapshot taken by bgwriter %X/%X",

Ditto.

I don't see any unintended consequences in this patch but it doesn't
mean there aren't any.  I'm definitely concerned by the exclusive locks
but it may turn out they do not actually represent a bottleneck.

This does seem like the kind of patch that should get committed very
early in the release cycle to allow maximum time for regression testing.

-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] Bug in to_timestamp().

[Tom Lane]

Artur Zakirov  writes:
> - now DCH_cache_getnew() is called after parse_format(). Because now 
> parse_format() can raise an error and in the next attempt 
> DCH_cache_search() could return broken cache entry.

I started looking at your 0001-to-timestamp-format-checking-v4.patch
and this point immediately jumped out at me.  Currently the code relies
... without any documentation ... on no elog being thrown out of
parse_format().  That's at the very least trouble waiting to happen.
There's a hack to deal with errors from within the NUMDesc_prepare
subroutine, but it's a pretty ugly and underdocumented hack.  And what
you had here was randomly different from that solution, too.

After a bit of thought it seemed to me that a much cleaner fix is to add
a "valid" flag to the cache entries, which we can leave clear until we
have finished parsing the new format string.  That avoids adding extra
data copying as you suggested, removes the need for PG_TRY, and just
generally seems cleaner and more bulletproof.

I've pushed a patch that does it that way.  The 0001 patch will need
to be rebased over that (might just require removal of some hunks,
not sure).

I also pushed 0002-to-timestamp-validation-v2.patch with some revisions
(it'd broken acceptance of BC dates, among other things, but I think
I fixed everything).

Since you told us earlier that you'd be on vacation through the end of
September, I'm assuming that nothing more will happen on this patch during
this commitfest, so I will mark the CF entry Returned With Feedback.

regards, tom lane


-- 
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] kqueue

[Thomas Munro]

On Thu, Sep 29, 2016 at 9:09 AM, Keith Fiske  wrote:
> On Thu, Sep 15, 2016 at 11:11 PM, Thomas Munro
>  wrote:
>> Ok, here's a version tweaked to use EVFILT_PROC for postmaster death
>> detection instead of the pipe, as Tom Lane suggested in another
>> thread[1].
>>
>> [...]
>
> Ran benchmarks on unaltered 96rc1 again just to be safe. Those are first.
> Decided to throw a 32 process test in there as well to see if there's
> anything going on between 4 and 64

Thanks!  A summary:

┌──┬─┬───┬┬───┐
│   code   │ clients │  average  │ standard_deviation │  median   │
├──┼─┼───┼┼───┤
│ 9.6rc1   │   1 │ 25704.923 │108.766 │ 25731.006 │
│ 9.6rc1   │   4 │ 94032.889 │322.562 │ 94123.436 │
│ 9.6rc1   │  32 │ 86647.401 │ 33.616 │ 86664.849 │
│ 9.6rc1   │  64 │ 79360.680 │   1217.453 │ 79941.243 │
│ 9.6rc1/kqueue-v6 │   1 │ 24569.683 │   1433.339 │ 25146.434 │
│ 9.6rc1/kqueue-v6 │   4 │ 93435.450 │ 50.214 │ 93442.716 │
│ 9.6rc1/kqueue-v6 │  32 │ 88000.328 │135.143 │ 87891.856 │
│ 9.6rc1/kqueue-v6 │  64 │ 71726.034 │   4784.794 │ 72271.146 │
└──┴─┴───┴┴───┘

┌─┬───┬───┬──┐
│ clients │ unpatched │  patched  │  percent_change  │
├─┼───┼───┼──┤
│   1 │ 25731.006 │ 25146.434 │ -2.271858317548874692000 │
│   4 │ 94123.436 │ 93442.716 │ -0.72322051651408051 │
│  32 │ 86664.849 │ 87891.856 │  1.415807001521458833000 │
│  64 │ 79941.243 │ 72271.146 │ -9.594668173973727179000 │
└─┴───┴───┴──┘

The variation in the patched 64 client numbers is quite large, ranging
from ~66.5k to ~79.5k.  The highest number matched the unpatched
numbers which ranged 77.9k to 80k.  I wonder if that is noise and we
need to run longer (in which case the best outcome might be 'this
patch is neutral on FreeBSD'), or if something the patch does is doing
is causing that (for example maybe EVFILT_PROC proc filters causes
contention on the process table lock).

Matteo's results with the v6 patch on a low end NetBSD machine were
not good.  But the report at [1] implies that larger NetBSD and
OpenBSD systems have terrible problems with the
poll-postmaster-alive-pipe approach, which this EVFILT_PROC approach
would seem to address pretty well.

It's difficult to draw any conclusions at this point.

[1] https://www.postgresql.org/message-id/flat/20160915135755.GC19008%40genua.de

-- 
Thomas Munro
http://www.enterprisedb.com

-- 
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] Showing parallel status in \df+

[Tom Lane]

Pavel Stehule  writes:
> We are in cycle because prosrc field is used for two independent features -
> and then it can be hard to find a agreement.

I thought pretty much everyone was on board with the idea of keeping
prosrc in \df+ for internal/C-language functions (and then probably
renaming the column, since it isn't actually source code in that case).
The argument is over what to do for PL functions, which is only one use
case not two.

regards, tom lane


-- 
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] compiler warning read_objtype_from_string()

[Alvaro Herrera]

Tom Lane wrote:

> I do not think you should assume that the compiler is smart enough to
> deduce that, nor that all compilers even know ereport(ERROR) doesn't
> return.  Personally I don't see the point of the "type" variable at
> all, anyway.  I would have written this as
> 
> [code]

Makes sense.  I will patch it that way.

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


-- 
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] Set log_line_prefix and application name in test drivers

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> Perhaps we should first try to get a consensus on the regression test
>> use-case.

> I thought Peter's suggestion for regression test drivers was a good one
> and I see no reason to block that.  Why do you (Tom) object so strongly
> against having a different one on buildfarm than elsewhere?  I'd rather
> have buildfarm adopt the new suggestion than having buildfarm drive the
> new stuff.

Well, my point is only that if you can't convince Andrew to sync the
buildfarm's choices with whatever your proposal is, then you haven't
got consensus.

regards, tom lane


-- 
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] Set log_line_prefix and application name in test drivers

[Tom Lane]

Robert Haas  writes:
> On Thu, Sep 15, 2016 at 5:18 PM, Robert Haas  wrote:
>> On Sat, Aug 27, 2016 at 3:59 PM, Tom Lane  wrote:
>>> I think the odds of getting to something that everyone would agree on
>>> are nil, so I'm not excited about getting into that particular
>>> bikeshed-painting discussion.  Look at the amount of trouble we're
>>> having converging on a default for the regression tests, which are
>>> a far narrower use-case than "everybody".

>> Well, practically anything that includes a PID and the timestamp is
>> going to be an improvement over the status quo.  Just because we can't
>> all agree on what would be perfect does not mean that we can't do
>> better than what we've got now.  +1 for trying.

> Is there any chance we can move forward here, or is this effort doomed for 
> now?

It seemed like nobody wanted to try to push this forward, and it will take
somebody actively pushing, IMO, for something to happen.

Perhaps we should first try to get a consensus on the regression test
use-case.

regards, tom lane


-- 
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] Set log_line_prefix and application name in test drivers

[Alvaro Herrera]

Tom Lane wrote:
> Robert Haas  writes:
> > On Thu, Sep 15, 2016 at 5:18 PM, Robert Haas  wrote:
> >> On Sat, Aug 27, 2016 at 3:59 PM, Tom Lane  wrote:
> >>> I think the odds of getting to something that everyone would agree on
> >>> are nil, so I'm not excited about getting into that particular
> >>> bikeshed-painting discussion.  Look at the amount of trouble we're
> >>> having converging on a default for the regression tests, which are
> >>> a far narrower use-case than "everybody".
> 
> >> Well, practically anything that includes a PID and the timestamp is
> >> going to be an improvement over the status quo.  Just because we can't
> >> all agree on what would be perfect does not mean that we can't do
> >> better than what we've got now.  +1 for trying.
> 
> > Is there any chance we can move forward here, or is this effort doomed for 
> > now?
> 
> It seemed like nobody wanted to try to push this forward, and it will take
> somebody actively pushing, IMO, for something to happen.
> 
> Perhaps we should first try to get a consensus on the regression test
> use-case.

I thought Peter's suggestion for regression test drivers was a good one
and I see no reason to block that.  Why do you (Tom) object so strongly
against having a different one on buildfarm than elsewhere?  I'd rather
have buildfarm adopt the new suggestion than having buildfarm drive the
new stuff.

Adopting a default prefix is a different question.  For one thing IMHO
it should not have %a (application name).  Christoph's suggestion
(Debian's default) seemed good.

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


-- 
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] Binary I/O for isn extension

[Robert Haas]

On Wed, Sep 28, 2016 at 2:05 PM, Shay Rojansky  wrote:
> Sorry about this, I just haven't had a free moment (and it's definitely not
> very high priority...)

No issues, just cleaning house.

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


-- 
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] Better tracking of free space during SP-GiST index build

[Robert Haas]

On Wed, Sep 28, 2016 at 2:11 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Thu, Sep 22, 2016 at 1:37 PM, Tom Lane  wrote:
>>> OK, I'll think about how to do that more efficiently.  The smaller
>>> incremental improvement isn't surprising, because in this example the
>>> index would still be 90-something MB if it had no free space at all,
>>> so there's going to be decreasing returns from any additional work
>>> to avoid wasted free space.  But if we can do it cheaply, this does
>>> suggest that using pages in order by free space is of value.
>
>> Tom, are you planning to do something about this patch yet this
>> CommitFest, or leave it until later?
>
> I doubt I will get to it this week, so let's mark it RWF for this fest.

OK, done.  Thanks for the reply.

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


-- 
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] Add support for restrictive RLS policies

[Stephen Frost]

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> Stephen, the typo "awseome" in the tests is a bit distracting.  Can you
> please fix it?

Done.

> I think you should use braces here, not parens:

Fixed.

> I don't think this paragraph is right -- you should call out each of the
> values PERMISSIVE and RESTRICTIVE (in upper case) instead.  Also note
> typos "Alternativly" and "visibillity".

Done.

> I dislike the "AND"d and "OR"d spelling of those terms.  Currently they
> only appear in comments within rowsecurity.c (of your authorship too, I
> imagine).  I think it'd be better to find actual words for those
> actions.

Reworded to not attempt to use AND and OR as verbs.  Additionally, a
patch is also included to remove those from the comments in
rowsecurity.c.  There are a few other places where we have "OR'd" in the
code base, but I didn't think it made sense to change those as part of
this effort.

* Jeevan Chalke (jeevan.cha...@enterprisedb.com) wrote:
> With this patch, pg_policy catalog now has seven columns, however
> Natts_pg_policy is still set to 6. It should be updated to 7 now.
> Doing this regression seems OK.

Ah, certainly interesting that it only caused incorrect behavior and not
a crash (and no incorrect behavior even on my system, at least with the
regression tests and other testing I've done).

Fixed.

> 1. In documentation, we should put both permissive as well as restrictive in
> the header like permissive|restrictive. 

I'm not sure which place in the documentation you are referring to
here..?  [ AS { PERMISSIVE | RESTRICTIVE } ] was added to the CREATE
POLICY synopsis documentation.

> 2. "If the policy is a "permissive" or "restrictive" policy." seems broken
> as
> sentence starts with "If" and there is no other part to it. Will it be
> better
> to say "Specifies whether the policy is a "permissive" or "restrictive"
> policy."?

Rewrote this to be clearer, I hope.

> 3. " .. a policy can instead by "restrictive""
> Do you mean "instead be" here?

This was also rewritten.

> 4. It will be good if we have an example for this in section
> "5.7. Row Security Policies"

I haven't added one yet, but will plan to do so.

> 5. AS ( PERMISSIVE | RESTRICTIVE )
> should be '{' and '}' instead of parenthesis.

Fixed.

> 6. I think following changes are irrelevant for this patch.
> Should be submitted as separate patch if required.

As mentioned, this is tab-completion for the new options which this
patch introduces.

> 7. Natts_pg_policy should be updated to 7 now.

Fixed.

> 8. In following error, $2 and @2 should be used to correctly display the
> option and location.

Fixed.

> I think adding negative test to test this error should be added in
> regression.

Done.

> 9. Need to update following comments in gram.y to reflect new changes.

Done.

> 10. ALTER POLICY has no changes for this. Any reason why that is not
> considered here.

As mentioned, I don't see a use-case for it currently.

> 11. Will it be better to use boolean for polpermissive in _policyInfo?
> And then set that appropriately while getting the policies. So that later we
> only need to test the boolean avoiding string comparison.

Done.

> 12. Since PERMISSIVE is default, we should dump only "RESTRICTIVE" when
> appropriate, like other default cases.

Done, for this and the other defaults.

> 13. Since PERMISSIVE is default, do we need changes like below?
> -\QCREATE POLICY p1 ON test_table FOR ALL TO PUBLIC \E
> +\QCREATE POLICY p1 ON test_table AS PERMISSIVE FOR ALL TO
> PUBLIC \E

Updated to reflect what pg_dump now produces.

> 14. While displaying policy details in permissionsList, per syntax, we
> should
> display (RESTRICT) before the command option. Also will it be better to use
> (RESTRICTIVE) instead of (RESTRICT)?

Fixed.

> 15. Similarly in describeOneTableDetails() too, can we have RESTRICTIVE
> after
> policy name and before command option ?
> If we do that then changes related to adding "POLICY" followed by
> "RESTRICTIVE"
> will be straight forward.

Fixed.

> 16. It be good to have test-coverage for permissionsList,
> describeOneTableDetails and dump-restore changes. Please add those.

Done.

> 17. In pg_policies view, we need to add details related to PERMISSIVE and
> RESTRICTIVE. Please do so. Also add test for it.

Done.

> 18. Fix typos pointed earlier by Alvera.

Done.

Updated patch attached.

Thanks!

Stephen
From 020871cddd3c7187bd55a52673cae0af17a95246 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Thu, 1 Sep 2016 02:11:30 -0400
Subject: [PATCH 1/2] Add support for restrictive RLS policies

We have had support for restrictive RLS policies since 9.5, but they
were only available through extensions which use the appropriate hooks.
This adds support into the grammer, catalog, psql and pg_dump for
restrictive RLS policies, thus reducing the cases where an extension is
necessary.
---
 doc/src/sgml/ref/create_policy.sgml   |  28 
 

Re: [HACKERS] Showing parallel status in \df+

[Alvaro Herrera]

Pavel Stehule wrote:

> I am sorry, I disagree. Proposed form is hard readable. Is not possible to
> simply copy/paste.

Why do you care?  You can use \sf if you want to copy the
function code.

> I cannot to imagine any use case for proposed format.

My vote (which was not counted by Stephen) was to remove it from \df+
altogether.  I stand by that.  People who are used to seeing the output
in \df+ will wonder "where the heck did it go" and eventually figure it
out, at which point it's no longer a problem.  We're not breaking
anyone's scripts, that's for sure.

If we're not removing it, I +0 support the option of moving it to
footers.  I'm -1 on doing nothing.

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


-- 
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] Showing parallel status in \df+

[Stephen Frost]

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> Pavel Stehule wrote:
> > I cannot to imagine any use case for proposed format.
> 
> My vote (which was not counted by Stephen) was to remove it from \df+

Oh, sorry about that, not sure how I missed it. :/

> altogether.  I stand by that.  People who are used to seeing the output
> in \df+ will wonder "where the heck did it go" and eventually figure it
> out, at which point it's no longer a problem.  We're not breaking
> anyone's scripts, that's for sure.
> 
> If we're not removing it, I +0 support the option of moving it to
> footers.  I'm -1 on doing nothing.

This is more-or-less the same position that I have.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Hash Indexes

[Robert Haas]

On Tue, Sep 27, 2016 at 3:06 PM, Jesper Pedersen
 wrote:
> I have been running various tests, and applications with this patch together
> with the WAL v5 patch [1].
>
> As I havn't seen any failures and doesn't currently have additional feedback
> I'm moving this patch to "Ready for Committer" for their feedback.

Cool!  Thanks for reviewing.

Amit, can you please split the buffer manager changes in this patch
into a separate patch?  I think those changes can be committed first
and then we can try to deal with the rest of it.  Instead of adding
ConditionalLockBufferShared, I think we should add an "int mode"
argument to the existing ConditionalLockBuffer() function.  That way
is more consistent with LockBuffer().  It means an API break for any
third-party code that's calling this function, but that doesn't seem
like a big problem.  There are only 10 callers of
ConditionalLockBuffer() in our source tree and only one of those is in
contrib, so probably there isn't much third-party code that will be
affected by this, and I think it's worth it for the long-term
cleanliness.

As for CheckBufferForCleanup, I think that looks OK, but: (1) please
add an Assert() that we hold an exclusive lock on the buffer, using
LWLockHeldByMeInMode; and (2) I think we should rename it to something
like IsBufferCleanupOK.  Then, when it's used, it reads like English:
if (IsBufferCleanupOK(buf)) { /* clean up the buffer */ }.

I'll write another email with my thoughts about the rest of the patch.
For the record, Amit and I have had extensive discussions about this
effort off-list, and as Amit noted in his original post, the design is
based on suggestions which I previously posted to the list suggesting
how the issues with hash indexes might get fixed.  Therefore, I don't
expect to have too many basic disagreements regarding the design of
the patch; if anyone else does, please speak up.  Andres already
stated that he things working on btree-over-hash would be more
beneficial than fixing hash, but at this point it seems like he's the
only one who takes that position.  Even if we accept that working on
the hash AM is a reasonable thing to do, it doesn't follow that the
design Amit has adopted here is ideal.  I think it's reasonably good,
but that's only to be expected considering that I drafted the original
version of it and have been involved in subsequent discussions;
someone else might dislike something that I thought was OK, and any
such opinions certainly deserve a fair hearing.  To be clear, It's
been a long time since I've looked at any of the actual code in this
patch and I have at no point studied it deeply, so I expect that I may
find a fair number of things that I'm not happy with in detail, and
I'll write those up along with any design-level concerns that I do
have.  This should in no way forestall review from anyone else who
wants to get involved.

Thanks,

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


-- 
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] Floating point comparison inconsistencies of the geometric types

[Kevin Grittner]

On Wed, Sep 28, 2016 at 12:02 PM, Emre Hasegeli  wrote:

>> `make check` finds differences per the attached.  Please
>> investigate why the regression tests are failing and what the
>> appropriate response is.
>
> I fixed the first one and workaround the second with COLLATE "C".  I
> have how my changes caused this regression.
>
> "select_views" test runs "SELECT name, #thepath FROM iexit ORDER BY 1,
> 2" and expects to get rows in this order:
>
>>  I- 580Ramp |8
>>  I- 580/I-680  Ramp |2
>
> With the collation on my laptop, this is actually true:
>
>> regression=# select 'I- 580/I-680  Ramp' < 'I- 580   
>>  Ramp';
>>  ?column?
>> --
>>  t
>> (1 row)
>
> However, on the Linux server, I am testing it is not:
>
>> regression=# select 'I- 580Ramp' < 'I- 580/I-680 
>>  Ramp';
>>  ?column?
>> --
>>  f
>> (1 row)
>
> Do you know how it is not failing on the master?

Well, those two results are not contradictory -- notice that you
switched the order of the values in the comparison.  I don't think
you've really found the explanation yet.

>> [discussing inline static functions compared to macros for min()/max(), etc.]
>> I suspect that they will be as fast or faster, and they eliminate
>> the hazard of multiple evaluation, where a programmer might not be
>> aware of the multiple evaluation or of some side-effect of an
>> argument.
>
> I reworked the the patches to use inline functions and fixed the
> problems I found.  The new versions are attached.

Will take a look and post again.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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] Hash Indexes

[Andres Freund]

On 2016-09-28 15:04:30 -0400, Robert Haas wrote:
> Andres already
> stated that he things working on btree-over-hash would be more
> beneficial than fixing hash, but at this point it seems like he's the
> only one who takes that position.

Note that I did *NOT* take that position. I was saying that I think we
should evaluate whether that's not a better approach, doing some simple
performance comparisons.

Greetings,

Andres Freund


-- 
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] Password identifiers, protocol aging and SCRAM protocol

[Stephen Frost]

Heikki, Michael, Magnus,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Tue, Sep 27, 2016 at 10:42 PM, Heikki Linnakangas  wrote:
> > The libpq-side is not. Just calling random() won't do. We haven't needed for
> > random numbers in libpq before, but now we do. Is the pgcrypto solution
> > portable enough that we can use it in libpq?
> 
> Do you think that urandom would be enough then? The last time I took a
> look at that, I saw urandom on all modern platforms even those ones:
> OpenBSD, NetBSD, Solaris, SunOS. For Windows the CryptGen stuff would
> be nice enough I guess..

Magnus had been working on a patch that, as I recall, he thought was
portable and I believe could be used on both sides.

Magnus, would what you were working on be helpful here...?

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Showing parallel status in \df+

[Pavel Stehule]

2016-09-28 21:59 GMT+02:00 Alvaro Herrera :

> Pavel Stehule wrote:
>
> > I am sorry, I disagree. Proposed form is hard readable. Is not possible
> to
> > simply copy/paste.
>
> Why do you care?  You can use \sf if you want to copy the
> function code.
>

I know so I can use \sf. But I don't see any sense to have less readable
output of any psql command.


>
> > I cannot to imagine any use case for proposed format.
>
> My vote (which was not counted by Stephen) was to remove it from \df+
> altogether.  I stand by that.  People who are used to seeing the output
> in \df+ will wonder "where the heck did it go" and eventually figure it
> out, at which point it's no longer a problem.  We're not breaking
> anyone's scripts, that's for sure.
>

I prefer removing before proposed solution with proposed format.

We are in cycle because prosrc field is used for two independent features -
and then it can be hard to find a agreement.

Name of function in dll is some different than PL function body. But it is
stored and displayed in one field - and it is impossible do it well.

Regards

Pavel


>
> If we're not removing it, I +0 support the option of moving it to
> footers.  I'm -1 on doing nothing.
>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] ICU integration

[Thomas Munro]

On Fri, Sep 23, 2016 at 6:27 PM, Thomas Munro
 wrote:
> On Wed, Aug 31, 2016 at 2:46 PM, Peter Eisentraut
>  wrote:
>> Here is a patch I've been working on to allow the use of ICU for sorting
>> and other locale things.
>
> This is very interesting work, and it's great to see some development
> in this area.  I've been peripherally involved in more than one
> collation-change-broke-my-data incident over the years.  I took the
> patch for a quick spin today.  Here are a couple of initial
> observations.

This seems like a solid start, but there are unresolved questions
about both high level goals (versioning strategy etc) and also some
technical details with this WIP patch.  It looks like several people
have an interest and ideas in this area, but clearly there isn't going
to be a committable patch in the next 48 hours.  So I will set this to
'Returned with Feedback' for now.  If you think you'll have a new
patch for the next CF then it looks like you can still 'Move to Next
CF' from 'Returned with Feedback' state if appropriate.  Thanks!

-- 
Thomas Munro
http://www.enterprisedb.com


-- 
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] Hash Indexes

[Robert Haas]

On Wed, Sep 28, 2016 at 3:06 PM, Andres Freund  wrote:
> On 2016-09-28 15:04:30 -0400, Robert Haas wrote:
>> Andres already
>> stated that he things working on btree-over-hash would be more
>> beneficial than fixing hash, but at this point it seems like he's the
>> only one who takes that position.
>
> Note that I did *NOT* take that position. I was saying that I think we
> should evaluate whether that's not a better approach, doing some simple
> performance comparisons.

OK, sorry.  I evidently misunderstood your position, for which I apologize.

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


-- 
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] kqueue

[Keith Fiske]

On Thu, Sep 15, 2016 at 11:11 PM, Thomas Munro <
thomas.mu...@enterprisedb.com> wrote:

> On Thu, Sep 15, 2016 at 11:04 AM, Thomas Munro
>  wrote:
> > On Thu, Sep 15, 2016 at 10:48 AM, Keith Fiske  wrote:
> >> Thomas Munro brought up in #postgresql on freenode needing someone to
> test a
> >> patch on a larger FreeBSD server. I've got a pretty decent machine
> (3.1Ghz
> >> Quad Core Xeon E3-1220V3, 16GB ECC RAM, ZFS mirror on WD Red HDD) so
> offered
> >> to give it a try.
> >>
> >> Bench setup was:
> >> pgbench -i -s 100 -d postgres
> >>
> >> I ran this against 96rc1 instead of HEAD like most of the others in this
> >> thread seem to have done. Not sure if that makes a difference and can
> re-run
> >> if needed.
> >> With higher concurrency, this seems to cause decreased performance. You
> can
> >> tell which of the runs is the kqueue patch by looking at the path to
> >> pgbench.
> >
> > Thanks Keith.  So to summarise, you saw no change with 1 client, but
> > with 4 clients you saw a significant drop in performance (~93K TPS ->
> > ~80K TPS), and a smaller drop for 64 clients (~72 TPS -> ~68K TPS).
> > These results seem to be a nail in the coffin for this patch for now.
> >
> > Thanks to everyone who tested.  I might be back in a later commitfest
> > if I can figure out why and how to fix it.
>
> Ok, here's a version tweaked to use EVFILT_PROC for postmaster death
> detection instead of the pipe, as Tom Lane suggested in another
> thread[1].
>
> The pipe still exists and is used for PostmasterIsAlive(), and also
> for the race case where kevent discovers that the PID doesn't exist
> when you try to add it (presumably it died already, but we want to
> defer the report of that until you call EventSetWait, so in that case
> we stick the traditional pipe into the kqueue set as before so that
> it'll fire a readable-because-EOF event then).
>
> Still no change measurable on my laptop.  Keith, would you be able to
> test this on your rig and see if it sucks any less than the last one?
>
> [1] https://www.postgresql.org/message-id/13774.1473972000%40sss.pgh.pa.us
>
> --
> Thomas Munro
> http://www.enterprisedb.com
>


Ran benchmarks on unaltered 96rc1 again just to be safe. Those are first.
Decided to throw a 32 process test in there as well to see if there's
anything going on between 4 and 64

~/pgsql96rc1/bin/pgbench -i -s 100 -d pgbench -p 5496

[keith@corpus ~]$ /home/keith/pgsql96rc1/bin/pgbench -T 60 -j 1 -c 1 -M
prepared -S -p 5496 pgbench
starting vacuum...end.
transaction type: 
scaling factor: 100
query mode: prepared
number of clients: 1
number of threads: 1
duration: 60 s
number of transactions actually processed: 1543809
latency average: 0.039 ms
tps = 25729.749474 (including connections establishing)
tps = 25731.006414 (excluding connections establishing)
[keith@corpus ~]$ /home/keith/pgsql96rc1/bin/pgbench -T 60 -j 1 -c 1 -M
prepared -S -p 5496 pgbench
starting vacuum...end.
transaction type: 
scaling factor: 100
query mode: prepared
number of clients: 1
number of threads: 1
duration: 60 s
number of transactions actually processed: 1548340
latency average: 0.039 ms
tps = 25796.928387 (including connections establishing)
tps = 25798.275891 (excluding connections establishing)
[keith@corpus ~]$ /home/keith/pgsql96rc1/bin/pgbench -T 60 -j 1 -c 1 -M
prepared -S -p 5496 pgbench
starting vacuum...end.
transaction type: 
scaling factor: 100
query mode: prepared
number of clients: 1
number of threads: 1
duration: 60 s
number of transactions actually processed: 1535072
latency average: 0.039 ms
tps = 25584.182830 (including connections establishing)
tps = 25585.487246 (excluding connections establishing)

[keith@corpus ~]$ /home/keith/pgsql96rc1/bin/pgbench -T 60 -j 4 -c 4 -M
prepared -S -p 5496 pgbench
starting vacuum...end.
transaction type: 
scaling factor: 100
query mode: prepared
number of clients: 4
number of threads: 4
duration: 60 s
number of transactions actually processed: 5621013
latency average: 0.043 ms
tps = 93668.594248 (including connections establishing)
tps = 93674.730914 (excluding connections establishing)
[keith@corpus ~]$ /home/keith/pgsql96rc1/bin/pgbench -T 60 -j 4 -c 4 -M
prepared -S -p 5496 pgbench
starting vacuum...end.
transaction type: 
scaling factor: 100
query mode: prepared
number of clients: 4
number of threads: 4
duration: 60 s
number of transactions actually processed: 5659929
latency average: 0.042 ms
tps = 94293.572928 (including connections establishing)
tps = 94300.500395 (excluding connections establishing)
[keith@corpus ~]$ /home/keith/pgsql96rc1/bin/pgbench -T 60 -j 4 -c 4 -M
prepared -S -p 5496 pgbench
starting vacuum...end.
transaction type: 
scaling factor: 100
query mode: prepared
number of clients: 4
number of threads: 4
duration: 60 s
number of transactions actually processed: 5649572
latency average: 0.042 ms
tps = 94115.854165 (including connections establishing)
tps = 94123.436211 (excluding connections 

Re: [HACKERS] Set log_line_prefix and application name in test drivers

[Christoph Berg]

Re: Robert Haas 2016-09-28 

> > Well, practically anything that includes a PID and the timestamp is
> > going to be an improvement over the status quo.  Just because we can't
> > all agree on what would be perfect does not mean that we can't do
> > better than what we've got now.  +1 for trying.
> 
> Is there any chance we can move forward here, or is this effort doomed for 
> now?

IMHO it would make sense. Maybe we should collect a few suggestions,
and then take a poll?

Christoph


-- 
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] Floating point comparison inconsistencies of the geometric types

[Kevin Grittner]

On Wed, Sep 28, 2016 at 2:04 PM, Kevin Grittner  wrote:

> Will take a look and post again.

I am moving this patch to the next CF.  You'll be hearing from me
sometime after this CF is closed.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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] "Some tests to cover hash_index"

[Robert Haas]

On Tue, Sep 20, 2016 at 2:26 PM, Alvaro Herrera
 wrote:
> Why not use generate_series() queries to insert the appropriate number
> of tuples, instead of a handful of INSERT lines each time?  Since each
> insert is a separate transaction, that would probably be faster.
>
> Why do you have a plpgsql function just to create a cursor?  Wouldn't it
> be simpler to create the cursor in an SQL statement?

This patch hasn't been updated in over a week, so I'm marking it
Returned with Feedback.  I think this is a good effort and I hope
something committable will come from it, but with 2 days left it's not
going to happen this CF.

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


-- 
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] Issue with bgworker, SPI and pgstat_report_stat

[Robert Haas]

On Sat, Sep 3, 2016 at 12:29 AM, Michael Paquier
 wrote:
> On Sat, Sep 3, 2016 at 10:02 AM, Tomas Vondra
>  wrote:
>> In any case, I think adding the pgstat_report_stat() into worker_spi seems
>> like a reasonable (and backpatchable) fix.
>
> Doing just that sounds reasonable seen from here. I am wondering also
> if it would not be worth mentioning in the documentation of the
> bgworkers that users trying to emulate somewhat the behavior of a
> backend should look at PostgresMain(). The code in itself is full of
> hints as well.

Everybody seems happy with this fix for a first step, so I've
committed it and back-patched it to 9.3.

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


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

Re: [HACKERS] [PATCH] SortSupport for macaddr type

[Robert Haas]

On Wed, Sep 14, 2016 at 6:14 AM, Julien Rouhaud
 wrote:
> On 26/08/2016 19:44, Brandur wrote:
>> Hello,
> Hello,
>
>> I've attached a patch to add SortSupport for Postgres' macaddr which has the
>> effect of improving the performance of sorting operations for the type. The
>> strategy that I employ is very similar to that for UUID, which is to create
>> abbreviated keys by packing as many bytes from the MAC address as possible 
>> into
>> Datums, and then performing fast unsigned integer comparisons while sorting.
>>
>> I ran some informal local benchmarks, and for cardinality greater than 100k
>> rows, I see a speed up on `CREATE INDEX` of roughly 25% to 65%. (For those
>> interested, I put a few more numbers into a small report here [2].)
>>
>
> That's a nice improvement!
>
>> Admittedly, this is not quite as useful as speeding up sorting on a more 
>> common
>> data type like TEXT or UUID, but the change still seems like a useful
>> performance improvement. I largely wrote it as an exercise to familiarize
>> myself with the Postgres codebase.
>>
>> I'll add an entry into the current commitfest as suggested by the Postgres 
>> Wiki
>> and follow up here with a link.
>>
>> Thanks, and if anyone has feedback or other thoughts, let me know!
>>
>
> I just reviewed your patch.  It applies and compiles cleanly, and the
> abbrev feature works as intended.  There's not much to say since this is
> heavily inspired on the uuid SortSupport. The only really specific part
> is in the abbrev_converter function, and I don't see any issue with it.
>
> I have a few trivial comments:
>
> * you used macaddr_cmp_internal() function name, for uuid the same
> function is named uuid_internal_cmp().  Using the same naming pattern is
> probably better.
>
> * the function comment on macaddr_abbrev_convert() doesn't mention
> specific little-endian handling
>
> * "There will be two bytes of zero padding on the least significant end"
>
> "least significant bits" would be better
>
> * This patch will trigger quite a lot modifications after a pgindent
> run.  Could you try to run pgindent on mac.c before sending an updated
> patch?

Since it's been two weeks and this patch hasn't been updated in
response to this review, I have marked it "Returned with Feedback" in
the CommitFest.  If it is updated, it can be resubmitted for the next
CommitFest.

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


-- 
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] Change error code for hstore syntax error

[Sherrylyn Branchaw]

Since no revised patch has been forthcoming and the CommitFest is due
to end shortly, I've marked this "Returned with Feedback".  Sherrylyn,
please feel free to update the patch and resubmit to the next
CommitFest.

Will do, Robert, and many thanks to Marko for the feedback. I apologize for
the delay; I had surgery two days ago and will get back to this as soon as
possible.

Sherrylyn

Re: [HACKERS] Transaction user id through logical decoding

[Craig Ringer]

On 28 Sep. 2016 17:50, "valeriof"  wrote:
>
> Hi all,
> I'm developing a custom plugin to stream Postgres CDC changes to my client
> application. One of the info the application needs is the user id of the
> user who executed a certain transaction. I can see we have access to other
> transaction info (xid, lsn, changed data) but apparently the user id is
not
> available.
> Does anyone know if it is possible to extract this info in any way?

It is not recorded in WAL so it isn't possible as-is.

Also you can't assume a tx is all done by one user id. SET ROLE, SECURITY
DEFINER, etc. Even the session user can change during a tx (which IMO a
defect).

You have a couple of options. You could patch pg to add an option to xlog
user id with heap and heap2 rmgr writes, but I doubt it'd have much chance
of getting into core. You'd need to work out how to tell when the new info
was there too.

You could add a new rmgr that logs use is at tx start and whenever it
changes. Doing this robustly could be interesting but I think it'd have
more chance. 10.0 at the earliest though.

You could use a FOR EACH ROW trigger added to each table to xlog a logical
wal message (9.6 only) with the user id changing the row. Maybe optimise by
keeping a cache with the last id logged and only log again if it changes.
Care here is needed for cleanup at xact end, rolled back subxact handling
etc.

(If you don't care about handling the corner cases you could use a FOR EACH
STATEMENT trigger instead.)

You could use a special table in an extension schema that you insert rows
into to record the user who performed an action. Using a before trigger.
Delete the row as soon as you insert it since you only care about the wal
record. Then when decoding inserts examine the affected table oid. If it's
your special table, save the stored user id in output plugin state instead
of sending it to the peer as a normal insert. BDR has some things similar
to this for its handling of ddl replication, TRUNCATE, and global sequence
voting that you could take a look at; see bdr_output.c and bdr_apply.c .

> Thanks,
> Valerio
>
>
>
> --
> View this message in context:
http://postgresql.nabble.com/Transaction-user-id-through-logical-decoding-tp5923261.html
> Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.
>
>
> --
> 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] Speed up Clog Access by increasing CLOG buffers

[Robert Haas]

On Wed, Sep 28, 2016 at 6:45 PM, Tomas Vondra
 wrote:
> So, is 300 too little? I don't think so, because Dilip saw some benefit from
> that. Or what scale factor do we think is needed to reproduce the benefit?
> My machine has 256GB of ram, so I can easily go up to 15000 and still keep
> everything in RAM. But is it worth it?

Dunno.  But it might be worth a test or two at, say, 5000, just to see
if that makes any difference.

I feel like we must be missing something here.  If Dilip is seeing
huge speedups and you're seeing nothing, something is different, and
we don't know what it is.  Even if the test case is artificial, it
ought to be the same when one of you runs it as when the other runs
it.  Right?

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


-- 
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] Speed up Clog Access by increasing CLOG buffers

[Tomas Vondra]

On 09/29/2016 01:59 AM, Robert Haas wrote:

On Wed, Sep 28, 2016 at 6:45 PM, Tomas Vondra
 wrote:

So, is 300 too little? I don't think so, because Dilip saw some benefit from
that. Or what scale factor do we think is needed to reproduce the benefit?
My machine has 256GB of ram, so I can easily go up to 15000 and still keep
everything in RAM. But is it worth it?


Dunno. But it might be worth a test or two at, say, 5000, just to
see if that makes any difference.



OK, I have some benchmarks to run on that machine, but I'll do a few 
tests with scale 5000 - probably sometime next week. I don't think the 
delay matters very much, as it's clear the patch will end up with RwF in 
this CF round.



I feel like we must be missing something here.  If Dilip is seeing
huge speedups and you're seeing nothing, something is different, and
we don't know what it is.  Even if the test case is artificial, it
ought to be the same when one of you runs it as when the other runs
it.  Right?



Yes, definitely - we're missing something important, I think. One 
difference is that Dilip is using longer runs, but I don't think that's 
a problem (as I demonstrated how stable the results are).


I wonder what CPU model is Dilip using - I know it's x86, but not which 
generation it is. I'm using E5-4620 v1 Xeon, perhaps Dilip is using a 
newer model and it makes a difference (although that seems unlikely).


regards

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


--
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] Tracking wait event for latches

[Michael Paquier]

On Wed, Sep 28, 2016 at 9:45 PM, Robert Haas  wrote:
> On Wed, Sep 28, 2016 at 8:38 AM, Michael Paquier
>  wrote:
>> So should I change back the patch to have only one argument for the
>> eventId, and guess classId from it?
>
> Why would you need to guess?

Incorrect wording from me perhaps? i just meant that processing needs
to know what is the classId coming for a specific eventId.

> But, yes, I think one argument is much preferable.

OK. Here is the wanted patch. I have reduced the routines of WaitLatch
& friends to use only one argument, and added what is the classId
associated with a given eventId in an array of multiple fields, giving
something like that:
+ const struct wait_event_entry WaitEventEntries[] = {
+   /* Activity */
+   {WAIT_ACTIVITY, "ArchiverMain"},
[...]

I have cleaned up as well the inclusions of pgstat.h that I added
previously. Patch is attached.
-- 
Michael
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 8ca1c1c..9265e00 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -496,7 +496,8 @@ pgfdw_get_result(PGconn *conn, const char *query)
 			wc = WaitLatchOrSocket(MyLatch,
    WL_LATCH_SET | WL_SOCKET_READABLE,
    PQsocket(conn),
-   -1L);
+   -1L,
+   WE_EXTENSION);
 			ResetLatch(MyLatch);
 
 			CHECK_FOR_INTERRUPTS();
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index f400785..bb975c1 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -679,6 +679,42 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   11:34   0:00 postgres: ser
   buffer in question.
  
 
+
+ 
+  Activity: The server process is idle.  This is used by
+  system processes waiting for activity in their main processing loop.
+  wait_event will identify the specific wait point.
+ 
+
+
+ 
+  Extension: The server process is waiting for activity
+  in an extension module.  This category is useful for modules to
+  track custom waiting points.
+ 
+
+
+ 
+  Client: The server process is waiting for some activity
+  on a socket from user applications, and that the server expects
+  something to happen that is independent from its internal processes.
+  wait_event will identify the specific wait point.
+ 
+
+
+ 
+  IPC: The server process is waiting for some activity
+  from another process in the server.  wait_event will
+  identify the specific wait point.
+ 
+
+
+ 
+  Timeout: The server process is waiting for a timeout
+  to expire.  wait_event will identify the specific wait
+  point.
+ 
+

   
  
@@ -1085,6 +1121,143 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   11:34   0:00 postgres: ser
  BufferPin
  Waiting to acquire a pin on a buffer.
 
+
+ Activity
+ ArchiverMain
+ Waiting in main loop of the archiver process.
+
+
+ AutoVacuumMain
+ Waiting in main loop of autovacuum launcher process.
+
+
+ BgWriterHibernate
+ Waiting in background writer process, hibernating.
+
+
+ BgWriterMain
+ Waiting in main loop of background writer process background worker.
+
+
+ CheckpointerMain
+ Waiting in main loop of checkpointer process.
+
+
+ PgStatMain
+ Waiting in main loop of the statistics collector process.
+
+
+ RecoveryWalAll
+ Waiting for WAL from any kind of source (local, archive or stream) at recovery.
+
+
+ RecoveryWalStream
+ Waiting for WAL from a stream at recovery.
+
+
+ SysLoggerMain
+ Waiting in main loop of syslogger process.
+
+
+ WalReceiverMain
+ Waiting in main loop of WAL receiver process.
+
+
+ WalSenderMain
+ Waiting in main loop of WAL sender process.
+
+
+ WalWriterMain
+ Waiting in main loop of WAL writer process.
+
+
+ Client
+ SecureRead
+ Waiting to read data from a secure connection.
+
+
+ SecureWrite
+ Waiting to write data to a secure connection.
+
+
+ SSLOpenServer
+ Waiting for SSL while attempting connection.
+
+
+ WalReceiverWaitStart
+ Waiting for startup process to send initial data for streaming replication.
+
+
+ WalSenderWaitForWAL
+ Waiting for WAL to be 

Re: [HACKERS] PATCH: Exclude additional directories in pg_basebackup

[Peter Eisentraut]

On 9/28/16 2:45 AM, Michael Paquier wrote:
> After all that fixed, I have moved the patch to "Ready for Committer".
> Please use the updated patch though.

Committed after some cosmetic changes.

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


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

Re: [HACKERS] pg_basebackup creates a corrupt file for pg_stat_tmp and pg_replslot on a backup location

[Peter Eisentraut]

On 9/25/16 8:06 AM, Ashutosh Sharma wrote:
> Hi Peter,
> 
>> I just wanted to update you, I have taken this commit fest entry patch
>> to review because I think it will be addresses as part of "Exclude
>> additional directories in pg_basebackup", which I'm also reviewing.
>> Therefore, I'm not actually planning on discussing this patch further.
>> Please correct me if this assessment does not match your expectations.
> 
> Thanks for the update. I am absolutely OK with it. I feel it would be
> a good idea to review "Exclude additional directories in
> pg_basebackup" which also addresses the issue reported by me.

That has been committed.

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


-- 
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] Handling dropped attributes in pglogical_proto

[Michael Paquier]

On Wed, Sep 28, 2016 at 11:25 PM, Konstantin Knizhnik
 wrote:
> But if table was just altered and some attribute was removed from the table,
> then rel->natts can be greater than natts.

This is part of pglogical, so you may want to reply on the dedicated
thread or send directly a patch to them. By the way, this code path
may need to care as well about attisdropped. It is never good not to
check for it.
-- 
Michael


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

Re: [HACKERS] WAL consistency check facility

[Michael Paquier]

On Fri, Sep 16, 2016 at 10:36 PM, Michael Paquier
 wrote:
> On Fri, Sep 16, 2016 at 10:30 PM, Robert Haas  wrote:
>> I don't think you have the right to tell Kuntal that he has to move
>> the patch to the next CommitFest because there are unspecified things
>> about the current version you don't like.  If you don't have time to
>> review further, that's your call, but he can leave the patch as Needs
>> Review and see if someone else has time.
>
> No complain from here if done this way. I don't mean any offense :)

Seeing nothing happening, I have moved the patch to next CF as there
is a new version, but no reviews for it.
-- 
Michael


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

Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol

[Michael Paquier]

On Wed, Sep 28, 2016 at 8:55 PM, Michael Paquier
 wrote:
>> Our b64_encode routine does use whitespace, so we can't use it as is for
>> SCRAM. As the patch stands, we might never output anything long enough to
>> create linefeeds, but let's be tidy. The base64 implementation is about 100
>> lines of code, so perhaps we should just leave src/backend/utils/encode.c
>> alone, and make a new copy of the base64 routines in src/common.
>
> OK, I'll refresh that tomorrow with the rest. Thanks for the commit to
> extend password_encryption.

OK, so after more chatting with Heikki, here is a list of TODO items
and a summary of the state of things:
- base64 encoding routines should drop whitespace (' ', \r, \t), and
it would be better to just copy those from the backend's encode.c to
src/common/. No need to move escape and binary things, nor touch
backend's base64 routines.
- No need to move sha1.c to src/common/. Better to just get sha2.c
into src/common/ as we aim at SCRAM-SHA-256.
- random() called in the client is no good. We need something better here.
- The error handling needs to be reworked and should follow the
protocol presented by RFC5802, by sending back e= messages. This needs
a bit of work, not much I think though as the infra is in place in the
core patch.
- Let's discard the md5-or-scram optional thing in pg_hba.conf. This
complicates the error handling protocol.

I am marking this patch as returned with feedback for current CF and
will post a new set soon, moving it to the next CF once I have the new
set of patches ready for posting.
-- 
Michael


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

Re: [HACKERS] [GENERAL] C++ port of Postgres

[Thomas Munro]

On Mon, Sep 26, 2016 at 10:57 AM, Thomas Munro
 wrote:
> On Thu, Sep 1, 2016 at 1:41 AM, Peter Eisentraut
>  wrote:
>>
>> [trimmed cc list because of big attachments]
>>
>> On 8/16/16 4:22 PM, Jim Nasby wrote:
>> > Joy, do you have an idea what a *minimally invasive* patch for C++
>> > support would look like? That's certainly the first step here.
>>
>> I developed a minimally invasive patch for C++ support a few years ago
>> shortly after I wrote that blog post.  Since there appears to have been
>> some interest here now, I have updated that and split it up into logical
>> chunks.
>>
>> So here you go.
>
>
> I looked at a random selection of these patches this morning.

And this morning I looked at the rest of them.

> 0004-Fix-LDFLAGS-test-for-C.patch

Makes sense.

> 0005-Add-test-for-Wmissing-prototypes.patch

This does seem to follow the example of how we test for support for
other warning flags.

> 0006-Remove-unnecessary-prototypes.patch

Looks OK.

> 0007-Fix-incorrect-type-cast.patch

  /* array of check flags, reported to consistentFn */
- bool   *entryRes;
+ GinTernaryValue *entryRes;

Right.  That would be pretty dodgy even in C if we ever use stdbool.h,
because sizeof(_Bool) is implementation defined.  The
interchangeability relies on bool and GinTernaryValue both being
typedefs for 'char'.  (Not to mention the dangerous contradictions
possible with bools obtained that way: 'b == false || b == true' can
be false, which I guess has been thought about already and is off
topic here.)

I wonder if the following bit of gin.h should be more nuanced: maybe
it's OK to convert between bool and GinTernaryValue, but it's
definitely not OK to cast between pointers types?  Or maybe we should
have a function/macro to convert between the types explicitly and not
encourage people to consider them convertible.

  /*
   * A ternary value used by tri-consistent functions.
   *
   * For convenience, this is compatible with booleans. A boolean can be
   * safely cast to a GinTernaryValue.
   */
  typedef char GinTernaryValue;

> 0008-Add-necessary-type-cast.patch

Maybe instead of this:

- gcv.check = check;
+ gcv.check = (GinTernaryValue *) check;

... it would be better to do this?

-bool   *check = (bool *) PG_GETARG_POINTER(0);
+GinTernaryValue *check = (GinTernaryValue *) PG_GETARG_POINTER(0);

> 0009-Rename-some-typedefs-to-avoid-name-conflicts.patch

I don't know if it's a relevant precedent or not, but I noticed that
fdwapi.h, amapi.h and tsmapi.h used the convention that function
pointer types are named XXX_function, and then the members of a struct
behaving as a kind of vtable are named XXX.

> 0010-Reorder-some-things.patch

> Forward declarations of static variables are not possible in C++, so
> move the full definition before its use.

Right.

> 0011-Add-missing-fields-in-struct-initializations.patch

I don't undestand why this is necessary, unless you're explicitly
choosing to enable a warning like missing-field-initializers for C++
but not for C.  Implicit zero-initialisation of trailing missing
initialisers is a feature, not a bug.  Also I noticed that 0013 (or a
proper solution to the keyword collision problem) is needed before
this one.

> 0012-Separate-enum-from-struct.patch

Right.

> 0013-Avoid-C-key-words.patch

> This is not a long-term solution, because C header files that are
> included somewhere might have C++ awareness and will break if the key
> word is defined away.  But this shows the list of words that would have
> to be renamed around the code.

Right, let's rename them all directly.

> 0015-Fix-function-prototypes-for-C.patch

I wonder if (perhaps in some later later patch) walkers should take
const pointers and mutators non-const.  That may require propagating
constness around some more places.

> 0017-Don-t-define-bool-in-C.patch

Check.

> 0018-Change-TimeoutId-from-enum-to-integer.patch

This works, but I feel like we're losing something valuable if we
convert all our enums to ints just because some tiny bit of code
somewhere wants to loop over them.  Maybe we should we keep enums like
this, and do the necessary casting in the small number of places that
do int-like-stuff with them?  Like so:

diff --git a/src/backend/utils/misc/timeout.c b/src/backend/utils/misc/timeout.c
index 7171a7c..cc5b2c4 100644
--- a/src/backend/utils/misc/timeout.c
+++ b/src/backend/utils/misc/timeout.c
@@ -348,7 +348,7 @@ InitializeTimeouts(void)

for (i = 0; i < MAX_TIMEOUTS; i++)
{
-   all_timeouts[i].index = i;
+   all_timeouts[i].index = (TimeoutId) i;
all_timeouts[i].indicator = false;
all_timeouts[i].timeout_handler = NULL;
all_timeouts[i].start_time = 0;
@@ -379,7 +379,8 @@ RegisterTimeout(TimeoutId id, timeout_handler_proc handler)
if (id >= USER_TIMEOUT)
{
/* Allocate a user-defined timeout 

Re: [HACKERS] Set log_line_prefix and application name in test drivers

[Tom Lane]

Peter Eisentraut  writes:
> On 9/28/16 6:13 PM, Robert Haas wrote:
>> Christoph/Debian:
>> log_line_prefix = '%t [%p-%l] %q%u@%d '
>> Peter:
>> log_line_prefix = '%t [%p]: [%l] %qapp=%a '

> ...
> I don't know why it wants that "-1" there, and I'm actually not sure
> what the point of %l is in practice.  Those are separate issues that are
> having their own lively discussions at times.  I could drop the [%l]
> from my proposal if that causes concerns.

+1 for dropping %l --- seems to me that its main result is to add useless
bytes to the log.  Surely if you need to match up lines from the same
process, that's not that hard as long as %p is in there.

I'd also vote for dropping "app=" out of the regression test version;
again, that seems like basically dead weight.

regards, tom lane


-- 
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] Sample configuration files

[Michael Paquier]

On Thu, Sep 29, 2016 at 2:25 AM, Robert Haas  wrote:
> So, anyone else have an opinion, pro or con?

Going through this thread, I'd vote -1. This is a documentation effort
mainly, and installing those files has zero effect if they are not
loaded via include_if_exists or include in postgresql.conf.
-- 
Michael


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

Re: [HACKERS] Handling dropped attributes in pglogical_proto

[Petr Jelinek]

On 29/09/16 05:33, Michael Paquier wrote:
> On Wed, Sep 28, 2016 at 11:25 PM, Konstantin Knizhnik
>  wrote:
>> But if table was just altered and some attribute was removed from the table,
>> then rel->natts can be greater than natts.
> 
> This is part of pglogical, so you may want to reply on the dedicated
> thread or send directly a patch to them. By the way, this code path
> may need to care as well about attisdropped. It is never good not to
> check for it.
> 

Agreed this does not belong to hackers and should reported on github.

But just as note the rel variable is not Relation, it's
PGLogicalRelation which gets populated by relation message from the
upstream so if you observe the behavior mentioned in the original email,
you are probably doing something unexpected there (Konstantin is not
using vanilla pglogical). Also the attmap should never contain
attisdropped attributes.

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


-- 
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] Set log_line_prefix and application name in test drivers

[Peter Eisentraut]

On 9/28/16 6:07 PM, Alvaro Herrera wrote:
> Adopting a default prefix is a different question.

A default prefix would require different settings for syslog, plain
text, and possibly some of the other variants.  I'm all in favor of
figuring that out, but it needs more work.

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


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

Re: [HACKERS] Fix checkpoint skip logic on idle systems by tracking LSN progress

[Michael Paquier]

On Thu, Sep 29, 2016 at 7:45 AM, David Steele  wrote:
> OK, I've done functional testing and this patch seems to work as
> specified (including the caveat noted above).  Some comments:

Thanks!

> * [PATCH 1/3] hs-checkpoints-v12-1
>
> +++ b/src/backend/access/transam/xlog.c
> +* Taking a lock is as well necessary to prevent potential torn reads
> +* on some platforms.
>
> How about, "Taking a lock is also necessary..."
>
> +   LWLockAcquire([i].l.lock, LW_EXCLUSIVE);
>
> That's a lot of exclusive locks and that would seem to have performance
> implications.  It seems to me this is going to be a hard one to
> benchmark because the regression (if any) would only be seen under heavy
> load on a very large system.
>
> In general I agree with the other comments that this could end up being
> a problem.  On the other hand, since the additional locks are only taken
> at checkpoint or archive_timeout it may not be that big a deal.

Yes, I did some tests on my laptop a couple of months back, that has 4
cores. After reducing NUM_XLOGINSERT_LOCKS from 8 to 4 to increase
contention and performing a bunch of INSERT using 4 clients on 4
different relations I could not catch a difference.. Autovacuum was
disabled to eliminate any noise. I tried checkpoint_segments at 30s to
see its effects, as well as larger values to see the impact with the
standby snapshot taken by the bgwriter. Other thoughts are welcome.

> +++ b/src/backend/access/transam/xloginsert.c * Should this record
> include the replication origin if one is set up?
>
> Outdated comment from XLogIncludeOrigin().

Fixed. I added as well some comments on top of XLogSetFlags to mention
what are the flags that can be used. I didn't think that it was
necessary to add an assertion here. Also, I noticed that the comment
on top of XLogInsertRecord mentioned those flags but was incorrect.

> * [PATCH 2/3] hs-checkpoints-v12-2
>
> +++ b/src/backend/postmaster/checkpointer.c
> +   /* OK, it's time to switch */
> +   elog(LOG, "Request XLog Switch");
>
> LOG level seems a bit much here, perhaps DEBUG1?

That's from Horiguchi-san's patch, and those would be definitely
better as DEBUG1 by looking at it. Now and in order to keep things
simple I think that we had better discard this patch for now. I was
planning to come back to this thing anyway once we are done with the
first problem.

> * [PATCH 3/3] hs-checkpoints-v12-3
>
> +* switch segment only when any substantial progress have 
> made from
> +* reasons will cause last_xlog_switch_lsn stay behind but it 
> doesn't
>
> How about, "Switch segment only when substantial progress has been made
> after the last segment was switched by a timeout.  Segment switching for
> other reasons..."
>
> +++ b/src/backend/access/transam/xlog.c
> +   elog(LOG, "Not a forced or shutdown checkpoint: progress_lsn 
> %X/%X,
> ckpt %X/%X",
> +   elog(LOG, "Checkpoint is skipped");
> +   elog(LOG, "snapshot taken by checkpoint %X/%X",
>
> Same for the above, seems like it would just be noise for most users.
>
> +++ b/src/backend/postmaster/bgwriter.c
> +   elog(LOG, "snapshot taken by bgwriter %X/%X",
>
> Ditto.

The original patch was developed to ease debugging, and I chose LOG to
not be polluted with a bunch of DEBUG1 entries :)

Now we can do something, as follows:
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8450,6 +8450,8 @@ CreateCheckPoint(int flags)
{
if (progress_lsn == ControlFile->checkPoint)
{
+   if (log_checkpoints)
+   ereport(LOG, "checkpoint skipped");
WALInsertLockRelease();
LWLockRelease(CheckpointLock);
END_CRIT_SECTION();
Letting users know that the checkpoint has been skipped sounds like a
good idea. Perhaps that's better if squashed with the first patch.

> I don't see any unintended consequences in this patch but it doesn't
> mean there aren't any.  I'm definitely concerned by the exclusive locks
> but it may turn out they do not actually represent a bottleneck.

That's a hard to see a difference. Perhaps I didn't try hard enough..

Well for now attached are two patches, that could just be squashed into one.
-- 
Michael
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f95fdb8..e87caa6 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8450,6 +8450,8 @@ CreateCheckPoint(int flags)
 	{
 		if (progress_lsn == ControlFile->checkPoint)
 		{
+			if (log_checkpoints)
+ereport(LOG, "checkpoint skipped");
 			WALInsertLockRelease();
 			LWLockRelease(CheckpointLock);
 			END_CRIT_SECTION();
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index b019bc1..ac40731 100644
--- a/src/backend/access/heap/heapam.c
+++ 

Re: [HACKERS] Set log_line_prefix and application name in test drivers

[Peter Eisentraut]

On 9/28/16 6:13 PM, Robert Haas wrote:
> Christoph/Debian:
> log_line_prefix = '%t [%p-%l] %q%u@%d '
> Peter:
> log_line_prefix = '%t [%p]: [%l] %qapp=%a '

I'm aware of two existing guidelines on log line formats: syslog and
pgbadger.  Syslog output looks like this:

Sep 28 00:58:56 hostname syslogd[46]: some text here

pgbadger by default asks for this:

log_line_prefix = '%t [%p]: [%l-1] user=%u,db=%d,app=%a,client=%h '

I don't know why it wants that "-1" there, and I'm actually not sure
what the point of %l is in practice.  Those are separate issues that are
having their own lively discussions at times.  I could drop the [%l]
from my proposal if that causes concerns.

On balance, I think my proposal is more in line with existing
wide-spread conventions.

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


-- 
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] Add support for restrictive RLS policies

[Craig Ringer]

On 27 September 2016 at 15:15, Jeevan Chalke
 wrote:
> Hello Stephen,
>
> On Tue, Sep 27, 2016 at 12:57 AM, Stephen Frost  wrote:
>>
>> Jeevan,
>>
>> * Jeevan Chalke (jeevan.cha...@enterprisedb.com) wrote:
>> > I have started reviewing this patch and here are couple of points I have
>> > observed so far:
>> >
>> > 1. Patch applies cleanly
>> > 2. make / make install / initdb all good.
>> > 3. make check (regression) FAILED. (Attached diff file for reference).
>>
>> I've re-based my patch on top of current head and still don't see the
>> failures which you are getting during the regression tests.  Is it
>> possible you were doing the tests without a full rebuild of the source
>> tree..?
>>
>> Can you provide details of your build/test environment and the full
>> regression before and after output?
>
>
> I still get same failures with latest sources and with new patch. Here are
> few details of my setup. Let me know if I missed any.
>
> $ uname -a
> Linux centos7 3.10.0-327.28.3.el7.x86_64 #1 SMP Thu Aug 18 19:05:49 UTC 2016
> x86_64 x86_64 x86_64 GNU/Linux
>
> HEAD at
> commit 51c3e9fade76c12e4aa37bffdf800bbf74fb3fb1
>
> configure switches:
> ./configure --with-openssl --with-tcl --with-perl --with-python
> --with-ossp-uuid --with-ldap --with-pam --with-zlib --with-pgport=5432
> --enable-depend --enable-debug --enable-cassert --prefix=`pwd`/install
> CFLAGS="-g -O0"

I suggest:

git reset --hard
git clean -fdx
ccache -C

then re-apply patch and re-check.

I've had a couple of issues recently caused by ccache doing something
funky :( but haven't been able to track it down yet.


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


-- 
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] assert violation in logical messages serialization

[Robert Haas]

On Tue, Sep 27, 2016 at 10:45 AM, Stas Kelvich  wrote:
> During processing of logical messages in ReorderBufferSerializeChange()
> pointer to ondisk structure isn’t updated after possible reorder buffer 
> realloc by
> ReorderBufferSerializeReserve().
>
> Actual reason spotted by Konstantin Knizhnik.

Thanks for the patch, and congratulations to Konstantin on the spot.
Committed and back-patched to 96.

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


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

Re: [HACKERS] Remove superuser() checks from pgstattuple

[Stephen Frost]

Michael,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Tue, Sep 27, 2016 at 4:43 AM, Stephen Frost  wrote:
> > * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> >> This is now being obsoleted by the later idea of allowing base installs
> >> from a chain of upgrade scripts.  But if your upgrade scripts contain
> >> ALTER TABLE commands, you will probably still want to write base install
> >> scripts that do a fresh CREATE TABLE instead.
> >
> > I've updated the patch to remove the new base version script and to rely
> > on the changes made by Tom to install the 1.4 version and then upgrade
> > to 1.5.
> >
> > Based on my testing, it appears to all work correctly.
> 
> Same conclusion from here.

Fantastic, thanks for the testing!

> > Updated (much smaller) patch attached.
> 
> I had a look at it, and it is doing the work it claims to do. So I am
> marking it as "Ready for Committer".

Great.  I'm going to go over the whole patch closely again and will push
it soon.

Thanks again!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Speed up Clog Access by increasing CLOG buffers

[Robert Haas]

On Tue, Sep 27, 2016 at 5:15 PM, Tomas Vondra
 wrote:
> So, I got the results from 3.10.101 (only the pgbench data), and it looks
> like this:
>
>  3.10.101   1  8 16 32 64128192
> 
>  granular-locking2582  18492  33416  49583  53759  53572  51295
>  no-content-lock 2580  18666  33860  49976  54382  54012  51549
>  group-update2635  18877  33806  49525  54787  54117  51718
>  master  2630  18783  33630  49451  54104  53199  50497
>
> So 3.10.101 performs even better tnan 3.2.80 (and much better than 4.5.5),
> and there's no sign any of the patches making a difference.

I'm sure that you mentioned this upthread somewhere, but I can't
immediately find it.  What scale factor are you testing here?

It strikes me that the larger the scale factor, the more
CLogControlLock contention we expect to have.  We'll pretty much do
one CLOG access per update, and the more rows there are, the more
chance there is that the next update hits an "old" row that hasn't
been updated in a long time.  So a larger scale factor also increases
the number of active CLOG pages and, presumably therefore, the amount
of CLOG paging activity.

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


-- 
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] should xlog_outdesc modify its argument?

[Heikki Linnakangas]

On 09/28/2016 02:35 AM, Mark Dilger wrote:

The function

  static void xlog_outdesc(StringInfo buf, XLogReaderState *record);

in src/backend/access/transam/xlog.c is called by XLogInsertRecord,
and after returning a string describing an XLogRecord, it clears the
state data in its XLogReaderState argument.  That mixes the read-only
semantics of "give me a string that describes this argument" and the
read-write semantics of "clear out the value in this argument".


I don't see where the "clears the state data" is happening. Can you 
elaborate?


- Heikki



--
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] Tracking wait event for latches

[Michael Paquier]

On Wed, Sep 28, 2016 at 3:40 PM, Thomas Munro
 wrote:
> On Wed, Sep 28, 2016 at 6:25 PM, Michael Paquier
>  wrote:
>> wait-event-set-v8.patch
>
> Ok, I'm just about ready to mark this as 'Ready for Committer'.

Thanks.

> Just a couple of things:
> + pgstat_report_wait_start((uint8) classId, (uint16) eventId);
> Unnecessary casts.

Right

> +
> + Client
> + SecureRead
> + Waiting to read data from a secure connection.
> +
> +
> + SecureWrite
> + Waiting to write data to a secure connection.
> +
>
> I think we want to drop the word 'secure' from the description lines
> in this patch.  Then I think we plan to make a separate patch that
> will rename the functions themselves and the corresponding wait points
> to something more generic?

Robert mentioned ClientRead/ClientWrite upthread. I still think that
SecureRead/SecureWrite is better as it respects the routine name where
the wait point is, and that's consistent with the rest.

> I'm assuming that my suggestions for making WE_WAL_SENDER_WAIT_WAL and
> WE_WAL_SENDER_MAIN have a dynamically chosen class ID would also be
> material for another patch, but it doesn't matter much because those
> processes won't show up yet anyway.

WAL senders do show up since 8299471 because they are counted as in
max_connections. That's pretty cool combined with this patch.

I am sending a new patch to save 30s to the committer potentially
looking at this patch.
-- 
Michael
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 8ca1c1c..9222b73 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -17,6 +17,7 @@
 #include "access/xact.h"
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
+#include "pgstat.h"
 #include "storage/latch.h"
 #include "utils/hsearch.h"
 #include "utils/memutils.h"
@@ -496,7 +497,9 @@ pgfdw_get_result(PGconn *conn, const char *query)
 			wc = WaitLatchOrSocket(MyLatch,
    WL_LATCH_SET | WL_SOCKET_READABLE,
    PQsocket(conn),
-   -1L);
+   -1L,
+   WAIT_EXTENSION,
+   WE_EXTENSION);
 			ResetLatch(MyLatch);
 
 			CHECK_FOR_INTERRUPTS();
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index f400785..bb975c1 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -679,6 +679,42 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   11:34   0:00 postgres: ser
   buffer in question.
  
 
+
+ 
+  Activity: The server process is idle.  This is used by
+  system processes waiting for activity in their main processing loop.
+  wait_event will identify the specific wait point.
+ 
+
+
+ 
+  Extension: The server process is waiting for activity
+  in an extension module.  This category is useful for modules to
+  track custom waiting points.
+ 
+
+
+ 
+  Client: The server process is waiting for some activity
+  on a socket from user applications, and that the server expects
+  something to happen that is independent from its internal processes.
+  wait_event will identify the specific wait point.
+ 
+
+
+ 
+  IPC: The server process is waiting for some activity
+  from another process in the server.  wait_event will
+  identify the specific wait point.
+ 
+
+
+ 
+  Timeout: The server process is waiting for a timeout
+  to expire.  wait_event will identify the specific wait
+  point.
+ 
+

   
  
@@ -1085,6 +1121,143 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   11:34   0:00 postgres: ser
  BufferPin
  Waiting to acquire a pin on a buffer.
 
+
+ Activity
+ ArchiverMain
+ Waiting in main loop of the archiver process.
+
+
+ AutoVacuumMain
+ Waiting in main loop of autovacuum launcher process.
+
+
+ BgWriterHibernate
+ Waiting in background writer process, hibernating.
+
+
+ BgWriterMain
+ Waiting in main loop of background writer process background worker.
+
+
+ CheckpointerMain
+ Waiting in main loop of checkpointer process.
+
+
+ PgStatMain
+ Waiting in main loop of the statistics collector process.
+
+
+ RecoveryWalAll
+ Waiting for WAL from any kind of source (local, archive or stream) at recovery.
+
+
+ RecoveryWalStream
+ Waiting for WAL from a stream at recovery.
+
+
+ SysLoggerMain
+ Waiting in main loop of syslogger 

Re: [HACKERS] Supporting huge pages on Windows

[Tsunakawa, Takayuki]

From: Thomas Munro [mailto:thomas.mu...@enterprisedb.com]
> >  huge_pages=off: 70412 tps
> >  huge_pages=on : 72100 tps
> 
> Hmm.  I guess it could be noise or random code rearrangement effects.

I'm not the difference was a random noise, because running multiple set of 
three runs of pgbench (huge_pages = on, off, on, off, on...) produced similar 
results.  But I expected a bit greater improvement, say, +10%.  There may be 
better benchmark model where the large page stands out, but I think pgbench is 
not so bad because its random data access would cause TLB cache misses.



> I saw your recent post[2] proposing to remove the sentence about the 512MB
> effective limit and I wondered why you didn't go to larger sizes with a
> larger database and more run time.  But I will let others with more
> benchmarking experience comment on the best approach to investigate Windows
> shared_buffers performance.

Yes, I could have gone to 8GB of shared_buffers because my PC has 16GB of RAM, 
but I felt the number of variations was sufficient.  Anyway, positive comments 
on benchmarking would be appreciated.

Regards
Takayuki Tsunakawa



-- 
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] LLVM Address Sanitizer (ASAN) and valgrind support

[Piotr Stefaniak]

On 2016-09-28 00:02, Andres Freund wrote:
> On 2015-09-07 17:05:10 +0100, Greg Stark wrote:
>> I feel like I remember hearing about this before but I can't find any
>> mention of it in my mail archives. It seems pretty simple to add
>> support for LLVM's Address Sanitizer (asan) by using the hooks we
>> already have for valgrind.
>
> Any plans to pick this up again?

Not remembering the context, I was initially confused about what exactly 
supposedly needs to be done in order to have ASan support, especially 
since I've been using it for a couple of years without any kind of 
modifications. Having read the whole thread now, I assume the discussion 
is now about getting MSan support, since apparently it's been already 
concluded that not much is needed for getting ASan support:

>> I don't even see any need offhand for a configure flag or autoconf
>> test. We could have a configure flag just to be consistent with
>> valgrind but it seems pointless. If you're compiling with asan I don't
>> see any reason to not use it. I'm building this to see if it works
>> now.
>
> I agree.  A flag guards Valgrind client requests, because we'd otherwise have
> no idea whether the user plans to run the binary under Valgrind.  For ASAN,
> all relevant decisions happen at build time.

Please correct me if I'm wrong.



-- 
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] Transactions involving multiple postgres foreign servers

[Ashutosh Bapat]

On Wed, Sep 28, 2016 at 10:43 AM, Masahiko Sawada  wrote:
> On Tue, Sep 27, 2016 at 9:06 PM, Ashutosh Bapat
>  wrote:
>> On Tue, Sep 27, 2016 at 2:54 PM, Masahiko Sawada  
>> wrote:
>>> On Mon, Sep 26, 2016 at 9:07 PM, Ashutosh Bapat
>>>  wrote:
 On Mon, Sep 26, 2016 at 5:25 PM, Masahiko Sawada  
 wrote:
> On Mon, Sep 26, 2016 at 7:28 PM, Ashutosh Bapat
>  wrote:
>> My original patch added code to manage the files for 2 phase
>> transactions opened by the local server on the remote servers. This
>> code was mostly inspired from the code in twophase.c which manages the
>> file for prepared transactions. The logic to manage 2PC files has
>> changed since [1] and has been optimized. One of the things I wanted
>> to do is see, if those optimizations are applicable here as well. Have
>> you considered that?
>>
>>
>
> Yeah, we're considering it.
> After these changes are committed, we will post the patch incorporated
> these changes.
>
> But what we need to do first is the discussion in order to get consensus.
> Since current design of this patch is to transparently execute DCL of
> 2PC on foreign server, this code changes lot of code and is
> complicated.

 Can you please elaborate. I am not able to understand what DCL is
 involved here. According to [1], examples of DCL are GRANT and REVOKE
 command.
>>>
>>> I meant transaction management command such as PREPARE TRANSACTION and
>>> COMMIT/ABORT PREPARED command.
>>> The web page I refered might be wrong, sorry.
>>>
> Another approach I have is to push down DCL to only foreign servers
> that support 2PC protocol, which is similar to DML push down.
> This approach would be more simpler than current idea and is easy to
> use by distributed transaction manager.

 Again, can you please elaborate, how that would be different from the
 current approach and how does it simplify the code.

>>>
>>> The idea is just to push down PREPARE TRANSACTION, COMMIT/ROLLBACK
>>> PREPARED to foreign servers that support 2PC.
>>> With this idea, the client need to do following operation when foreign
>>> server is involved with transaction.
>>>
>>> BEGIN;
>>> UPDATE parent_table SET ...; -- update including foreign server
>>> PREPARE TRANSACTION 'xact_id';
>>> COMMIT PREPARED 'xact_id';
>>>
>>> The above PREPARE TRANSACTION and COMMIT PREPARED command are pushed
>>> down to foreign server.
>>> That is, the client needs to execute PREPARE TRANSACTION and
>>>
>>> In this idea, I think that we don't need to do followings,
>>>
>>> * Providing the prepare id of 2PC.
>>>   Current patch adds new API prepare_id_provider() but we can use the
>>> prepare id of 2PC that is used on parent server.
>>>
>>> * Keeping track of status of foreign servers.
>>>   Current patch keeps track of status of foreign servers involved with
>>> transaction but this idea is just to push down transaction management
>>> command to foreign server.
>>>   So I think that we no longer need to do that.
>>
>>> COMMIT/ROLLBACK PREPARED explicitly.
>>
>> The problem with this approach is same as one previously stated. If
>> the connection between local and foreign server is lost between
>> PREPARE and COMMIT the prepared transaction on the foreign server
>> remains dangling, none other than the local server knows what to do
>> with it and the local server has lost track of the prepared
>> transaction on the foreign server. So, just pushing down those
>> commands doesn't work.
>
> Yeah, my idea is one of the first step.
> Mechanism that resolves the dangling foreign transaction and the
> resolver worker process are necessary.
>
>>>
>>> * Adding max_prepared_foreign_transactions parameter.
>>>   It means that the number of transaction involving foreign server is
>>> the same as max_prepared_transactions.
>>>
>>
>> That isn't true exactly. max_prepared_foreign_transactions indicates
>> how many transactions can be prepared on the foreign server, which in
>> the method you propose should have a cap of max_prepared_transactions
>> * number of foreign servers.
>
> Oh, I understood, thanks.
>
> Consider sharding solution using postgres_fdw (that is, the parent
> postgres server has multiple shard postgres servers), we need to
> increase max_prepared_foreign_transactions whenever new shard server
> is added to cluster, or to allocate enough size in advance. But the
> estimation of enough max_prepared_foreign_transactions would not be
> easy, for example can we estimate it by (max throughput of the system)
> * (the number of foreign servers)?
>
> One new idea I came up with is that we set transaction id on parent
> server to global transaction id (gid) that is prepared on shard
> server.
> And pg_fdw_resolver worker process 

Re: [HACKERS] Tracking wait event for latches

[Thomas Munro]

On Wed, Sep 28, 2016 at 6:25 PM, Michael Paquier
 wrote:
> wait-event-set-v8.patch

Ok, I'm just about ready to mark this as 'Ready for Committer'.  Just
a couple of things:

+ pgstat_report_wait_start((uint8) classId, (uint16) eventId);

Unnecessary casts.

+
+ Client
+ SecureRead
+ Waiting to read data from a secure connection.
+
+
+ SecureWrite
+ Waiting to write data to a secure connection.
+

I think we want to drop the word 'secure' from the description lines
in this patch.  Then I think we plan to make a separate patch that
will rename the functions themselves and the corresponding wait points
to something more generic?

I'm assuming that my suggestions for making WE_WAL_SENDER_WAIT_WAL and
WE_WAL_SENDER_MAIN have a dynamically chosen class ID would also be
material for another patch, but it doesn't matter much because those
processes won't show up yet anyway.

-- 
Thomas Munro
http://www.enterprisedb.com


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

Re: [HACKERS] PATCH: Exclude additional directories in pg_basebackup

[Michael Paquier]

On Tue, Sep 27, 2016 at 11:27 PM, David Steele  wrote:
> On 9/26/16 2:36 AM, Michael Paquier wrote:
>
>> Just a nit:
>>  
>> - postmaster.pid
>> + postmaster.pid and postmaster.opts
>>  
>> Having one  block for each file would be better.
>
> OK, changed.

You did not actually change it :)

>> +const char *excludeFile[] =
>> excludeFiles[]?
>>
>> +# Move pg_replslot out of $pgdata and create a symlink to it
>> +rename("$pgdata/pg_replslot", "$tempdir/pg_replslot")
>> +   or die "unable to move $pgdata/pg_replslot";
>> +symlink("$tempdir/pg_replslot", "$pgdata/pg_replslot");
>> This will blow up on Windows. Those tests need to be included in the
>> SKIP block. Even if your code needs to support junction points on
>> Windows, as perl does not offer an equivalent for it we just cannot
>> test it on this platform.
>
> Fixed.

Thanks for the updated version.

+
+ backup_label and tablespace_map.  If these
+ files exist they belong to an exclusive backup and are not applicable
+ to the base backup.
+
This is a bit confusing. When using pg_basebackup the files are
ignored, right, but they are included in the tar stream so they are
not excluded at the end. So we had better remove purely this
paragraph. Similarly, postgresql.auto.conf.tmp is something that
exists only for a short time frame. Not listing it directly is fine
IMO.

+   /* If symlink, write it as a directory anyway */
+#ifndef WIN32
+   if (S_ISLNK(statbuf->st_mode))
+#else
+   if (pgwin32_is_junction(pathbuf))
+#endif
+
+   statbuf->st_mode = S_IFDIR | S_IRWXU;
Indentation here is confusing. Coverity would complain.

+   /* Stat the file */
+   snprintf(pathbuf, MAXPGPATH, "%s/%s", path, de->d_name);
There is no need to put the stat() call this early in the loop as this
is just used for re-creating empty directories.

+if (strcmp(pathbuf + basepathlen + 1,
+   excludeFiles[excludeIdx]) == 0)
There is no need for complicated maths, you can just use de->d_name.

pg_xlog has somewhat a similar treatment, but per the exception
handling of archive_status it is better to leave its check as-is.

The DEBUG1 entries are really useful for debugging, it would be nice
to keep them in the final patch.

Thinking harder, your logic can be simplified. You could just do the following:
- Check for interrupts first
- Look at the list of excluded files
- Call lstat()
- Check for excluded directories

After all that fixed, I have moved the patch to "Ready for Committer".
Please use the updated patch though.
-- 
Michael
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 0f09d82..a8daa07 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -1090,6 +1090,22 @@ SELECT pg_stop_backup();

 

+The contents of the pg_dynshmem/, pg_stat_tmp/,
+pg_notify/, pg_serial/,
+pg_snapshots/, and pg_subtrans/ directories can
+be omitted from the backup as they will be initialized on postmaster
+startup. If the  is set and is
+under the database cluster directory then the contents of the directory
+specified by  can also be omitted.
+   
+
+   
+Any file or directory beginning with pgsql_tmp can be
+omitted from the backup.  These files are removed on postmaster start and
+the directories will be recreated as needed.
+   
+
+   
 The backup label
 file includes the label string you gave to pg_start_backup,
 as well as the time at which pg_start_backup was run, and
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 68b0941..fe4d511 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -2069,7 +2069,9 @@ The commands accepted in walsender mode are:


 
- various temporary files created during the operation of the PostgreSQL server
+ Various temporary files and directories created during the operation of
+ the PostgreSQL server, i.e. any file or directory beginning with
+ pgsql_tmp.
 


@@ -2082,7 +2084,11 @@ The commands accepted in walsender mode are:


 
- pg_replslot is copied as an empty directory.
+ pg_replslot, pg_dynshmem,
+ pg_stat_tmp, pg_notify,
+ pg_serial, pg_snapshots, and
+ pg_subtrans are copied as empty directories (even if they
+ are symbolic links).
 


diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 9f1eae1..984ea5b 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -610,10 +610,8 @@ PostgreSQL documentation
   
The backup will include all files in the data directory and tablespaces,
including the configuration files and any additional files placed in the
-   directory by third parties. But only regular files and directories are
-   

[HACKERS] psql casts aspersions on server reliability

[Robert Haas]

psql tends to do things like this:

rhaas=# select * from pg_stat_activity;
FATAL:  terminating connection due to administrator command
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.

Basically everything psql has to say about this is a lie:

1. It says the server closed the connection unexpectedly, but it just
finished processing a FATAL message from the server.  So how
unexpected is it that the connection would thereafter be closed?

2.  It says the server probably terminated abnormally, but PostgreSQL
is rarely so unreliable as to just give up the ghost and die.  The
ErrorResponse it just processed has an errcode of
ERRCODE_ADMIN_SHUTDOWN, so probably what happened is somebody either
shut down the server or killed the session.

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


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

Re: [HACKERS] pg_dump / copy bugs with "big lines" ?

[Daniel Verite]

Tomas Vondra wrote:

> 4) HandleParallelMessage needs a tweak, as it uses msg->len in a log 
> message, but with '%d' and not '%ld' (as needed after changing the type 
> to Size).

This could be %zu for the Size type. It's supported by elog().

However, it occurs to me that if we don't claim the 2GB..4GB range for
the CopyData message, because its Int32 length is not explicitly
unsigned as mentioned upthread, then it should follow that we don't
need to change StringInfo.{len,maxlen} from int type to Size type.

We should just set the upper limit for StringInfo.maxlen to
(0x7fff-1) instead of MaxAllocHugeSize for stringinfos with the
allow_long flag set, and leave the len and maxlen fields to their
original, int type.

Does that make sense?


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Password identifiers, protocol aging and SCRAM protocol

[Michael Paquier]

On Wed, Sep 28, 2016 at 7:03 PM, Heikki Linnakangas  wrote:
> On 09/28/2016 12:53 PM, Heikki Linnakangas wrote:
>>
>> On 09/26/2016 09:02 AM, Michael Paquier wrote:

 * [PATCH 2/8] Move encoding routines to src/common/
>
>
> I wonder if it is confusing to have two of encode.h/encode.c.  Perhaps
> they should be renamed to make them distinct?
>>>
>>> Yes it may be a good idea to rename that, like encode_utils.[c|h] for
>>> the new files.
>>
>>
>> Looking at these encoding functions, the SCRAM protocol actually uses
>> base64 for everything.

OK, I thought that moving everything made more sense for consistency
but let's keep src/common/ as small as possible.

> Oh, one more thing. The SCRAM spec says:
>
>> The use of base64 in SCRAM is restricted to the canonical form with
>> no whitespace.
>
> Our b64_encode routine does use whitespace, so we can't use it as is for
> SCRAM. As the patch stands, we might never output anything long enough to
> create linefeeds, but let's be tidy. The base64 implementation is about 100
> lines of code, so perhaps we should just leave src/backend/utils/encode.c
> alone, and make a new copy of the base64 routines in src/common.

OK, I'll refresh that tomorrow with the rest. Thanks for the commit to
extend password_encryption.
-- 
Michael


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