Re: [HACKERS] Rationalizing code-sharing among src/bin/ directories

[Michael Paquier]

On Fri, Mar 25, 2016 at 12:55 PM, Tom Lane  wrote:
> Michael Paquier  writes:
>> Not sure if Andres is working on that for now or not, the main
>> discussion that I am foreseeing here is how we are going to map elevel
>> for the frontend (should FATAL, PANIC exit immediately, etc).
>
> Doesn't seem that complicated to me: elevel >= ERROR results in exit(1),
> otherwise just print to stderr and return.  We'd be eyeballing each case
> that we remove "#ifndef FRONTEND" from anyway; if it's expecting behavior
> noticeably more complicated than that, we could leave it as-is.

Something that I see as mandatory as well is a way to bypass some of
the elevels depending on the way a frontend tool is called, so we'd
need something like that in the common elog facility:

void
elog(blah)
{
#ifdef FRONTEND
 if (!callback_routine_defined_in_frontend(elevel))
 return;
#endif

 blah_blah_move_on.
}

This would be useful to avoid code patterns of this type in a frontend
tool where for example a debug flag is linked with elevel. For example
this pattern:
if (debug)
elog(DEBUG1, "Debug blah");
could be reduced to as the callback routine would allow bypassing
elog() directly:
elog(DEBUG1, "Debug blah");

That's just food for thought at this stage, I get back to reviewing...
-- 
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] Odd system-column handling in postgres_fdw join pushdown patch

[Ashutosh Bapat]

A much simpler solution, that will work with postgres_fdw, might be to just
deparse these columns with whatever random values (except for tableoid)
they are expected to have in those places. Often these values can simply be
NULL or 0. For tableoid deparse it to 'oid value'::oid. Thus for a user
query

select t1.taleoid, t2.xmax, t1.c1, t2.c2 from t1 join t2 on (...) ... --
where t1 and t2 are foreign tables with same names on the foreign server.

the query sent to the foreign server would look like

select '15623'::oid, NULL, t1.c1, t2.c2 from t1 join t2 on (...) ... --
where '15623' is oid of t1 on local server.

This does spend more bandwidth than necessary and affect performance, here
is why the approach might be better,
1. It's not very common to request these system columns in a "join" query
involving foreign tables. Usually they will have user columns or ctid
(DMLs) but very rarely other system columns.

2. This allows expressions involving these system columns to be pushed
down, whenever we will start pushing them down in the targetlist.

3. The changes to the code are rather small. deparseColumnRef() will need
to produce the strings above instead of actual column names.

4. The approach will work with slight change, if and when, we need the
actual system column values from the foreign server. That time the above
function needs to deparse the column names instead of constant values.

Having to hardcode tableoid at the time of planning should be fine since
change in tableoid between planning and execution will trigger plan cache
invalidation. I haven't tried this though.

Sorry for bringing this solution late to the table.

On Thu, Mar 24, 2016 at 3:04 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

>
>
> On Thu, Mar 24, 2016 at 9:31 AM, Etsuro Fujita <
> fujita.ets...@lab.ntt.co.jp> wrote:
>
>> On 2016/03/23 13:44, Ashutosh Bapat wrote:
>>
>>> An FDW can choose not to use those functions, so I don't see a
>>> connection between scan list having simple Vars and existence of those
>>> functions (actually a single one). But having those function would
>>> minimize the code that each FDW has to write, in case they want those
>>> functions. E.g. we have to translate Var::varno to tableoid in case
>>> that's requested by pulling RTE and then getting oid out from there. If
>>> that functionality is available in the core, 1. the code is not
>>> duplicated 2. every FDW will get the same tableoid. Similarly for the
>>> other columns.
>>>
>>
>> OK.  Then, I'd like to propose a function that would create interger
>> Lists of indexes of tableoids, xids and cids plus
>
>
> Ok,
>
>
>> an OID List of these tableoids,
>
>
> I didn't get this.
>
>
>> in a given fdw_scan_tlist, on the assumption that each expression in the
>> fdw_scan_tlist is a simple Var.
>
>
> I guess this is Ok. In fact, at least for now an expression involving any
> of those columns is not pushable to the foreign server, as the expression
> can not be evaluated there. So, if we come across such a case in further
> pushdowns, we will need to have a different solution for pushing down such
> target lists.
>
>
>> I'd also like to propose another function that would fill system columns
>> using these Lists when creating a scan tuple.
>>
>> Ok.
>
> I had imagined that the code to extract the above lists and filling the
> values in scan tuple will be in FDW. We only provide a function to supply
> those values. But what you propose might actually be much practical.
>
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company
>



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: [HACKERS] Performance degradation in commit 6150a1b0

[Amit Kapila]

On Wed, Mar 23, 2016 at 1:59 PM, Ashutosh Sharma 
wrote:
>
> Hi All,
>
> I have been working on this issue for last few days trying to investigate
what could be the probable reasons for Performance degradation at commit
6150a1b0. After going through Andres patch for moving buffer I/O and
content lock out of Main Tranche, the following two things come into my
> mind.
>
> 1. Content Lock is no more used as a pointer in BufferDesc structure
instead it is included as LWLock structure. This  basically increases the
overall structure size from 64bytes to 80 bytes. Just to investigate on
this, I have reverted the changes related to content lock from commit
6150a1b0 and taken at least 10 readings and with this change i can see that
the overall performance is similar to what it was observed earlier i.e.
before commit 6150a1b0.
>
> 2. Secondly, i can see that the BufferDesc structure padding is 64 bytes
however the PG CACHE LINE ALIGNMENT is 128 bytes. Also, after changing the
BufferDesc structure padding size to 128 bytes along with the changes
mentioned in above point #1, I see that the overall performance is again
similar to what is observed before commit 6150a1b0.
>
> Please have a look into the attached test report that contains the
performance test results for all the scenarios discussed above and let me
know your thoughts.
>

So this indicates that changing back content lock as LWLock* in BufferDesc
brings back the performance which indicates that increase in BufferDesc
size to more than 64bytes on this platform has caused regression.  I think
it is worth trying the patch [1] as suggested by Andres as that will reduce
the size of BufferDesc which can bring back the performance.  Can you once
try the same?

[1] -
http://www.postgresql.org/message-id/capphfdsrot1jmsnrnccqpnzeu9vut7tx6b-n1wyouwwfhd6...@mail.gmail.com

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

Re: [HACKERS] Rationalizing code-sharing among src/bin/ directories

[Tom Lane]

Michael Paquier  writes:
> Not sure if Andres is working on that for now or not, the main
> discussion that I am foreseeing here is how we are going to map elevel
> for the frontend (should FATAL, PANIC exit immediately, etc).

Doesn't seem that complicated to me: elevel >= ERROR results in exit(1),
otherwise just print to stderr and return.  We'd be eyeballing each case
that we remove "#ifndef FRONTEND" from anyway; if it's expecting behavior
noticeably more complicated than that, we could leave it as-is.

> Stability of 9.6 is first and just ahead.

Agreed, this is code cleanup not a high priority.

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] Combining Aggregates

[David Rowley]

On 25 March 2016 at 06:17, Robert Haas  wrote:
> On Mon, Mar 21, 2016 at 2:18 PM, David Rowley
>  wrote:
>> I've attached 2 of the patches which are affected by the changes.
>
> I think the documentation for 0001 needs some work yet.  The
> additional paragraph that you've added...
>
> (1) doesn't seem to appear at a very logical place in the
> documentation - I think it should be much further down, as it's a
> minor detail.  Maybe document this the same way as the documentation
> patch you just sent for the combine-function stuff does it; and

Thanks. I also realised this when writing the combine documents fix.

> (2) isn't indented consistently with the surrounding paragraphs; and
>
> (3) is missing a closing  tag
>
> Also, I'd just cut this:
>
> +  This is required due to
> +  the process model being unable to pass references to INTERNAL
> +   types between different PostgreSQL
> +  processes.
>
> Instead, I'd change the earlier sentence in the paragraph, which
> currently reads:
>
> +  These
> +  functions are required in order to allow parallel aggregation for 
> aggregates
> +  with an stype of 
> +  INTERNAL.
>
> I'd replace the period at end with a comma and add "since
> INTERNAL values represent arbitrary in-memory data
> structures which can't be passed between processes".  I think that's a
> bit smoother.
>

In my rewrite I've incorporated these words.

Thanks for checking over this. A patch will follow in my response to
the next email.

-- 
 David Rowley   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] Rationalizing code-sharing among src/bin/ directories

[Michael Paquier]

On Fri, Mar 25, 2016 at 9:52 AM, Tom Lane  wrote:
> Michael Paquier  writes:
>> Preventing the use of elog in the frontend is something that has been
>> addressed multiple times with FRONTEND, so that's not likely going to
>> be an issue I think. Andres has mentioned as well having some elog
>> stuff available in frontend..
>
> I'm on board with doing something about that one, though.  There are
> quite a lot of places that could be cleaned up.

Not sure if Andres is working on that for now or not, the main
discussion that I am foreseeing here is how we are going to map elevel
for the frontend (should FATAL, PANIC exit immediately, etc). I think
as well that some sort of callback system would be needed to allow
frontend clients to perform filtering of the log entries. Say
pg_rewind has a --debug mode, so this would map with DEBUG1 entries,
we'd need a way to control if those are issued or not depending on the
frontend call arguments...

Surely that's a cleanup patch 9.7 though, so there are a couple of
months ahead of us... And that's not the highest priority now.
Stability of 9.6 is first and just ahead.
-- 
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] Alter or rename enum value

[Tom Lane]

Jim Nasby  writes:
> I'm certain there's a really good reason adding new values isn't allowed 
> inside of a transaction. It's probably documented in the code.

Yes, see AlterEnum():

 * Ordinarily we disallow adding values within transaction blocks, because
 * we can't cope with enum OID values getting into indexes and then having
 * their defining pg_enum entries go away.  However, it's okay if the enum
 * type was created in the current transaction, since then there can be no
 * such indexes that wouldn't themselves go away on rollback.  (We support
 * this case because pg_dump --binary-upgrade needs it.)

Deleting an enum value is similarly problematic.  Let's assume you're
willing to take out sufficiently widespread locks to prevent entry of
any new rows containing the doomed enum value (which, in reality, is
pretty much unworkable in production situations).  Let's assume that
you're willing to hold those locks long enough to VACUUM away every
existing dead row containing that value (see previous parenthetical
comment, squared).  You're still screwed, because there might be
instances of the to-be-deleted value sitting in upper levels of btree
indexes (or other index types).  There is no mechanism for getting
rid of those, short of a full index rebuild; and you cannot remove
the pg_enum entry without breaking such indexes.

It's conceivable that we could do something like adding an "isdead"
column to pg_enum and making enum_in reject new values that're marked
isdead.  But I can't see that we'd ever be able to support true
removal of an enum value at reasonable cost.  And I'm not really sure
where the use-case argument is for working hard on it.

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] Show dropped users' backends in pg_stat_activity

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Robert Haas  writes:
> > I am not really in favor of half-fixing this.  If we can't
> > conveniently wait until a dropped role is completely out of the
> > system, then I don't see a lot of point in trying to do it in the
> > limited cases where we can.  If LEFT JOIN is the way to go, then,
> > blech, but, so be it.
> 
> I concur.  Let's put the left join(s) into those views and call it
> good.

I'd suggest we also add some notes to the documentation that the correct
approach to dropping users is to disallow access first, then kill any
existing backends, and then drop the user.  That, plus the left joins,
seems like it's good enough.

> BTW, I think we would need the left joins even if we had interlocking
> in DROP, just to protect ourselves against race conditions.  Remember
> that what pg_stat_activity shows is a snapshot, which might be more or
> less out of date compared to the catalog contents.

True, though that would likely be a much smaller set of cases that might
also be short lived.

Might be good to also note in the docs how to kill off sessions which
are regular users but which no longer have a username, for folks who end
up in this situation that they managed to drop a role which still had
connections to the system.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] avg,first,last,median in one query

[Jim Nasby]

On 3/24/16 9:00 AM, Konstantin Knizhnik wrote:

But unfortunately it is not possible to calculate median is such way
because percentile_disc is not compatible with OVER:


I don't know if you could use cume_dist()[1] to do this, but even if you 
can't it probably wouldn't be hard to modify it to do what you need.


[1] http://www.postgresql.org/docs/9.5/static/functions-window.html
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.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] Alter or rename enum value

[Jim Nasby]

On 3/24/16 2:00 PM, Matthias Kurz wrote:

ALTER TYPE bogon DROP VALUE 'cat'; -- not implemented in 9.5 but should
work in future
ROLLBACK;


Dropping a value is significantly harder because that value could be in use.

I'm certain there's a really good reason adding new values isn't allowed 
inside of a transaction. It's probably documented in the code.


To answer your question about "what goes into a release", there's really 
no process for that. What goes into a release is what someone was 
interested enough in to get community approval for the idea, write the 
patch, and shepard the patch through the review process. So if you want 
these features added, you need to either: do it yourself, convince 
someone else to do it for free, or pay someone to do it for you.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.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] Rationalizing code-sharing among src/bin/ directories

[Tom Lane]

Michael Paquier  writes:
> On Fri, Mar 25, 2016 at 1:11 AM, Alvaro Herrera
>  wrote:
>> I wondered about the list stuff while messing about in pg_dump awhile
>> ago.  It seems moderately okay, but not terribly generic; maybe we
>> should get rid of all that stuff and make ilist.c available to frontend.
>> Not sure how easy is that, given that AFAIR ilist uses elog.  Anyway
>> maybe we can discuss that in the future, to avoid blocking your patch.

> Definitely worth it, list mimics of what is in pg_dump are located in
> pg_basebackup, and since a 9.6 patch in psql as well. That's as much
> code duplication.

Well, it's not a *lot* of code duplication.  Agreed, it's a bit ugly
that we've got three or four copied-and-pasted versions of
simple_foo_list_append, but I'm not convinced that it'd be worth the
trouble to do anything about that.  If FE code starts needing more list
functionality than that, maybe importing ilist or similar would be
worthwhile, but right now I think it would be overkill.

> Preventing the use of elog in the frontend is something that has been
> addressed multiple times with FRONTEND, so that's not likely going to
> be an issue I think. Andres has mentioned as well having some elog
> stuff available in frontend..

I'm on board with doing something about that one, though.  There are
quite a lot of places that could be cleaned up.

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] Proposal: "Causal reads" mode for load balancing reads without stale data

[Michael Paquier]

On Thu, Mar 24, 2016 at 11:20 PM, Robert Haas  wrote:
> On Thu, Mar 24, 2016 at 2:11 AM, Michael Paquier
>  wrote:
>> -   SyncRepWaitForLSN(gxact->prepare_end_lsn);
>> +   SyncRepWaitForLSN(gxact->prepare_end_lsn, false);
>> Isn't it important to ensure that a PREPARE LSN is applied as well on
>> the standby with remote_apply? Say if an application prepares a
>> transaction, it would commit locally but its LSN may not be applied on
>> the standby with this patch. That would be a surprising behavior for
>> the user.
>
> You need to wait for COMMIT PREPARED, but I don't see why you need to
> wait for PREPARE itself.

Multi-master conflict resolution. Knowing in-time that a prepared
transaction has been applied is useful on another node for lock
conflicts resolution. Say a PRIMARY KEY insert is prepared, and we
want to know at application level that its prepared state is visible
everywhere.
-- 
Michael


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

Re: [HACKERS] [PROPOSAL] VACUUM Progress Checker.

[Amit Langote]

On 2016/03/24 22:01, Robert Haas wrote:
> On Thu, Mar 24, 2016 at 8:45 AM, Rahila Syed  wrote:
>>
>> -   values[i+3] =
>> UInt32GetDatum(beentry->st_progress_param[i]);
>> +   values[i+3] =
>> Int64GetDatum(beentry->st_progress_param[i]);
>>
>>
>> Attached patch fixes this.
> 
> Uggh, what a stupid mistake on my part.
> 
> Committed.  Thanks for the patch.

Thanks Rahila and Robert.

- Amit




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

Re: [HACKERS] [WIP] Effective storage of duplicates in B-tree index.

[Jim Nasby]

On 3/24/16 10:21 AM, Alexander Korotkov wrote:

1) It's a great feature many users dream about.


Doesn't matter if it starts eating their data...


2) Patch is not very big.
3) Patch doesn't introduce significant infrastructural changes.  It just
change some well-isolated placed.


It doesn't really matter how big the patch is, it's a question of "What 
did the patch fail to consider?". With something as complicated as the 
btree code, there's ample opportunities for missing things. (And FWIW, 
I'd argue that a 51kB patch is certainly not small, and a patch that is 
doing things in critical sections isn't terribly isolated).


I do think this will be a great addition, but it's just too late to be 
adding this to 9.6.


(BTW, I'm getting bounces from a.lebe...@postgrespro.ru, as well as 
postmaster@. I emailed i...@postgrespro.ru about this but never heard back.)

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.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] Rationalizing code-sharing among src/bin/ directories

[Michael Paquier]

On Fri, Mar 25, 2016 at 1:11 AM, Alvaro Herrera
 wrote:
> Tom Lane wrote:
>
>> What I propose doing is leaving the above-listed items in
>> pg_dump/dumputils.h/.c, and moving the rest of what's in those files
>> to new files src/include/fe_utils/string_utils.h and
>> src/fe_utils/string_utils.c.
>
> Seems reasonable.
>
>> This name is a bit arbitrary, but most of what's there is string
>> processing of some flavor or other, with some list processing thrown
>> in for good measure.  If anyone's got a different color to paint this
>> bikeshed, please speak up.
>
> I wondered about the list stuff while messing about in pg_dump awhile
> ago.  It seems moderately okay, but not terribly generic; maybe we
> should get rid of all that stuff and make ilist.c available to frontend.
> Not sure how easy is that, given that AFAIR ilist uses elog.  Anyway
> maybe we can discuss that in the future, to avoid blocking your patch.

Definitely worth it, list mimics of what is in pg_dump are located in
pg_basebackup, and since a 9.6 patch in psql as well. That's as much
code duplication.

Preventing the use of elog in the frontend is something that has been
addressed multiple times with FRONTEND, so that's not likely going to
be an issue I think. Andres has mentioned as well having some elog
stuff available in frontend..
-- 
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] Using quicksort for every external sort run

[Peter Geoghegan]

On Sun, Mar 20, 2016 at 11:01 PM, Peter Geoghegan  wrote:
> Allowing 0 tuple runs in rare cases seems like the simplest solution.
> After all, mergeprereadone() is expressly prepared for 0 tuple runs.
> It says "ensure that we have at least one tuple, if any are to be
> had". There is no reason to assume that it says this only because it
> imagines that no tuples might be found *only after* the first preread
> for the merge (by which I mean I don't think that only applies when a
> final on-the-fly merge reloads tuples from one particular tape
> following running out of tuples of the tape/run in memory).

I just realized that there is what amounts to an over-zealous
assertion in dumpbatch():

> +* When this edge case hasn't occurred, the first memtuple should not
> +* be found to be heapified (nor should any other memtuple).
> +*/
> +   Assert(state->memtupcount == 0 ||
> +  state->memtuples[0].tupindex == HEAP_RUN_NEXT);

The problem is that state->memtuples[0].tupindex won't have been
*reliably* initialized here. We could make sure that it is for the
benefit of this assertion, but I think it would be better to just
remove the assertion, which isn't testing very much over and above the
similar assertions that appears in the only dumpbatch() caller,
dumptuples().

-- 
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] NOT EXIST for PREPARE

[Andreas Karlsson]

On 03/24/2016 11:21 PM, Merlin Moncure wrote:

Personally I think the right solution would be to add support for prepared
statements in pgbouncer, and have pgbouncer run PREPARE as necessary, either
after opening a new connection to the database or at the first use of a
given prepared statement in a connection.


maybe so. A while back I was running a hacked pgbouncer that had
support for async notifications (i still have the code around
somewhere).  It can be done -- however this not a complete solution;
both LISTEN and PREPARE are possible from within server-side
functions.


Yes, it is not a complete solution, but it solves the problems of many 
users. I think even just supporting the protocol level prepare and 
execute commands would be enough for many of those who have problems 
with pgbouncer.


Andreas


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

Re: [HACKERS] NOT EXIST for PREPARE

[Merlin Moncure]

On Thu, Mar 24, 2016 at 2:52 PM, Andreas Karlsson  wrote:
> On 03/23/2016 09:10 PM, Stephen Frost wrote:
>>
>> * Merlin Moncure (mmonc...@gmail.com) wrote:
>>>
>>> No one is arguing that that you should send it any every time (at
>>> least -- I hope not).
>>
>>
>> I'm not sure I follow how you can avoid that though?
>>
>> pgbouncer in transaction pooling mode may let a particular connection
>> die off and then, when you issue a new request, create a new one- which
>> won't have any prepared queries in it, even though you never lost your
>> connection to pgbouncer.
>>
>> That's why I was saying you'd have to send it at the start of every
>> transaction, which does add to network load and requires parsing, etc.
>> Would be nice to avoid that, if possible, but I'm not quite sure how.
>>
>> One thought might be to have the server somehow have a pre-canned set of
>> queries already set up and ready for you to use when you connect,
>> without any need to explicitly prepare them, etc.
>
> Personally I think the right solution would be to add support for prepared
> statements in pgbouncer, and have pgbouncer run PREPARE as necessary, either
> after opening a new connection to the database or at the first use of a
> given prepared statement in a connection.

maybe so. A while back I was running a hacked pgbouncer that had
support for async notifications (i still have the code around
somewhere).  It can be done -- however this not a complete solution;
both LISTEN and PREPARE are possible from within server-side
functions.

melrin


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

Re: [HACKERS] btree_gin and btree_gist for enums

[Andrew Dunstan]

On 03/24/2016 12:40 PM, Matt Wilmas wrote:



It would be *really* nice to have this in 9.6.  It seems it's simply 
filling out functionality that should already be there, right?  An 
oversight/bug fix so it works "as advertised?" :-)



I think that would be stretching the process a bit far. I'm certainly 
not prepared to commit it unless there is a general consensus to make an 
exception for it.



Is any other btree type-compatibility missing from these modules?  (I 
notice the btree_gin docs don't mention "numeric," but it works.)



uuid would be another type that should be fairly easily covered but isn't.





I just looked over the patch, and the actual code addition/changes 
seem pretty small and straightforward (or am I wrong?). 




Yes, sure, it's all fairly simple. Took me a little while to understand 
what I was doing, but once I did it was pretty much plain sailing.


cheers

andrew




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

Re: [HACKERS] [WIP] Effective storage of duplicates in B-tree index.

[Peter Geoghegan]

On Thu, Mar 24, 2016 at 7:17 AM, Robert Haas  wrote:
> I really like this idea, and the performance results seem impressive,
> but I think we should push this out to 9.7.  A btree patch that didn't
> have WAL support until two and a half weeks into the final CommitFest
> just doesn't seem to me like a good candidate.  First, as a general
> matter, if a patch isn't code-complete at the start of a CommitFest,
> it's reasonable to say that it should be reviewed but not necessarily
> committed in that CommitFest.  This patch has had some review, but I'm
> not sure how deep that review is, and I think it's had no code review
> at all of the WAL logging changes, which were submitted only a week
> ago, well after the CF deadline.  Second, the btree AM is a
> particularly poor place to introduce possibly destabilizing changes.
> Everybody depends on it, all the time, for everything.  And despite
> new tools like amcheck, it's not a particularly easy thing to debug.

Regrettably, I must agree. I don't see a plausible path to commit for
this patch in the ongoing CF.

I think that Anastasia did an excellent job here, and I wish I could
have been of greater help sooner. Nevertheless, it would be unwise to
commit this given the maturity of the code. There have been very few
instances of performance improvements to the B-Tree code for as long
as I've been interested, because it's so hard, and the standard is so
high. The only example I can think of from the last few years is
Kevin's commit 2ed5b87f96 and Tom's commit 1a77f8b63d both of which
were far less invasive, and Simon's commit c7111d11b1, which we just
outright reverted from 9.5 due to subtle bugs (and even that was
significantly less invasive than this patch). Improving nbtree is
something that requires several rounds of expert review, and that's
something that's in short supply for the B-Tree code in particular. I
think that a new testing strategy is needed to make this easier, and I
hope to get that going with amcheck. I need help with formalizing a
"testing first" approach for improving the B-Tree code, because I
think it's the only way that we can move forward with projects like
this. It's *incredibly* hard to push forward patches like this given
our current, limited testing strategy.

-- 
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] Relation extension scalability

[Robert Haas]

On Thu, Mar 24, 2016 at 7:17 AM, Dilip Kumar  wrote:
>> Yet another possibility could be to call it as
>> GetPageWithFreeSpaceExtended and call it from GetPageWithFreeSpace with
>> value of oldPage as InvalidBlockNumber.
>
> Yes I like this.. Changed the same.

After thinking about this some more, I don't think this is the right
approach.  I finally understand what's going on here:
RecordPageWithFreeSpace updates the FSM lazily, only adjusting the
leaves and not the upper levels.  It relies on VACUUM to update the
upper levels.  This seems like it might be a bad policy in general,
because VACUUM on a very large relation may be quite infrequent, and
you could lose track of a lot of space for a long time, leading to a
lot of extra bloat.  However, it's a particularly bad policy for bulk
relation extension, because you're stuffing a large number of totally
free pages in there in a way that doesn't make them particularly easy
for anybody else to discover.  There are two ways we can fail here:

1. Callers who use GetPageWithFreeSpace() rather than
GetPageFreeSpaceExtended() will fail to find the new pages if the
upper map levels haven't been updated by VACUUM.

2. Even callers who use GetPageFreeSpaceExtended() may fail to find
the new pages.  This can happen in two separate ways, namely (a) the
lastValidBlock saved by RelationGetBufferForTuple() can be in the
middle of the relation someplace rather than near the end, or (b) the
bulk-extension performed by some other backend can have overflowed
onto some new FSM page that won't be searched even though a relatively
plausible lastValidBlock was passed.

It seems to me that since we're adding a whole bunch of empty pages at
once, it's worth the effort to update the upper levels of the FSM.
This isn't a case of discovering a single page with an extra few bytes
of storage available due to a HOT prune or something - this is a case
of putting at least 20 and plausibly hundreds of extra pages into the
FSM.   The extra effort to update the upper FSM pages is trivial by
comparison with the cost of extending the relation by many blocks.

So, I suggest adding a new function FreeSpaceMapBulkExtend(BlockNumber
first_block, BlockNumber last_block) which sets all the FSM entries
for pages between first_block and last_block to 255 and then bubbles
that up to the higher levels of the tree and all the way to the root.
Have the bulk extend code use that instead of repeatedly calling
RecordPageWithFreeSpace.  That should actually be much more efficient,
because it can call fsm_readbuf(), LockBuffer(), and
UnlockReleaseBuffer() just once per FSM page instead of once per FSM
page *per byte modified*.  Maybe that makes no difference in practice,
but it can't hurt.

-- 
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: [BUGS] Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Alvaro Herrera]

Christian Ullrich wrote:

> To be honest, I'm not sure what can and cannot be done in auth code. I
> took inspiration from the existing SSPI code and nearly every error
> check in pg_SSPI_recvauth() ends up doing ereport(ERROR) already,
> directly or via pg_SSPI_error(). If this could cause serious trouble,
> someone would have noticed yet.

I think the problem is whether the report is sent to the client or not,
but I may be confusing with something else (COMMERROR reports?).

> What *could* happen, anyway? Can ereport(ERROR) in a backend make the
> postmaster panic badly enough to force a shared memory reset?

Probably not, since it's running in a backend already at that point, not
in postmaster.

-- 
Álvaro Herrerahttp://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: [BUGS] Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Christian Ullrich]

* From: Alvaro Herrera [mailto:alvhe...@2ndquadrant.com]

> Christian Ullrich wrote:

> > * Christian Ullrich wrote:
> >
> > >* From: Magnus Hagander [mailto:mag...@hagander.net]
> 
> > >>Code uses a mix of malloc() and palloc() (through sprintf). Is there
> > >>a reason for that?
> > >
> > >I wasn't sure which to prefer, so I looked around in auth.c, and
> > >other than RADIUS, everything seems to use malloc() (although the
> > >sample size is not too great). Should I use palloc() instead?
> >
> > The single instance of malloc() has been replaced with palloc().
> 
> I'm wary of palloc() in this code actually ... if the allocation fails,
> I'm not sure it's okay to use ereport(ERROR) which is what would happen
> with palloc.  With the malloc code, you report the problem with
> elog(LOG) and then return STATUS_ERROR which lets the calling code
> handle the failure in a different way.  I didn't actually review your
> new code, but I recall this from previous readings of auth code; so if
> you're going to use palloc(), you better audit what happens on OOM.
> 
> For the same reason, using psprintf is probably not acceptable either.

To be honest, I'm not sure what can and cannot be done in auth code. I took 
inspiration from the existing SSPI code and nearly every error check in 
pg_SSPI_recvauth() ends up doing ereport(ERROR) already, directly or via 
pg_SSPI_error(). If this could cause serious trouble, someone would have 
noticed yet.

What *could* happen, anyway? Can ereport(ERROR) in a backend make the 
postmaster panic badly enough to force a shared memory reset?

-- 
Christian



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

Re: [HACKERS] NOT EXIST for PREPARE

[Andreas Karlsson]

On 03/23/2016 09:10 PM, Stephen Frost wrote:

* Merlin Moncure (mmonc...@gmail.com) wrote:

No one is arguing that that you should send it any every time (at
least -- I hope not).


I'm not sure I follow how you can avoid that though?

pgbouncer in transaction pooling mode may let a particular connection
die off and then, when you issue a new request, create a new one- which
won't have any prepared queries in it, even though you never lost your
connection to pgbouncer.

That's why I was saying you'd have to send it at the start of every
transaction, which does add to network load and requires parsing, etc.
Would be nice to avoid that, if possible, but I'm not quite sure how.

One thought might be to have the server somehow have a pre-canned set of
queries already set up and ready for you to use when you connect,
without any need to explicitly prepare them, etc.


Personally I think the right solution would be to add support for 
prepared statements in pgbouncer, and have pgbouncer run PREPARE as 
necessary, either after opening a new connection to the database or at 
the first use of a given prepared statement in a connection.


Application level connection poolers with prepared statement support, 
e.g. sequel for Ruby, does not need any special support from PostgreSQL 
and work just fine with our current feature set.


Andreas


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

Re: [HACKERS] PostgreSQL 9.6 behavior change with set returning (funct).*

[Merlin Moncure]

On Wed, Mar 23, 2016 at 3:18 PM, Stephen Frost  wrote:
> I have to side with what I believe is Tom's position on this one.  I do
> like the notion of throwing an error in cases where someone sent us
> something that we're pretty sure is wrong, but I don't agree that we
> should continue to carry on bug-compatibility with things that are
> already one foot in the grave and really just need to be shoved all the
> way in.

Well, the fact that it turns out to be 2+ SRF, not just 1 as a trigger
has significantly lowered my alarm.  I agree that such usages are tiny
and the LCM way of determining rows is definitely bizarre. Don't
believe me?  Try figuring out when

select generate_series(1,nextval('s')), generate_series(1,nextval('s'));

terminates.  (hint: it doesn't).   Another cute multiple SRF
invocation: 
http://merlinmoncure.blogspot.com/2007/12/12-days-of-christmas-postgresql-style.html
:-)

> This isn't the only break in backwards compatibility we've had over the
> years and is pretty far from the largest (string escaping, anyone?  or
> removing implicit casts?) and I'd argue we're better off for it.

String escaping was an unmitigated disaster. Implict cast removal cost
my company a couple of hundred thousand bucks and came within a hair
of pushing postgres out completely (not that I'm complaining, we're
the better for that but these decisions must not be taken lightly).
Things are different now.

On Wed, Mar 23, 2016 at 5:34 PM, Tom Lane  wrote:
> (The SRF invocations might be buried inside expressions, but we'd find
> them and convert them anyway.  Also, we'd merge textually-identical SRF
> invocations into a single ROWS FROM entry to avoid multiple evaluation,
> at least for SRFs not marked volatile.)  Having done that, the executor's
> support for SRFs in general expressions could be removed, a significant
> savings.

That's just brilliant -- I'd be on board with that FWIW.

merlin


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

Re: [HACKERS] Rationalizing code-sharing among src/bin/ directories

[Tom Lane]

Aleksey Demakov  writes:
> Hi there,
>> On 23 Mar 2016, at 22:38, Tom Lane  wrote:
>> Anybody want to bikeshed the directory name src/feutils?  Maybe fe-utils
>> would be more readable.  And where to put the corresponding header files?
>> src/include/fe-utils?

> For me “utils" sounds like something of auxiliary nature. if some pretty
> basic stuff is going to be added there, then I believe that fe_common or
> perhaps fe_shared would be a bit more suitable.

Meh...  The stuff that is in-scope for it at the moment doesn't seem
all that basic.  It's just assorted widgets that turned out to be
useful outside their original home.  Really basic stuff is going into
src/common/ these days.

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] Alter or rename enum value

[Matthias Kurz]

>
> ilm...@ilmari.org (Dagfinn Ilmari Mannsåker) writes:
>
> >
> > I was bored and thought "how hard could it be?", and a few hours'
> > hacking later, I have something that seems to work.  It doesn't do IF
> > NOT EXISTS yet, and the error messaging could do with some improvement,
> > and there are no docs.  The patch is attached, as well as at
> > https://github.com/ilmari/postgres/commit/enum-alter-value
>
> I've added it to the 2016-09 commitfest as well:
> https://commitfest.postgresql.org/10/588/


Nice! Thank you!

Actually you still miss a "DROP VALUE" action. Also please make sure this
also works when altering an existing enum within a new transaction -
otherwise it does not really make sense (Usually someone wants to alter
existing enums, not ones that have just been created).

As a result a script like this should pass without problems:
-- ### script start
CREATE TYPE bogus AS ENUM('dog');

-- TEST 1:
BEGIN;
ALTER TYPE bogus ADD VALUE 'cat'; -- fails in 9.5 because of the
transaction but should work in future
COMMIT;

-- TEST 2:
BEGIN;
ALTER TYPE bogus RENAME TO bogon;
ALTER TYPE bogon ADD VALUE 'horse'; -- fails in 9.5 because of the
transaction but should work in future
COMMIT;

-- TEST 3:
BEGIN;
ALTER TYPE bogon ALTER VALUE 'dog' TO 'pig'; -- not implemented in 9.5 but
should work in future
ROLLBACK;

-- TEST 4:
BEGIN;
ALTER TYPE bogon DROP VALUE 'cat'; -- not implemented in 9.5 but should
work in future
ROLLBACK;
-- ### script end

End result of enum "bogon" (which was named "bogus" at the beginning of the
script):
-- ###
pig
horse
-- ###

Thank you!

Re: [HACKERS] Combining Aggregates

[Robert Haas]

On Thu, Mar 24, 2016 at 1:17 PM, Robert Haas  wrote:
> I'm going to read through the code again now.

OK, I noticed another documentation problem: you need to update
catalogs.sgml for these new columns.

+* Validate the serial function, if present. We must ensure
that the return
+* Validate the de-serial function, if present. We must ensure that the

I think that you should refer to these consistently in the comments as
the "serialization function" and the "deserialization function", even
though the SQL syntax is different.  And unhyphenated.

+   /* check that we also got a serial type */
+   if (!OidIsValid(aggSerialType))
+   ereport(ERROR,
+   (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+errmsg("must specify serialization
type when specifying serialization function")));

I think that in parallel cases, this check is in DefineAggregate(),
not here.  See, e.g. "aggregate mfinalfunc must not be specified
without mstype".

Existing type parameters to CREATE AGGREGATE have IsPolymorphicType()
checks to enforce sanity in various ways, but you seem not to have
added that for the serial type.

+   /* don't call a strict serial function with
NULL input */
+   if (pertrans->serialfn.fn_strict &&
+   pergroupstate->transValueIsNull)
+   continue;

Shouldn't this instead set aggnulls[aggno] = true?  And doesn't the
hunk in combine_aggregates() have the same problem?

+   /*
+* serial and de-serial functions must match, if
present. Remember that
+* these will be InvalidOid if they're not required
for this agg node
+*/

Explain WHY they need to match.  And maybe update the overall comment
for the function.

+ "'-' AS
aggdeserialfn,aggmtransfn, aggminvtransfn, "

Whitespace.

In your pg_aggregate.h changes, avg(numeric) sets aggserialtype but no
aggserialfn or aggdeserialfn.

-- 
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] Alter or rename enum value

[Dagfinn Ilmari Mannsåker]

ilm...@ilmari.org (Dagfinn Ilmari Mannsåker) writes:

>
> I was bored and thought "how hard could it be?", and a few hours'
> hacking later, I have something that seems to work.  It doesn't do IF
> NOT EXISTS yet, and the error messaging could do with some improvement,
> and there are no docs.  The patch is attached, as well as at
> https://github.com/ilmari/postgres/commit/enum-alter-value

I've added it to the 2016-09 commitfest as well:
https://commitfest.postgresql.org/10/588/

-- 
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen



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

Re: [HACKERS] Alter or rename enum value

[Dagfinn Ilmari Mannsåker]

Matthias Kurz  writes:

[altering and dropping enum values]

>>> Andrew Dunstan  writes:
>>> > On 03/09/2016 11:07 AM, Tom Lane wrote:
>>> >> I have a vague recollection that we discussed this at the time the enum
>>> >> stuff went in, and there are concurrency issues?  Don't recall details
>>> >> though.
>>>
>>> > Rings a vague bell, but should it be any worse than adding new labels?
>>>
>>> I think what I was recalling is the hazards discussed in the comments for
>>> RenumberEnumType.  However, the problem there is that a backend could make
>>> inconsistent ordering decisions due to seeing two different pg_enum rows
>>> under different snapshots.  Updating a single row to change its name
>>> doesn't seem to have a comparable hazard, and it wouldn't affect ordering
>>> anyway.  So it's probably no worse than any other object-rename situation.
>>>
>>> regards, tom lane
>>>
>>
>>
> Is there a way or a procedure we can go through to make the these ALTER
> TYPE enhancements a higher priority? How do you choose which
> features/enhancements to implement (next)?

I was bored and thought "how hard could it be?", and a few hours'
hacking later, I have something that seems to work.  It doesn't do IF
NOT EXISTS yet, and the error messaging could do with some improvement,
and there are no docs.  The patch is attached, as well as at
https://github.com/ilmari/postgres/commit/enum-alter-value

>From 1cb8feac0179eaa44720822ad84e146ec3ba67ff Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Thu, 24 Mar 2016 17:50:58 +
Subject: [PATCH] Add ALTER TYPE ... ALTER VALUE '...' TO '...'

Needs docs, and when altering a non-existent value to one that already
exists, it should probably complain about the former fact, not the
latter.
---
 src/backend/catalog/pg_enum.c  | 93 ++
 src/backend/commands/typecmds.c| 17 +--
 src/backend/parser/gram.y  | 14 ++
 src/include/catalog/pg_enum.h  |  2 +
 src/include/nodes/parsenodes.h |  1 +
 src/test/regress/expected/enum.out | 22 +
 src/test/regress/sql/enum.sql  | 11 +
 7 files changed, 155 insertions(+), 5 deletions(-)

diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
index af89daa..81e170c 100644
--- a/src/backend/catalog/pg_enum.c
+++ b/src/backend/catalog/pg_enum.c
@@ -464,6 +464,99 @@ restart:
 
 
 /*
+ * RenameEnumLabel
+ *		Add a new label to the enum set. By default it goes at
+ *		the end, but the user can choose to place it before or
+ *		after any existing set member.
+ */
+void
+RenameEnumLabel(Oid enumTypeOid,
+const char *oldVal,
+const char *newVal)
+{
+	Relation	pg_enum;
+	HeapTuple	enum_tup;
+	CatCList   *list;
+	int			nelems;
+	int			nbr_index;
+	Form_pg_enum nbr_en;
+
+
+
+	/* check length of new label is ok */
+	if (strlen(newVal) > (NAMEDATALEN - 1))
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_NAME),
+ errmsg("invalid enum label \"%s\"", newVal),
+ errdetail("Labels must be %d characters or less.",
+		   NAMEDATALEN - 1)));
+
+	/*
+	 * Acquire a lock on the enum type, which we won't release until commit.
+	 * This ensures that two backends aren't concurrently modifying the same
+	 * enum type.  Without that, we couldn't be sure to get a consistent view
+	 * of the enum members via the syscache.  Note that this does not block
+	 * other backends from inspecting the type; see comments for
+	 * RenumberEnumType.
+	 */
+	LockDatabaseObject(TypeRelationId, enumTypeOid, 0, ExclusiveLock);
+
+	/*
+	 * Check if label is already in use.  The unique index on pg_enum would
+	 * catch this anyway, but we prefer a friendlier error message.
+	 */
+	enum_tup = SearchSysCache2(ENUMTYPOIDNAME,
+			   ObjectIdGetDatum(enumTypeOid),
+			   CStringGetDatum(newVal));
+	if (HeapTupleIsValid(enum_tup))
+	{
+		ReleaseSysCache(enum_tup);
+		ereport(ERROR,
+(errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("enum label \"%s\" already exists",
+		newVal)));
+	}
+
+	pg_enum = heap_open(EnumRelationId, RowExclusiveLock);
+
+	/* Get the list of existing members of the enum */
+	list = SearchSysCacheList1(ENUMTYPOIDNAME,
+			   ObjectIdGetDatum(enumTypeOid));
+	nelems = list->n_members;
+
+	/* Locate the element to rename */
+	for (nbr_index = 0; nbr_index < nelems; nbr_index++)
+	{
+		enum_tup = &(list->members[nbr_index]->tuple);
+		nbr_en = (Form_pg_enum) GETSTRUCT(enum_tup);
+
+		if (strcmp(NameStr(nbr_en->enumlabel), oldVal) == 0)
+			break;
+	}
+	if (nbr_index >= nelems)
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"%s\" is not an existing enum label",
+		oldVal)));
+
+	enum_tup = heap_copytuple(enum_tup);
+	nbr_en = (Form_pg_enum) GETSTRUCT(enum_tup);
+	ReleaseCatCacheList(list);
+
+	/* Update new pg_enum entry */
+	namestrcpy(_en->enumlabel, newVal);
+	simple_heap_update(pg_enum, 

Re: [BUGS] Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Alvaro Herrera]

Christian Ullrich wrote:
> * Christian Ullrich wrote:
> 
> >* From: Magnus Hagander [mailto:mag...@hagander.net]

> >>Code uses a mix of malloc() and palloc() (through sprintf). Is there a
> >>reason for that?
> >
> >I wasn't sure which to prefer, so I looked around in auth.c, and other than
> >RADIUS, everything seems to use malloc() (although the sample size is not
> >too great). Should I use palloc() instead?
> 
> The single instance of malloc() has been replaced with palloc().

I'm wary of palloc() in this code actually ... if the allocation fails,
I'm not sure it's okay to use ereport(ERROR) which is what would happen
with palloc.  With the malloc code, you report the problem with
elog(LOG) and then return STATUS_ERROR which lets the calling code
handle the failure in a different way.  I didn't actually review your
new code, but I recall this from previous readings of auth code; so if
you're going to use palloc(), you better audit what happens on OOM.

For the same reason, using psprintf is probably not acceptable either.

-- 
Álvaro Herrerahttp://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] [PATCH] we have added support for box type in SP-GiST index

[Teodor Sigaev]

+  * boxtype_spgist.c

The names on the file header need to be changed, too.

Oops. fixed




I'll try to explain with two-dimensional example over points. ASCII-art:

Thank you for the explanation.  Should we incorporate this with the patch.

added



+ cmp_double(const double a, const double b)

Does this function necessary?  We can just compare the doubles.
Yes, it compares with limited precision as it does by geometry operations. 
Renamed to point actual arguments.





+ boxPointerToRangeBox(BOX *box, RangeBox * rectangle)

The "rectangle" variable in here should be renamed.

fixed




+ Assert(is_infinite(b) == 0);

This is failing on my test.  You can reproduce with the script I have sent.

I didn't know:
# select '(1,inf)'::point;
  point
-
 (1,inf)

fixed




Wouldn't it be simpler to palloc and return the value on those functions?

evalRangeBox() initializes part of RectBox, so, it could not palloc its
result.
Other methods use the same signature just for consistency.


I couldn't understand it.  evalRangeBox() can palloc and return the
result.  evalRectBox() can return the result palloc'ed by
evalRangeBox().  The caller wouldn't need to palloc anything.

evalRangeBox() is used to initialize fields of RangeBox in evalRectBox(). If
evalRangeBox() will palloc its result then we need to copy its result into 
RangeBox struct and free. Let both fucntion use the same interface.



I went through all other variables:

+ int r = is_infinite(a);
+ double  x = *(double *) a;
+ double  y = *(double *) b;
+ spgInnerConsistentIn *in = (spgInnerConsistentIn *) PG_GETARG_POINTER(0);
+ spgLeafConsistentIn *in = (spgLeafConsistentIn *) PG_GETARG_POINTER(0);
+ BOX*leafBox = DatumGetBoxP(in->leafDatum);

Shouldn't they be "const", too?
They could. But it doesn't required. To be in consistent state I've removed 
const modifier where possible






+/*
+ * Begin. This block evaluates the median of coordinates of boxes
+ */


I would rather explain what the function does on the function header.


fixed

The "end" part of it is still there.

Oops again, fixed




Do we really need to copy the traversalValues on allTheSame case.
Wouldn't it work if just the same value is passed for all of them.
The search shouldn't continue after allTheSame case.


Seems, yes. spgist tree could contain a locng branches with allTheSame.


Does SP-GiST allows any node under allTheSame to not being allTheSame?

No, SP-GiST doesn't allow that

  Not setting traversalValues for allTheSame worked fine with my test.
it works until allthesame branch contains only one inner node. Otherwise 
traversalValue will be freeed several times, see spgWalk().

# select i as id, '1,2,3,4'::box as b into x from generate_series(1,100) i;
# create index ix on i using spgist (b);
# select count(*) from x where b && '1,2,3,4'::box; -- coredump
gdb:
#0  0x00080143564a in thr_kill () from /lib/libc.so.7
#1  0x000801435636 in raise () from /lib/libc.so.7
#2  0x0008014355b9 in abort () from /lib/libc.so.7
#3  0x00a80739 in in_error_recursion_trouble () at elog.c:199
#4  0x00abb748 in pfree (pointer=0x801e90868) at mcxt.c:1016
#5  0x0053330c in freeScanStackEntry (so=0x801e8d358, 
stackEntry=0x801e935d8) at spgscan.c:47
#6  0x00532cdb in spgWalk (index=0x801f1c588, so=0x801e8d358, 
scanWholeIndex=1 '\001', storeRes=0x532d10 
...




+ if (in->allTheSame)

Most of the things happening before this check is not used in there.
Shouldn't we move this to the top of the function?

yep, fixed




+ out->nodeNumbers = (int *) palloc(sizeof(int) * in->nNodes);

This could go before allTheSame block.

yep, fixed

I've attached all patches again. Thank you very much!

--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


q4d-4.patch.gz
Description: GNU Zip compressed data


traversalValue-2.patch.gz
Description: GNU Zip compressed data


range-1.patch.gz
Description: GNU Zip compressed 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] multivariate statistics v14

[Alvaro Herrera]

Tomas Vondra wrote:

> >+values[Anum_pg_mv_statistic_stamcv  - 1] = PointerGetDatum(data);
> >
> >Why the double space (that's actually in several places in several of
> >the patches).
> 
> To align the whole block like this:
> 
> nulls[Anum_pg_mv_statistic_stadeps  -1] = true;
> nulls[Anum_pg_mv_statistic_stamcv   -1] = true;
> nulls[Anum_pg_mv_statistic_stahist  -1] = true;
> nulls[Anum_pg_mv_statistic_standist -1] = true;
> 
> But I won't fight for this too hard, if it breaks rules somehow.

Yeah, it will be undone by pgindent.  I suggest you pgindent all the
patches in the series.  With some clever patch vs. patch -R application,
you can do it without having to resolve any conflicts when pgindent
modifies code that a patch further up in the series modifies again.

-- 
Álvaro Herrerahttp://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] Rationalizing code-sharing among src/bin/ directories

[Aleksey Demakov]

Hi there,

> On 23 Mar 2016, at 22:38, Tom Lane  wrote:
> Anybody want to bikeshed the directory name src/feutils?  Maybe fe-utils
> would be more readable.  And where to put the corresponding header files?
> src/include/fe-utils?


For me “utils" sounds like something of auxiliary nature. if some pretty
basic stuff is going to be added there, then I believe that fe_common or
perhaps fe_shared would be a bit more suitable.

Regards,
Aleksey



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

Re: [HACKERS] dealing with extension dependencies that aren't quite 'e'

[Abhijit Menon-Sen]

At 2016-03-24 12:31:16 -0300, alvhe...@2ndquadrant.com wrote:
>
> In other words I think the conclusion here is that we must use
> qualified_name in the new production rather than switching the old
> production to any_name.

Makes sense.

> I think I would like to see code implement both alternatives to see
> which one is least ugly.  Maybe a third idea will manifest itself upon
> seeing those.

Here's the first one. ExecAlterObjectDependsStmt() looks like this:

+ObjectAddress
+ExecAlterObjectDependsStmt(AlterObjectDependsStmt *stmt)
+{
+ObjectAddress address;
+ObjectAddress extAddr;
+Relationrel = NULL;
+
+/*
+ * If the parser handed us a RangeVar, we add the relation's name to
+ * stmt->objname so that we can pass it to get_object_address().
+ */
+if (stmt->relation)
+{
+stmt->objname = lcons(makeString(stmt->relation->relname), 
stmt->objname);
+if (stmt->relation->schemaname)
+stmt->objname = lcons(makeString(stmt->relation->schemaname), 
stmt->objname);
+if (stmt->relation->catalogname)
+stmt->objname = lcons(makeString(stmt->relation->catalogname), 
stmt->objname);
+}
+
+address = get_object_address(stmt->objectType, stmt->objname, 
stmt->objargs,
+ , AccessExclusiveLock, false);
+
+if (rel)
+heap_close(rel, NoLock);
+
+extAddr = get_object_address(OBJECT_EXTENSION, stmt->extname, NULL,
+ , AccessExclusiveLock, false);
+
+recordDependencyOn(, , DEPENDENCY_AUTO_EXTENSION);
+
+return address;
+}

(This works fine for both functions and triggers, I tested it.)

Complete patch attached for reference.

I'll post the get_object_address_rv() variant tomorrow, but comments are
welcome in the meantime.

-- Abhijit
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 951f59b..189b771 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -2864,6 +2864,19 @@
   
  
 
+
+
+ DEPENDENCY_AUTO_EXTENSION (x)
+ 
+  
+   The dependent object is not a member of the extension that is the
+   referenced object (and so should not be ignored by pg_dump), but
+   cannot function without it and should be dropped when the
+   extension itself is. The dependent object may be dropped on its
+   own as well.
+  
+ 
+

 
Other dependency flavors might be needed in future.
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index c48e37b..a284bed 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -587,6 +587,7 @@ findDependentObjects(const ObjectAddress *object,
 		{
 			case DEPENDENCY_NORMAL:
 			case DEPENDENCY_AUTO:
+			case DEPENDENCY_AUTO_EXTENSION:
 /* no problem */
 break;
 			case DEPENDENCY_INTERNAL:
@@ -786,6 +787,7 @@ findDependentObjects(const ObjectAddress *object,
 subflags = DEPFLAG_NORMAL;
 break;
 			case DEPENDENCY_AUTO:
+			case DEPENDENCY_AUTO_EXTENSION:
 subflags = DEPFLAG_AUTO;
 break;
 			case DEPENDENCY_INTERNAL:
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index 5af0f2f..339a313 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -23,6 +23,7 @@
 #include "catalog/pg_collation.h"
 #include "catalog/pg_conversion.h"
 #include "catalog/pg_event_trigger.h"
+#include "catalog/pg_extension.h"
 #include "catalog/pg_foreign_data_wrapper.h"
 #include "catalog/pg_foreign_server.h"
 #include "catalog/pg_language.h"
@@ -32,6 +33,7 @@
 #include "catalog/pg_opclass.h"
 #include "catalog/pg_opfamily.h"
 #include "catalog/pg_proc.h"
+#include "catalog/pg_trigger.h"
 #include "catalog/pg_ts_config.h"
 #include "catalog/pg_ts_dict.h"
 #include "catalog/pg_ts_parser.h"
@@ -391,6 +393,45 @@ ExecRenameStmt(RenameStmt *stmt)
 }
 
 /*
+ * Executes an ALTER OBJECT / DEPENDS ON EXTENSION statement.
+ *
+ * Return value is the address of the altered object.
+ */
+ObjectAddress
+ExecAlterObjectDependsStmt(AlterObjectDependsStmt *stmt)
+{
+	ObjectAddress address;
+	ObjectAddress extAddr;
+	Relation	rel = NULL;
+
+	/*
+	 * If the parser handed us a RangeVar, we add the relation's name to
+	 * stmt->objname so that we can pass it to get_object_address().
+	 */
+	if (stmt->relation)
+	{
+		stmt->objname = lcons(makeString(stmt->relation->relname), stmt->objname);
+		if (stmt->relation->schemaname)
+			stmt->objname = lcons(makeString(stmt->relation->schemaname), stmt->objname);
+		if (stmt->relation->catalogname)
+			stmt->objname = lcons(makeString(stmt->relation->catalogname), stmt->objname);
+	}
+
+	address = get_object_address(stmt->objectType, stmt->objname, stmt->objargs,
+ , AccessExclusiveLock, false);
+
+	if (rel)
+		heap_close(rel, NoLock);
+
+	extAddr = get_object_address(OBJECT_EXTENSION, stmt->extname, NULL,
+ , AccessExclusiveLock, false);
+
+	recordDependencyOn(, , 

Re: [HACKERS] Combining Aggregates

[Robert Haas]

On Mon, Mar 21, 2016 at 2:18 PM, David Rowley
 wrote:
> I've attached 2 of the patches which are affected by the changes.

I think the documentation for 0001 needs some work yet.  The
additional paragraph that you've added...

(1) doesn't seem to appear at a very logical place in the
documentation - I think it should be much further down, as it's a
minor detail.  Maybe document this the same way as the documentation
patch you just sent for the combine-function stuff does it; and

(2) isn't indented consistently with the surrounding paragraphs; and

(3) is missing a closing  tag

Also, I'd just cut this:

+  This is required due to
+  the process model being unable to pass references to INTERNAL
+   types between different PostgreSQL
+  processes.

Instead, I'd change the earlier sentence in the paragraph, which
currently reads:

+  These
+  functions are required in order to allow parallel aggregation for aggregates
+  with an stype of 
+  INTERNAL.

I'd replace the period at end with a comma and add "since
INTERNAL values represent arbitrary in-memory data
structures which can't be passed between processes".  I think that's a
bit smoother.

I'm going to read through the code again 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] BUG #13854: SSPI authentication failure: wrong realm name used

[Robbie Harwood]

Christian Ullrich  writes:

> Updated patch attached.

Okay, I am happy now.  Thanks!


signature.asc
Description: PGP signature

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Petr Jelinek]

On 24/03/16 17:28, Robert Haas wrote:

On Wed, Mar 23, 2016 at 3:17 AM, Michael Paquier
 wrote:

- 0001 fixes the global declarations of TIMEZONE_GLOBAL and
TZNAME_GLOBAL to be WIN32-compliant. I got bitten by that in the ECPG
compilation.


So this isn't going to break other Windows builds?  I mean, if we've
got the names for those symbols wrong, how is this working right now?



We didn't older versions just defined the other variants as well, but 
the _timezone and _tzname have been around since at least VS2003.


--
  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] Combining Aggregates

[Robert Haas]

On Thu, Mar 24, 2016 at 5:22 AM, David Rowley
 wrote:
> On 21 January 2016 at 08:06, Robert Haas  wrote:
>> I re-reviewed this and have committed most of it with only minor
>> kibitizing.  A few notes:
>
> I realised today that the combinefunc is rather undocumented. I've
> attached a patch which aims to fix this.
>
> Comments are welcome.

Looks good to me.  Committed.

-- 
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] btree_gin and btree_gist for enums

[Matt Wilmas]

Hi Andrew, all,

First message here!  I didn't get around to sending an intro/"thank you all" 
e-mail yet, and a small performance (?) patch+idea(s)...  (CPU stuff, since 
I don't otherwise know much about PG internals.)  Anyway...


- Original Message -
From: "Andrew Dunstan"
Sent: Thursday, March 17, 2016


Here is a patch to add enum support to btree_gin and btree_gist. I
didn't include distance operations, as I didn't think it terribly
important, and there isn't a simple way to compute it sanely and
efficiently, so no KNN support.


Major thanks for coming up with this a week after your "enums and indexing" 
message.  My love of all things Postgres has a lot to do with being able to 
do nearly anything one could want (coming from MySQL :-/)!  So I was like, 
"Wait, what?" when you brought up the subject, since this was something I 
hadn't actually tried, but was planning to use btree_gin/gist with a good 
mix of stuff, including enums (array and scalar).


At first I thought it was just arrays of enums, until this patch for the 
contrib extensions (for the btree parts with GIN/GiST; plus GiST 
exclusion?), and your blog posts... [1][2]  I wasn't certain from the second 
post whether your array solution can be used today without this patch (yes, 
just tried 9.5).  So, two separate issues, and the patch addresses scalar 
stuff, IIUC.


It would be *really* nice to have this in 9.6.  It seems it's simply filling 
out functionality that should already be there, right?  An oversight/bug fix 
so it works "as advertised?" :-)  Is any other btree type-compatibility 
missing from these modules?  (I notice the btree_gin docs don't mention 
"numeric," but it works.)


I just looked over the patch, and the actual code addition/changes seem 
pretty small and straightforward (or am I wrong?).  And it's not changing 
anything in the core, so...


Well, just wanted to argue my case! :^)

[1] http://adpgtech.blogspot.com/2016/03/gist-and-gin-support-for-enums.html
[2] http://adpgtech.blogspot.com/2016/03/gin-indexing-array-of-enums.html


cheers

andrew


Thanks,
Matt 




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

Re: [HACKERS] Rationalizing code-sharing among src/bin/ directories

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> This name is a bit arbitrary, but most of what's there is string
>> processing of some flavor or other, with some list processing thrown
>> in for good measure.  If anyone's got a different color to paint this
>> bikeshed, please speak up.

> I wondered about the list stuff while messing about in pg_dump awhile
> ago.  It seems moderately okay, but not terribly generic; maybe we
> should get rid of all that stuff and make ilist.c available to frontend.
> Not sure how easy is that, given that AFAIR ilist uses elog.  Anyway
> maybe we can discuss that in the future, to avoid blocking your patch.

It wouldn't be terribly hard to split the file into, say, string_utils
and simple_list files.  On reflection that seems like a good idea as
long as we're going for cleanup rather than a one-to-one rename.

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] VS 2015 support in src/tools/msvc

[Robert Haas]

On Wed, Mar 23, 2016 at 3:17 AM, Michael Paquier
 wrote:
> - 0001 fixes the global declarations of TIMEZONE_GLOBAL and
> TZNAME_GLOBAL to be WIN32-compliant. I got bitten by that in the ECPG
> compilation.

So this isn't going to break other Windows builds?  I mean, if we've
got the names for those symbols wrong, how is this working right 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] NOT EXIST for PREPARE

[Yury Zhuravlev]

Tom Lane wrote:

because it would break applications
I still do not agree with this. The app expects that there can be no 
mistakes and it does not happen.

I can not invent a situation when it is breaks.

Thanks. 


--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres 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] Rationalizing code-sharing among src/bin/ directories

[Alvaro Herrera]

Tom Lane wrote:

> What I propose doing is leaving the above-listed items in
> pg_dump/dumputils.h/.c, and moving the rest of what's in those files
> to new files src/include/fe_utils/string_utils.h and
> src/fe_utils/string_utils.c.

Seems reasonable.

> This name is a bit arbitrary, but most of what's there is string
> processing of some flavor or other, with some list processing thrown
> in for good measure.  If anyone's got a different color to paint this
> bikeshed, please speak up.

I wondered about the list stuff while messing about in pg_dump awhile
ago.  It seems moderately okay, but not terribly generic; maybe we
should get rid of all that stuff and make ilist.c available to frontend.
Not sure how easy is that, given that AFAIR ilist uses elog.  Anyway
maybe we can discuss that in the future, to avoid blocking your patch.

-- 
Álvaro Herrerahttp://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] BUG #13854: SSPI authentication failure: wrong realm name used

[Christian Ullrich]

* From: Christian Ullrich

> * From: Robbie Harwood [mailto:rharw...@redhat.com]
> 
> > Christian Ullrich  writes:

> > > + /* Replace domainname with realm name. */
> > > + if (upnamerealmsize > domainnamesize)
> > > + {
> > > + pfree(upname);
> > > + ereport(LOG,
> > > + 
> > > (errcode(ERRCODE_INVALID_ROLE_SPECIFICATION),
> > > +  errmsg("realm name too long")));
> > > +  return STATUS_ERROR;
> > > + }
> > > +
> > > + /* Length is now safe. */
> > > + strcpy(domainname, p+1);
> >
> > Is this an actual fail state or something born out of convenience?  A
> > naive reading of this code doesn't explain why it's forbidden for the
> > upn realm to be longer than the domain name.
> 
> Because it's copied *into* domainname right there on the last line.
> 
> That said, sizeof(domainname) is MAXPGPATH, which is 1024, so there is
> absolutely no chance that the realm could be longer -- it would need an
> AD forest at least 16 domains deep.

Oh, sorry, I misunderstood the question. Yes, it's due to convenience, but
a) it *is* rather convenient given the plentiful buffer I get, and
b) doing it differently involves char** inout parameters and potential
trouble with pointer aliasing in the caller, both things I'd rather avoid.

-- 
Christian



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

Re: [HACKERS] NOT EXIST for PREPARE

[Vladimir Sitnikov]

Tom> Not to mention that the whole idea of that being a semantically
Tom> significant property of a name is a monstrous kluge.

You are right here.

Just in case, Marko Kreen says (see [1]) pgbouncer has all the information
required to remap statement names, so he says pgbouncer needs no
cooperation from backend nor from app side in order to implement
prepared statements properly.

[1]: https://github.com/pgbouncer/pgbouncer/issues/126#issuecomment-200900171

Vladimir


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

Re: [HACKERS] Rationalizing code-sharing among src/bin/ directories

[Robert Haas]

On Wed, Mar 23, 2016 at 4:00 PM, Alvaro Herrera
 wrote:
>> Anybody want to bikeshed the directory name src/feutils?  Maybe fe-utils
>> would be more readable.
>
> Yes, +1 for either a - or _ in there.

I vote for an underscore, since that's what we mostly do.

[rhaas pgsql]$ find . -type d | awk -F/ '{print $NF}' | grep - | wc -l
   6
[rhaas pgsql]$ find . -type d | awk -F/ '{print $NF}' | grep _ | wc -l
  74

-- 
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] Show dropped users' backends in pg_stat_activity

[Tom Lane]

Robert Haas  writes:
> I am not really in favor of half-fixing this.  If we can't
> conveniently wait until a dropped role is completely out of the
> system, then I don't see a lot of point in trying to do it in the
> limited cases where we can.  If LEFT JOIN is the way to go, then,
> blech, but, so be it.

I concur.  Let's put the left join(s) into those views and call it
good.

BTW, I think we would need the left joins even if we had interlocking
in DROP, just to protect ourselves against race conditions.  Remember
that what pg_stat_activity shows is a snapshot, which might be more or
less out of date compared to the catalog contents.

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] Rationalizing code-sharing among src/bin/ directories

[Tom Lane]

I wrote:
> I have a modest proposal for improving this: let's move all the code
> that's currently shared by two or more src/bin/ subdirectories into a
> new directory, say src/feutils, and build it into a ".a" library in
> the same way that src/common/ is handled.

Moving along on this project: I'm looking at pg_dump's dumputils.c/.h,
which contains a fair amount of generally-useful stuff but also some
stuff that seems to have no earthly use outside pg_dump/pg_dumpall.
In particular I do not see the point of moving these things to a shared
directory:

PGDUMP_STRFTIME_FMT macro

extern bool buildACLCommands(const char *name, const char *subname,
 const char *type, const char *acls, const char *owner,
 const char *prefix, int remoteVersion,
 PQExpBuffer sql);
extern bool buildDefaultACLCommands(const char *type, const char *nspname,
const char *acls, const char *owner,
int remoteVersion,
PQExpBuffer sql);
extern void buildShSecLabelQuery(PGconn *conn, const char *catalog_name,
 uint32 objectId, PQExpBuffer sql);
extern void emitShSecLabels(PGconn *conn, PGresult *res,
PQExpBuffer buffer, const char *target, const char *objname);

What I propose doing is leaving the above-listed items in
pg_dump/dumputils.h/.c, and moving the rest of what's in those files
to new files src/include/fe_utils/string_utils.h and
src/fe_utils/string_utils.c.  This name is a bit arbitrary, but most of
what's there is string processing of some flavor or other, with some list
processing thrown in for good measure.  If anyone's got a different color
to paint this bikeshed, please speak up.

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] BUG #13854: SSPI authentication failure: wrong realm name used

[Christian Ullrich]

On 2016-03-24 16:35, Christian Ullrich wrote:


* From: Robbie Harwood [mailto:rharw...@redhat.com]


Christian Ullrich  writes:



   pg_SSPI_recvauth(Port *port)
   {
int mtype;
+   int status;


The section of this function for include_realm checking already uses an
int status return code (retval).  I would expect to see them share a
variable rather than have both "retval" and "status".


I would not, because retval is local to that last if, but you are right, status
does not need to be in function scope.


Moved declaration.


+   /* Build SAM name (DOMAIN\\user), then translate to UPN
+  (user@kerberos.realm). The realm name is returned in
+  lower case, but that is fine because in SSPI auth,
+  string comparisons are always case-insensitive. */


Since we're already considering changing things: this is not the comment
style for this file (though it is otherwise a good comment).


True. Will fix.


Reformatted.


+   upname = (char*)palloc(upnamesize);


I don't believe this cast is typically included.


Left over from when this was malloc() before Magnus' first look at it.


Removed.

Updated patch attached.

--
Christian

diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
new file mode 100644
index 3b2935c..f7d7b5a
*** a/doc/src/sgml/client-auth.sgml
--- b/doc/src/sgml/client-auth.sgml
*** omicron bryanh
*** 1097,1102 
--- 1097,1140 
   
  
   
+   compat_realm
+   
+
+ If set to 1, the domain's SAM-compatible name (also known as the
+ NetBIOS name) is used for the include_realm
+ option. This is the default. If set to 0, the true realm name from
+ the Kerberos user principal name is used. Leave this option
+ disabled to maintain compatibility with existing 
+ pg_ident.conf files.
+
+
+ Do not enable this option unless your server runs under a domain
+ account (this includes virtual service accounts on a domain member
+ system) and all clients authenticating through SSPI are also using
+ domain accounts, or authentication will fail.
+
+   
+  
+ 
+  
+   upn_username
+   
+
+ If this option is enabled along with compat_realm,
+ the user name from the Kerberos UPN is used for authentication. If
+ it is disabled (the default), the SAM-compatible user name is used.
+ By default, these two names are identical for new user accounts.
+
+
+ Note that libpq uses the SAM-compatible name if no
+ explicit user name is specified. If you use
+ libpq (e.g. through the ODBC driver), you should
+ leave this option disabled.
+
+   
+  
+ 
+  
map

 
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
new file mode 100644
index 7f1ae8c..6830764
*** a/src/backend/libpq/auth.c
--- b/src/backend/libpq/auth.c
*** typedef SECURITY_STATUS
*** 155,160 
--- 155,165 
(WINAPI * QUERY_SECURITY_CONTEXT_TOKEN_FN) (

   PCtxtHandle, void **);
  static intpg_SSPI_recvauth(Port *port);
+ static intpg_SSPI_make_upn(char *accountname,
+size_t accountnamesize,
+char *domainname,
+size_t domainnamesize,
+bool 
update_accountname);
  #endif
  
  /*
*** pg_SSPI_recvauth(Port *port)
*** 1263,1268 
--- 1268,1282 
  
free(tokenuser);
  
+   if (!port->hba->compat_realm)
+   {
+   int status = pg_SSPI_make_upn(accountname, sizeof(accountname),
+ 
domainname, sizeof(domainname),
+ 
port->hba->upn_username);
+   if (status != STATUS_OK)
+   return status;
+   }
+ 
/*
 * Compare realm/domain if requested. In SSPI, always compare case
 * insensitive.
*** pg_SSPI_recvauth(Port *port)
*** 1298,1303 
--- 1312,1419 
else
return check_usermap(port->hba->usermap, port->user_name, 
accountname, true);
  }
+ 
+ /*
+  * Replaces the domainname with the Kerberos realm name,
+  * and optionally the accountname with the Kerberos user name.
+  */
+ static intpg_SSPI_make_upn(char *accountname,
+size_t accountnamesize,
+

Re: [HACKERS] NOT EXIST for PREPARE

[Tom Lane]

Vladimir Sitnikov  writes:
> Tom>If you think that's not a protocol change, you are mistaken.  It
> Tom>changes a behavior that's specified in the protocol documentation.

> Even if it requires documentation, this particular change will work seamlessly
> across existing implementations of v3 protocol.

No, because it would break applications that are not expecting prepared
statement names starting with '__' to work differently than they did
before.  Not to mention that the whole idea of that being a semantically
significant property of a name is a monstrous kluge.

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] NOT EXIST for PREPARE

[Vladimir Sitnikov]

Tom>If you think that's not a protocol change, you are mistaken.  It
Tom>changes a behavior that's specified in the protocol documentation.

Even if it requires documentation, this particular change will work seamlessly
across existing implementations of v3 protocol.

For instance, it would not require to update pgbouncer to support that
__ convention.
In other words, __ convention is transparent to pgbouncer.

Consider Prepare2 kind of message is added. Then it would require to update
virtually every software that talks v3 protocol.

That is why I say that "some kind of __ convention" does not require protocol
version bump, while "adding new message" does require the bump.

Just to be clear: I'm not fond of encoding the answer to the universe
into statement name.
However, I find that "name convention" a smart invention.

Vladimir


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

Re: [HACKERS] Show dropped users' backends in pg_stat_activity

[Robert Haas]

On Tue, Mar 22, 2016 at 11:35 PM, Kyotaro HORIGUCHI
 wrote:
> Even if blocking DROPs is not perfect for all cases,
> unconditionally allowing to DROP a role still doesn't seem proper
> behavior, especially for replication roles. And session logins
> seem to me to have enough reason to be treated differently than
> disguising as another role using SET ROLE or sec-definer.
>
> The attached patch blocks DROP ROLE for roles that own active
> sessions, and on the other hand prevents a session from being
> activated if the login role is concurrently dropped.
>
> Oskari's LEFT-Join patch is still desirable.
>
> Is this still pointless?

I am not really in favor of half-fixing this.  If we can't
conveniently wait until a dropped role is completely out of the
system, then I don't see a lot of point in trying to do it in the
limited cases where we can.  If LEFT JOIN is the way to go, then,
blech, but, so be 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

Re: [HACKERS] Rationalizing code-sharing among src/bin/ directories

[Tom Lane]

Robert Haas  writes:
> On Wed, Mar 23, 2016 at 4:00 PM, Alvaro Herrera
>  wrote:
>> Yes, +1 for either a - or _ in there.

> I vote for an underscore, since that's what we mostly do.

Yup, I had just counted and come to the same conclusion.  fe_utils/
it shall be.

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] NOT EXIST for PREPARE

[Yury Zhuravlev]

Vladimir Sitnikov wrote:

Just to be clear: I'm not fond of encoding the answer to the universe
into statement name.
However, I find that "name convention" a smart invention.


I forgot one more decision: add GUC variable.
A little fatty for this but not touch the protocol and easy to implement.

Thanks.

--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres 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] BUG #13854: SSPI authentication failure: wrong realm name used

[Christian Ullrich]

* From: Robbie Harwood [mailto:rharw...@redhat.com]

> Christian Ullrich  writes:
> 
> > Updated patch attached.
> 
> I unfortunately don't have windows machines to test this on, but I
> thought it might be helpful to review this anyway since I'm touching
> code in the same general area (GSSAPI).  And as far as I can tell, you
> don't break anything there; master continues to behave as expected.

Thanks for the review.

> Some comments inline:
> 
> >   pg_SSPI_recvauth(Port *port)
> >   {
> > int mtype;
> > +   int status;
> 
> The section of this function for include_realm checking already uses an
> int status return code (retval).  I would expect to see them share a
> variable rather than have both "retval" and "status".

I would not, because retval is local to that last if, but you are right, status
does not need to be in function scope.

> > +   status = pg_SSPI_make_upn(accountname, sizeof(accountname),
> > + domainname,
> sizeof(domainname),
> > + 
> > port->hba->upn_username);
> 
> This is the only place I see that this function is called.  That being
> the case, why bother with the sizes of parameters?  Why doesn't
> pg_SSPI_make_upn() just call sizeof() by itself rather than taking those
> as arguments?

sizeof(array) != sizeof(pointer).

> > +   /* Build SAM name (DOMAIN\\user), then translate to UPN
> > +  (user@kerberos.realm). The realm name is returned in
> > +  lower case, but that is fine because in SSPI auth,
> > +  string comparisons are always case-insensitive. */
> 
> Since we're already considering changing things: this is not the comment
> style for this file (though it is otherwise a good comment).

True. Will fix.

> > +   upname = (char*)palloc(upnamesize);
> 
> I don't believe this cast is typically included.

Left over from when this was malloc() before Magnus' first look at it.

> > +   /* Replace domainname with realm name. */
> > +   if (upnamerealmsize > domainnamesize)
> > +   {
> > +   pfree(upname);
> > +   ereport(LOG,
> > +   (errcode(ERRCODE_INVALID_ROLE_SPECIFICATION),
> > +errmsg("realm name too long")));
> > +return STATUS_ERROR;
> > +   }
> > +
> > +   /* Length is now safe. */
> > +   strcpy(domainname, p+1);
> 
> Is this an actual fail state or something born out of convenience?  A
> naive reading of this code doesn't explain why it's forbidden for the
> upn realm to be longer than the domain name.

Because it's copied *into* domainname right there on the last line. 

That said, sizeof(domainname) is MAXPGPATH, which is 1024, so there is
absolutely no chance that the realm could be longer -- it would need an
AD forest at least 16 domains deep.

> > +   /* Replace account name as well (in case UPN != SAM)? */
> > +   if (update_accountname)
> > +   {
> > +   if ((p - upname + 1) > accountnamesize)
> > +   {
> > +   pfree(upname);
> > +   ereport(LOG,
> > +   
> > (errcode(ERRCODE_INVALID_ROLE_SPECIFICATION),
> > +errmsg("translated account name too
> long")));
> > +return STATUS_ERROR;
> > +   }
> > +
> > +   *p = 0;
> > +   strcpy(accountname, upname);
> 
> Same as above.

Yup.

-- 
Christian



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

Re: [HACKERS] Small patch: fix code duplication in heapam.c

[Aleksander Alekseev]

> All that code is hotspot stuff, and turning it into a pile of nested
> procedures doesn't seem like it improves either performance or
> readability.

Your concern regarding performance is understandable. But I should note
that any standard compiler supports inlining these days (probably
this statement is true for at least a decade now). Here is assembly code
of patched version of heap_open produced by GCC 4.8.4 with -O2 flag:

(lldb) disassemble 
postgres`heap_open:
0x497db0 <+0>:   pushq  %rbx
0x497db1 <+1>:   callq  0x497af0  ; relation_open(...)
0x497db6 <+6>:   movq   %rax, %rbx
->  0x497db9 <+9>:   movq   0x30(%rax), %rax
0x497dbd <+13>:  movzbl 0x6f(%rax), %eax
0x497dc1 <+17>:  cmpb   $0x69, %al; 'i', RELKIND_INDEX
0x497dc3 <+19>:  je 0x497dce
0x497dc5 <+21>:  cmpb   $0x63, %al; 'c', COMPOSITE_TYPE
0x497dc7 <+23>:  je 0x497dd7
0x497dc9 <+25>:  movq   %rbx, %rax
0x497dcc <+28>:  popq   %rbx
0x497dcd <+29>:  retq   

As you see heap_open_check_relation procedure was successfully inlined.
Just to be sure that less smart compilers will more likely apply this
optimization I updated patch with 'inline' hints (see attachment).

And even if compiler decide not to apply inlining in this case there is
much more to consider than presence or absence of one 'call' assembly
instruction. For instance compiler may believe that on this concrete
architecture it will be more beneficial to make code shorter so it
would fit to CPU cache better.

Anyway I don't believe that imaginary benchmarks are worth trusting. I
personally don't have much faith in non-imaginary benchmarks either but
it's a different story.

In the same time I'm deeply convinced that this patch will make code
more readable at least because it makes code much shorter:

 src/backend/access/heap/heapam.c | 109 +++---
 1 file changed, 39 insertions(+), 70 deletions(-)

Thus we can see more code on the screen. Besides since there is no code
duplication there is less change that somebody someday will change say
heap_openrv without updating heap_openrv_extended accordingly. 

-- 
Best regards,
Aleksander Alekseev
http://eax.me/
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 34ba385..c55e6a7 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1188,13 +1188,17 @@ try_relation_open(Oid relationId, LOCKMODE lockmode)
 }
 
 /* 
- *		relation_openrv - open any relation specified by a RangeVar
+ *		relation_openrv_extended - open any relation specified by a RangeVar
  *
- *		Same as relation_open, but the relation is specified by a RangeVar.
+ *		Same as relation_openrv, but with an additional missing_ok argument
+ *		allowing a NULL return rather than an error if the relation is not
+ *		found.  (Note that some other causes, such as permissions problems,
+ *		will still result in an ereport.)
  * 
  */
-Relation
-relation_openrv(const RangeVar *relation, LOCKMODE lockmode)
+inline Relation
+relation_openrv_extended(const RangeVar *relation, LOCKMODE lockmode,
+		 bool missing_ok)
 {
 	Oid			relOid;
 
@@ -1213,43 +1217,26 @@ relation_openrv(const RangeVar *relation, LOCKMODE lockmode)
 		AcceptInvalidationMessages();
 
 	/* Look up and lock the appropriate relation using namespace search */
-	relOid = RangeVarGetRelid(relation, lockmode, false);
+	relOid = RangeVarGetRelid(relation, lockmode, missing_ok);
+
+	/* Return NULL on not-found */
+	if (!OidIsValid(relOid))
+		return NULL;
 
 	/* Let relation_open do the rest */
 	return relation_open(relOid, NoLock);
 }
 
 /* 
- *		relation_openrv_extended - open any relation specified by a RangeVar
+ *		relation_openrv - open any relation specified by a RangeVar
  *
- *		Same as relation_openrv, but with an additional missing_ok argument
- *		allowing a NULL return rather than an error if the relation is not
- *		found.  (Note that some other causes, such as permissions problems,
- *		will still result in an ereport.)
+ *		Same as relation_open, but the relation is specified by a RangeVar.
  * 
  */
 Relation
-relation_openrv_extended(const RangeVar *relation, LOCKMODE lockmode,
-		 bool missing_ok)
+relation_openrv(const RangeVar *relation, LOCKMODE lockmode)
 {
-	Oid			relOid;
-
-	/*
-	 * Check for shared-cache-inval messages before trying to open the
-	 * relation.  See comments in relation_openrv().
-	 */
-	if (lockmode != NoLock)
-		AcceptInvalidationMessages();
-
-	/* Look up and lock the appropriate relation using namespace search */
-	relOid = RangeVarGetRelid(relation, lockmode, missing_ok);
-
-	/* Return NULL on not-found */
-	if (!OidIsValid(relOid))
-		return NULL;
-
-	/* Let relation_open do the rest */
-	return relation_open(relOid, NoLock);
+	return relation_openrv_extended(relation, lockmode, false);
 }
 
 /* 
@@ -1275,6 +1262,24 @@ relation_close(Relation relation, 

Re: [HACKERS] dealing with extension dependencies that aren't quite 'e'

[Alvaro Herrera]

Robert Haas wrote:
> On Wed, Mar 23, 2016 at 1:00 PM, Abhijit Menon-Sen  
> wrote:
> > Now, the first part of this works fine. But with the second part, I get
> > a reduce/reduce conflict if I use any_name. Here's an excerpt from the
> > verbose bison output:
> >
> > State 2920
> >
> >   1181 AlterObjectDependsStmt: ALTER TRIGGER name ON any_name DEPENDS . ON 
> > EXTENSION name
> >
> > ON  shift, and go to state 3506
> >
> >
> > State 2921
> >
> >   1165 RenameStmt: ALTER TRIGGER name ON qualified_name RENAME . TO name
> >
> > TO  shift, and go to state 3507
> 
> Yeah, that ain't gonna work.  If the existing ALTER TRIGGER production
> uses "name ON qualified_name", switching to "name ON any_name" for the
> new production is not going to work.  You're going to have to make it
> "name ON qualified_name" and deal with the fallout of that some other
> way.

I helped Abhijit study this problem yesterday.  I found it a bit
annoying that RenameStmt uses RangeVars here (qualified_name) instead of
plain lists like the other productions that use generic objects do.  I
thought one idea to get from under this problem is to change these
productions into using lists too (any_name), but the problem with that
is that qualified_name allows inheritance markers (a * and the ONLY
keyword), which any_name doesn't.  So it'd probably be okay for some
cases, but others are definitely going to need the inheritance markers
in some other way, which would be annoying.

The other problem is that the downstream code uses the RangeVar lookup
callback mechanism to lookup objects and perform permissions checking
before locking.  I imagine the only way to make that work with
lists-in-parser would be to turn the lists into rangevars after the
parser is done ... but that doesn't sound any better.

In other words I think the conclusion here is that we must use
qualified_name in the new production rather than switching the old
production to any_name.

> > I could change ExecAlterObjectDependsStmt() to check if stmt->relation
> > is set, and if so, convert the RangeVar back to a List* in the right
> > format before passing it to get_object_address. That would work fine,
> > but it doesn't seem like a good solution.
> >
> > I could write some get_object_address_rv() wrapper that does essentially
> > the same conversion, but that's only slightly better.
> 
> I might have a view on which of these alternatives is for the best at
> some point in time, but I do not have one now.

I think I would like to see code implement both alternatives to see
which one is least ugly.  Maybe a third idea will manifest itself upon
seeing those.

-- 
Álvaro Herrerahttp://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: [BUGS] [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Robert Haas]

On Thu, Mar 24, 2016 at 11:07 AM, Robbie Harwood  wrote:
> Christian Ullrich  writes:
>
>> Updated patch attached.
>
> I unfortunately don't have windows machines to test this on, but I
> thought it might be helpful to review this anyway since I'm touching
> code in the same general area (GSSAPI).  And as far as I can tell, you
> don't break anything there; master continues to behave as expected.
>
> Some comments inline:
>
>>   pg_SSPI_recvauth(Port *port)
>>   {
>>   int mtype;
>> + int status;
>
> The section of this function for include_realm checking already uses an
> int status return code (retval).  I would expect to see them share a
> variable rather than have both "retval" and "status".
>
>> + status = pg_SSPI_make_upn(accountname, sizeof(accountname),
>> +   domainname, 
>> sizeof(domainname),
>> +   
>> port->hba->upn_username);
>
> This is the only place I see that this function is called.  That being
> the case, why bother with the sizes of parameters?  Why doesn't
> pg_SSPI_make_upn() just call sizeof() by itself rather than taking those
> as arguments?

Well, suppose there is another caller to that function in the future
which wants to use arguments of different lengths.  This actually
seems pretty sensible to me - concern about the length of the buffer
is isolated in the caller, without any spooky action at a distance.

-- 
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] dealing with extension dependencies that aren't quite 'e'

[Robert Haas]

On Wed, Mar 23, 2016 at 1:00 PM, Abhijit Menon-Sen  wrote:
> Now, the first part of this works fine. But with the second part, I get
> a reduce/reduce conflict if I use any_name. Here's an excerpt from the
> verbose bison output:
>
> State 2920
>
>   1181 AlterObjectDependsStmt: ALTER TRIGGER name ON any_name DEPENDS . ON 
> EXTENSION name
>
> ON  shift, and go to state 3506
>
>
> State 2921
>
>   1165 RenameStmt: ALTER TRIGGER name ON qualified_name RENAME . TO name
>
> TO  shift, and go to state 3507

Yeah, that ain't gonna work.  If the existing ALTER TRIGGER production
uses "name ON qualified_name", switching to "name ON any_name" for the
new production is not going to work.  You're going to have to make it
"name ON qualified_name" and deal with the fallout of that some other
way.

> I could change ExecAlterObjectDependsStmt() to check if stmt->relation
> is set, and if so, convert the RangeVar back to a List* in the right
> format before passing it to get_object_address. That would work fine,
> but it doesn't seem like a good solution.
>
> I could write some get_object_address_rv() wrapper that does essentially
> the same conversion, but that's only slightly better.

I might have a view on which of these alternatives is for the best at
some point in time, but I do not have one 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] Reworks of CustomScan serialization/deserialization

[Robert Haas]

On Wed, Mar 23, 2016 at 1:36 PM, Petr Jelinek  wrote:
> Ok, I am happy with it, marked it as ready for committer (it was marked as
> committed although it wasn't committed).

Thanks for fixing the status.   I had forgotten about this thread.

I can't really endorse the naming conventions here.  I mean, we've got
the main extensible nodes stuff in extensible.h, and then we've got
this stuff in custom_node.h (BTW, there is a leftover reference to
custom-node.h).  There's no hint in the naming that this relates to
scans, and why is it extensible in one place and custom in another?

I'm not quite sure how to clean this up.  At a minimum, I think we
should standardize on "custom_scan.h" instead of "custom_node.h".  I
think that would be clearer.  But I'm wondering if we should bite the
bullet and rename everything from "custom" to "extensible" and declare
it all in "extensible.h".

src/backend/nodes/custom_node.c:45: indent with spaces.
+}

-- 
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] NOT EXIST for PREPARE

[Tom Lane]

Vladimir Sitnikov  writes:
> Craig>I really, really doubt you can change this before we do a
> protocol version bump.

> Technically speaking, the idea of using first bytes of statement name
> to convey extra information does not require protocol version bump. It
> can be backward and forward compatible.

> For instance: if statement name starts with __, then skip throwing an
> error if statement with exactly same text and parameter types has
> already been prepared.

If you think that's not a protocol change, you are mistaken.  It
changes a behavior that's specified in the protocol documentation.

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] [WIP] Effective storage of duplicates in B-tree index.

[Alexander Korotkov]

On Thu, Mar 24, 2016 at 5:17 PM, Robert Haas  wrote:

> On Fri, Mar 18, 2016 at 1:19 PM, Anastasia Lubennikova
>  wrote:
> > Please, find the new version of the patch attached. Now it has WAL
> > functionality.
> >
> > Detailed description of the feature you can find in README draft
> > https://goo.gl/50O8Q0
> >
> > This patch is pretty complicated, so I ask everyone, who interested in
> this
> > feature,
> > to help with reviewing and testing it. I will be grateful for any
> feedback.
> > But please, don't complain about code style, it is still work in
> progress.
> >
> > Next things I'm going to do:
> > 1. More debugging and testing. I'm going to attach in next message
> couple of
> > sql scripts for testing.
> > 2. Fix NULLs processing
> > 3. Add a flag into pg_index, that allows to enable/disable compression
> for
> > each particular index.
> > 4. Recheck locking considerations. I tried to write code as less
> invasive as
> > possible, but we need to make sure that algorithm is still correct.
> > 5. Change BTMaxItemSize
> > 6. Bring back microvacuum functionality.
>
> I really like this idea, and the performance results seem impressive,
> but I think we should push this out to 9.7.  A btree patch that didn't
> have WAL support until two and a half weeks into the final CommitFest
> just doesn't seem to me like a good candidate.  First, as a general
> matter, if a patch isn't code-complete at the start of a CommitFest,
> it's reasonable to say that it should be reviewed but not necessarily
> committed in that CommitFest.  This patch has had some review, but I'm
> not sure how deep that review is, and I think it's had no code review
> at all of the WAL logging changes, which were submitted only a week
> ago, well after the CF deadline.  Second, the btree AM is a
> particularly poor place to introduce possibly destabilizing changes.
> Everybody depends on it, all the time, for everything.  And despite
> new tools like amcheck, it's not a particularly easy thing to debug.
>

It's all true.  But:
1) It's a great feature many users dream about.
2) Patch is not very big.
3) Patch doesn't introduce significant infrastructural changes.  It just
change some well-isolated placed.

Let's give it a chance.  I've signed as additional reviewer and I'll do my
best in spotting all possible issues in this patch.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Robbie Harwood]

Christian Ullrich  writes:

> Updated patch attached.

I unfortunately don't have windows machines to test this on, but I
thought it might be helpful to review this anyway since I'm touching
code in the same general area (GSSAPI).  And as far as I can tell, you
don't break anything there; master continues to behave as expected.

Some comments inline:

>   pg_SSPI_recvauth(Port *port)
>   {
>   int mtype;
> + int status;

The section of this function for include_realm checking already uses an
int status return code (retval).  I would expect to see them share a
variable rather than have both "retval" and "status".

> + status = pg_SSPI_make_upn(accountname, sizeof(accountname),
> +   domainname, 
> sizeof(domainname),
> +   
> port->hba->upn_username);

This is the only place I see that this function is called.  That being
the case, why bother with the sizes of parameters?  Why doesn't
pg_SSPI_make_upn() just call sizeof() by itself rather than taking those
as arguments?

> + /* Build SAM name (DOMAIN\\user), then translate to UPN
> +(user@kerberos.realm). The realm name is returned in
> +lower case, but that is fine because in SSPI auth,
> +string comparisons are always case-insensitive. */

Since we're already considering changing things: this is not the comment
style for this file (though it is otherwise a good comment).

> + upname = (char*)palloc(upnamesize);

I don't believe this cast is typically included.

> + /* Replace domainname with realm name. */
> + if (upnamerealmsize > domainnamesize)
> + {
> + pfree(upname);
> + ereport(LOG,
> + (errcode(ERRCODE_INVALID_ROLE_SPECIFICATION),
> +  errmsg("realm name too long")));
> +  return STATUS_ERROR;
> + }
> + 
> + /* Length is now safe. */
> + strcpy(domainname, p+1);

Is this an actual fail state or something born out of convenience?  A
naive reading of this code doesn't explain why it's forbidden for the
upn realm to be longer than the domain name.

> + /* Replace account name as well (in case UPN != SAM)? */
> + if (update_accountname)
> + {
> + if ((p - upname + 1) > accountnamesize)
> + {
> + pfree(upname);
> + ereport(LOG,
> + 
> (errcode(ERRCODE_INVALID_ROLE_SPECIFICATION),
> +  errmsg("translated account name too 
> long")));
> +  return STATUS_ERROR;
> + }
> + 
> + *p = 0;
> + strcpy(accountname, upname);

Same as above.

Minus the few small things above, this looks good to me, assuming it
resolves the issue.

--Robbie


signature.asc
Description: PGP signature

Re: [HACKERS] WIP: Access method extendability

[Alvaro Herrera]

Alvaro Herrera wrote:
> Teodor Sigaev wrote:
> 
> > So, per patch status:
> > create-am
> > ready
> 
> Teodor agreed to me committing this one instead of him; thanks.  I just
> pushed it after some mostly cosmetic adjustments.

This was maybe too laconic.  I actually changed the code a good bit.
Some of the changes:

* pg_dump support got changed to use selectDumpableAccessMethod to
compare the OID to FirstNormalOid rather than embedding "1" in the
SQL query.  This is in line with what we do for other
no-schema-qualified object types.

* I changed DROP ACCESS METHOD to use the generic DropStmt instead of
creating a new production.  I find that most of the DropFooStmt
productions are useless -- we should remove them and instead merge
everything into DropStmt.

* I changed get_object_address to use get_object_address_unqualified,
just like all other object types which use no-schema-qualified object
names.  This removes a screenful of code.  I had to revert get_am_oid to
its original state instead of adding the "amtype" argument.  I added
get_index_am_oid.

* In SGML docs (and psql help) I changed the "TYPE INDEX" literal with
"TYPE access_method_type".

* I moved OCLASS_AM to a more sensible place rather than just stuffing
it at the end of the list

* I removed more than half the includes in the new file amcmds; there
were not necessary.

* I changed this:

+errmsg("function %s must return type 
\"index_am_handler\"",
+   NameListToString(handler_name)));

to this:

+errmsg("function %s must return type \"%s\"",
+   NameListToString(handler_name),
+   "index_am_handler")));

This eases the job of translators: 1) there's no point in presenting the
type name to translate, since it cannot be translated, and 2) all the
equivalent sentences share a single translation instead of needing a
dozen separate translations that only differ in a word that cannot be
translated anyway.  In doing this I noticed that most other uses do not
do this, so I'm going to change them too.


.. Oh crap.  I just noticed I forgot to update a comment in pg_dump's
getAccessMethods.  And we're missing psql tab-complete support for the
new commands.

-- 
Álvaro Herrerahttp://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] NOT EXIST for PREPARE

[Vladimir Sitnikov]

Craig>I really, really doubt you can change this before we do a
protocol version bump.

Technically speaking, the idea of using first bytes of statement name
to convey extra information does not require protocol version bump. It
can be backward and forward compatible.

For instance: if statement name starts with __, then skip throwing an
error if statement with exactly same text and parameter types has
already been prepared.

by "prepare ..." below I mean v3 prepare message, not "prepare sql" command.

prepare __s1(int) select $1; -> prepared
prepare __s1(int) select $1; -> prepared, no error since statement
name starts with __
prepare s1(int) select $1; -> prepared
prepare s1(int) select $1; -> error "statement s1 has already been used"

>doesn't have any kind of capabilities negotiation

Do you think capability negotiation should indeed be at the protocol level?
What's wrong with say "select * from backend_capabilities" at the
connection setup?

Vladimir


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

Re: [HACKERS] NOT EXIST for PREPARE

[Craig Ringer]

On 24 March 2016 at 20:03, Yury Zhuravlev 
wrote:

> I have a big question. What need to do with message protocol?
> If we write name in Parse message we store prepared statement. I see some
> solutions for this problem but all not ideal:
> 1. We can add second char token for parse message. But too serious change.
> 2. We can try add parameter to tail of message. But in tail we have
> variable length array with parameters. 3. Detect prefix of prepared name.
> For example "__". Effects think clear.
>

I really, really doubt you can change this before we do a protocol version
bump. The current protocol is too inflexible and doesn't have any kind of
capabilities negotiation. I don't think any of those options can work.


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

Re: [HACKERS] [PATCH] we have added support for box type in SP-GiST index

[Emre Hasegeli]

> +  * boxtype_spgist.c

The names on the file header need to be changed, too.

> I'll try to explain with two-dimensional example over points. ASCII-art:
>|
>|
> 1  |  2
>|
> ---+-
>|P
>  3 |  4
>|
>|
>
> '+' with 'A' represents a centroid or, other words, point which splits plane
> for 4 quadrants and it strorend in parent node. 1,2,3,4 are labels of
> quadrants, each labeling will be the same for all pictures and all
> centriods, and i will not show them in pictures later to prevent too
> complicated images. Let we add C - child node (and again, it will split
> plane for 4 quads):
>
>
>| |
>+-+---
>  X |B|C
>| |
> ---+-+---
>|P|A
>| |
>|
>|
>
> A and B are points of intersection of lines. So, box PBCAis a bounding box
> for points contained in 3-rd (see labeling above). For example X labeled
> point is not a descendace of child node with centroid  C because it must be
> in branch of 1-st quad of parent node. So, each node (except root) will have
> a limitation in its quadrant. To transfer that limitation the traversalValue
> is used.
>
> To keep formatting I attached a txt file with this fragment.

Thank you for the explanation.  Should we incorporate this with the patch.

>>> +  * Portions Copyright (c) 1996-2015, PostgreSQL Global Development
>>> Group
>>
>> 2016.
>
> fixed

Not on the patch.

> + cmp_double(const double a, const double b)

Does this function necessary?  We can just compare the doubles.

> + boxPointerToRangeBox(BOX *box, RangeBox * rectangle)

The "rectangle" variable in here should be renamed.

> + Assert(is_infinite(b) == 0);

This is failing on my test.  You can reproduce with the script I have sent.

>> Wouldn't it be simpler to palloc and return the value on those functions?
>
> evalRangeBox() initializes part of RectBox, so, it could not palloc its
> result.
> Other methods use the same signature just for consistency.

I couldn't understand it.  evalRangeBox() can palloc and return the
result.  evalRectBox() can return the result palloc'ed by
evalRangeBox().  The caller wouldn't need to palloc anything.

>> Many variables are defined with "const".  I am not sure they are all
>> that doesn't change.  If it applies to the pointer, "out" should also
>> not change in here.  Am I wrong?
>
> No, changes

I now read about "const".  I am sorry for not being experienced in C.
The new version of the patch marks them as "const" by mistake.

I went through all other variables:

> + int r = is_infinite(a);
>
> + double  x = *(double *) a;
> + double  y = *(double *) b;
>
> + spgInnerConsistentIn *in = (spgInnerConsistentIn *) 
> PG_GETARG_POINTER(0);
>
> + spgLeafConsistentIn *in = (spgLeafConsistentIn *) PG_GETARG_POINTER(0);
>
> + BOX*leafBox = DatumGetBoxP(in->leafDatum);

Shouldn't they be "const", too?

>>> +/*
>>> + * Begin. This block evaluates the median of coordinates of boxes
>>> + */
>>
>> I would rather explain what the function does on the function header.
>
> fixed

The "end" part of it is still there.

>> Do we really need to copy the traversalValues on allTheSame case.
>> Wouldn't it work if just the same value is passed for all of them.
>> The search shouldn't continue after allTheSame case.
>
> Seems, yes. spgist tree could contain a locng branches with allTheSame.

Does SP-GiST allows any node under allTheSame to not being allTheSame?
 Not setting traversalValues for allTheSame worked fine with my test.

> + if (in->allTheSame)

Most of the things happening before this check is not used in there.
Shouldn't we move this to the top of the function?

> + out->nodeNumbers = (int *) palloc(sizeof(int) * in->nNodes);

This could go before allTheSame block.


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

Re: [HACKERS] RFC: replace pg_stat_activity.waiting with something more descriptive

[Robert Haas]

On Thu, Mar 24, 2016 at 7:28 AM, Alexander Korotkov
 wrote:
> Since, patch for exposing current wait event information in PGPROC was
> committed, it becomes possible to collect wait event statistics using
> sampling.  Despite I'm not fan of this approach, it is still useful and
> definitely better than nothing.
> In PostgresPro, we actually already had it.  Now, it's too late to include
> something new to 9.6.  This is why I've rework it and publish at github as
> an extension for 9.6: https://github.com/postgrespro/pg_wait_sampling/
> Hopefully, it could be considered as contrib for 9.7.

Spiffy.  That was fast.

I think the sampling approach is going to be best on very large
systems under heavy load; I suspect counting every event is going to
be too expensive - especially once we add more events for things like
block read and client wait.  It is quite possible that we can do other
things when tracing individual sessions or in scenarios where some
performance degradation is OK.  But I like the idea of doing the
sampling thing first - I think that will be very useful.

-- 
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] PostgreSQL 9.6 behavior change with set returning (funct).*

[Tom Lane]

Robert Haas  writes:
> On Wed, Mar 23, 2016 at 6:34 PM, Tom Lane  wrote:
>> A possibly larger problem is that it causes the SRFs to be evaluated
>> before sorting/ordering/limiting.

> I'm not sure I understand quite what the problem is here.

If you have "SELECT srf(x), y FROM ... ORDER BY y LIMIT n", we go
to some lengths as of 9118d03a8 to ensure that the ordering happens
before the SRF is expanded, meaning in particular that the SRF is
expanded only far enough to satisfy the limit.  That is not the case
for SRF-in-FROM, and can't be if for instance there's a GROUP BY
involved.  As-is, the proposed transformation obviously breaks the
semantics if grouping/aggregation/windowing is involved, and my point
is that that's true for ORDER BY/LIMIT as well.  We'd need to fix
things so that the apparent order of performing those steps stays
the same.

I think that's probably doable in most cases by making a sub-select,
but I'm not sure if that works for every case --- in particular, what
do we do with a SRF referenced in ORDER BY or GROUP BY?

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] Proposal: "Causal reads" mode for load balancing reads without stale data

[Robert Haas]

On Thu, Mar 24, 2016 at 2:11 AM, Michael Paquier
 wrote:
> On Wed, Mar 23, 2016 at 11:32 PM, Robert Haas  wrote:
>> Well, I wouldn't go that far.  It seems pretty clear that remote_apply
>> by itself is useful - I can't imagine anybody seriously arguing the
>> contrary, whatever they think of this implementation.  My view,
>> though, is that by itself that's pretty limiting: you can only have
>> one standby, and if that standby falls over then you lose
>> availability.  Causal reads fixes both of those problems - admittedly
>> that requires some knowledge in the application or the pooler, but
>> it's no worse than SSI in that regard.  Still, half a loaf is better
>> than none, and I imagine even just getting remote_apply would make a
>> few people quite happy.
>
> OK, let's do so then, even if causal reads don't get into 9.6 users
> could get advantage of remote_apply on multiple nodes if the N-sync
> patch gets in.
>
> Just looking at 0001.
>
> -remote_write, local, and off.
> +remote_write, remote_apply,
> local, and off.
>  The default, and safe, setting
> I imagine that a run of pgindent would be welcome for such large lines.

I didn't think pgindent touched the docs.  But I agree lines over 80
characters should be wrapped if they're being modified anyway.

> +#define XactCompletionSyncApplyFeedback(xinfo) \
> +   (!!(xinfo & XACT_COMPLETION_SYNC_APPLY_FEEDBACK))
> That's not directly something this patch should take care of, but the
> notation "!!" has better be avoided (see stdbool thread with VS2015).

+1.

> -   SyncRepWaitForLSN(gxact->prepare_end_lsn);
> +   SyncRepWaitForLSN(gxact->prepare_end_lsn, false);
> Isn't it important to ensure that a PREPARE LSN is applied as well on
> the standby with remote_apply? Say if an application prepares a
> transaction, it would commit locally but its LSN may not be applied on
> the standby with this patch. That would be a surprising behavior for
> the user.

You need to wait for COMMIT PREPARED, but I don't see why you need to
wait for PREPARE itself.

-- 
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] 2PC support for pglogical

[Stas Kelvich]

> On 24 Mar 2016, at 17:03, Robert Haas  wrote:
> 
> On Wed, Mar 23, 2016 at 1:44 AM, Craig Ringer  wrote:
>> On 10 March 2016 at 22:50, Stas Kelvich  wrote:
>>> Hi.
>>> 
>>> Here is proof-of-concept version of two phase commit support for logical
>>> replication.
>> 
>> I missed this when you posted it, so sorry for the late response.
>> 
>> I've read through this but not tested it yet. I really appreciate you doing
>> it, it's been on my wishlist/backburner for ages.
>> 
>> On reading through the patch I noticed that there doesn't seem to be any
>> consideration of locking. The prepared xact can still hold strong locks on
>> catalogs. How's that handled? I think Robert's group locking stuff is what
>> we'll want here - for a prepared xact we can join the prepared xact as a
>> group lock member so we inherit its locks. Right now if you try DDL in a
>> prepared xact I suspect it'll break.
> 
> I have grave doubts about that approach.  It's not impossible it could
> be right, but I wouldn't bet the farm on it.
> 

I think all the locking already handled properly by creating dummy backend in 
PGPROC, as it done in usual postgres 2pc implementation.

Just checked DDL with following scenario:

* prepare tx that inserts a row in table on master
* execute DROP TABLE on pglogical slave
* commit prepared on master

Now it behaves as expected — slave blocks DROP TABLE until commit prepared on 
master.

---
Stas Kelvich
Postgres Professional: http://www.postgrespro.com
Russian Postgres 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] [WIP] Effective storage of duplicates in B-tree index.

[Robert Haas]

On Fri, Mar 18, 2016 at 1:19 PM, Anastasia Lubennikova
 wrote:
> Please, find the new version of the patch attached. Now it has WAL
> functionality.
>
> Detailed description of the feature you can find in README draft
> https://goo.gl/50O8Q0
>
> This patch is pretty complicated, so I ask everyone, who interested in this
> feature,
> to help with reviewing and testing it. I will be grateful for any feedback.
> But please, don't complain about code style, it is still work in progress.
>
> Next things I'm going to do:
> 1. More debugging and testing. I'm going to attach in next message couple of
> sql scripts for testing.
> 2. Fix NULLs processing
> 3. Add a flag into pg_index, that allows to enable/disable compression for
> each particular index.
> 4. Recheck locking considerations. I tried to write code as less invasive as
> possible, but we need to make sure that algorithm is still correct.
> 5. Change BTMaxItemSize
> 6. Bring back microvacuum functionality.

I really like this idea, and the performance results seem impressive,
but I think we should push this out to 9.7.  A btree patch that didn't
have WAL support until two and a half weeks into the final CommitFest
just doesn't seem to me like a good candidate.  First, as a general
matter, if a patch isn't code-complete at the start of a CommitFest,
it's reasonable to say that it should be reviewed but not necessarily
committed in that CommitFest.  This patch has had some review, but I'm
not sure how deep that review is, and I think it's had no code review
at all of the WAL logging changes, which were submitted only a week
ago, well after the CF deadline.  Second, the btree AM is a
particularly poor place to introduce possibly destabilizing changes.
Everybody depends on it, all the time, for everything.  And despite
new tools like amcheck, it's not a particularly easy thing to debug.

-- 
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] Small patch: fix code duplication in heapam.c

[Tom Lane]

Aleksander Alekseev  writes:
> I discovered that there is a lot of code duplication in heapam.c.
> In particular relation_openrv and relation_openrv_extended procedures
> and also heap_openrv and heap_openrv_extended procedures are almost the
> same. Here is a patch that fixes this.

As with that other patch to refactor palloc-related stuff, I'm not
convinced that this is an improvement.  All that code is hotspot stuff,
and turning it into a pile of nested procedures doesn't seem like it
improves either performance or readability.

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] 2PC support for pglogical

[Robert Haas]

On Wed, Mar 23, 2016 at 1:44 AM, Craig Ringer  wrote:
> On 10 March 2016 at 22:50, Stas Kelvich  wrote:
>> Hi.
>>
>> Here is proof-of-concept version of two phase commit support for logical
>> replication.
>
> I missed this when you posted it, so sorry for the late response.
>
> I've read through this but not tested it yet. I really appreciate you doing
> it, it's been on my wishlist/backburner for ages.
>
> On reading through the patch I noticed that there doesn't seem to be any
> consideration of locking. The prepared xact can still hold strong locks on
> catalogs. How's that handled? I think Robert's group locking stuff is what
> we'll want here - for a prepared xact we can join the prepared xact as a
> group lock member so we inherit its locks. Right now if you try DDL in a
> prepared xact I suspect it'll break.

I have grave doubts about that approach.  It's not impossible it could
be right, but I wouldn't bet the farm 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] avg,first,last,median in one query

[Konstantin Knizhnik]

Hi, hackers.

I need advice from SQL experts: is there any way in PostgreSQL to 
calculate avg,first,last,median aggregates in one query?

Assume that we have the following table:

create table Securities ("Symbol" varchar, "Date" date, "Time" time, 
"Price" real);


We can simulate median using percentile_disc:

select "Symbol","Date",
avg("Price"),
percentile_disc(0.5) within group (order by "Price")
from Securities
group by "Symbol","Date";

And all other aggregates can be calculated using windows functions:

select distinct "Symbol","Date",
first_value("Price") over (partition by "Symbol","Date" order by 
"Time" rows between unbounded preceding and unbounded following),
last_value("Price") over (partition by "Symbol","Date" order by 
"Time" rows between unbounded preceding and unbounded following),
avg("Price") over (partition by "Symbol","Date" rows between 
unbounded preceding and unbounded following)

from Securities;

I wonder is there are any simpler/efficient alternative to the query above?

But unfortunately it is not possible to calculate median is such way 
because percentile_disc is not compatible with OVER:


ERROR: OVER is not supported for ordered-set aggregate percentile_disc

So is there any chance to calculate all this four aggregates in one 
query without writing some supplementary functions?


Additional question: what is the most efficient way of calculating 
MEDIAN in PostgreSQL?

I found three different approaches:

1. Using CTE:

https://www.periscopedata.com/blog/medians-in-sql.html

2. Using user-defined aggregate function which uses array_appendand so 
materialize all values in memory:


https://wiki.postgresql.org/wiki/Aggregate_Median

3. Using percentile aggregate:

http://blog.jooq.org/2015/01/06/how-to-emulate-the-median-aggregate-function-using-inverse-distribution-functions/


Thanks in advance,

--

Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres 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] PostgreSQL 9.6 behavior change with set returning (funct).*

[Robert Haas]

On Wed, Mar 23, 2016 at 6:34 PM, Tom Lane  wrote:
> I wrote:
>> ... I'd love to
>> toss the entire SRF-in-tlist feature overboard one of these years,
>> both because of semantic issues like these and because a fair amount
>> of code and overhead could be ripped out if it were disallowed.
>> But I don't know how we get to that --- as Merlin says, there's a
>> pretty large amount of application code depending on the feature.
>
> BTW, after further thought I seem to recall some discussion about whether
> we could mechanically transform SRF-in-tlist into a LATERAL query.
> That is, we could make the rewriter or planner convert
>
> SELECT srf1(...), srf2(...)
>   FROM ...
>   WHERE ...
>
> into
>
> SELECT u.s1, u.s2
>   FROM ...,  LATERAL ROWS FROM(srf1(...), srf2(...)) AS u(s1,s2)
>   WHERE ...

I think *that* would be grand.  If I'm not wrong, that's the behavior
that anybody would naively expect.

> (The SRF invocations might be buried inside expressions, but we'd find
> them and convert them anyway.  Also, we'd merge textually-identical SRF
> invocations into a single ROWS FROM entry to avoid multiple evaluation,
> at least for SRFs not marked volatile.)  Having done that, the executor's
> support for SRFs in general expressions could be removed, a significant
> savings.

That is a highly appealing fringe benefit.

> One problem with this is that it only duplicates the current behavior
> in cases where the SRFs all have the same period.  But you could argue
> that the current behavior in cases where they don't is widely considered
> a bug anyway.

I would so argue.  Also, wouldn't this fix the problem that
(srf(blah)).* causes multiple evaluation?  That would be *awesome*.

> A possibly larger problem is that it causes the SRFs to be evaluated
> before sorting/ordering/limiting.

I'm not sure I understand quite what the problem is here.  If there's
a LIMIT, then the proposed transformation would presumably run the SRF
only enough times to fill the limit.  I'm not sure you have any
guarantees about ordering anyway - doesn't that depend on whether the
planner can find a way to produce presorted output via an index scan,
merge join, etc.?

> In view of the discussions that led
> up to 9118d03a8, that could be a fatal objection :-(.  Maybe we could
> get around it with a different transformation, into a two-level query?
> The above sketch doesn't really consider what to do with GROUP BY,
> ORDER BY, etc, but maybe we could push those down into a sub-select
> that becomes the first FROM item.

That seems like it might work.

> Anyway, that's food for thought not something that could realistically
> happen right now.

Ah, bummer.

-- 
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] Small patch: Change calling convention for ShmemInitHash (and fix possible bug)

[Aleksander Alekseev]

Hello

I would like to continue discussion regarding changing calling
convention for ShmemInitHash procedure:

http://www.postgresql.org/message-id/CA+TgmoZm=uowt8a_xasfoogwufeelj861ntadiceopyfehv...@mail.gmail.com

Currently this procedure has two arguments --- init_size and max_size.
But since shared hash tables have fixed size there is little sense to
pass two arguments. In fact currently ShmemInitHash is always called
with init_size == max_size with only one exception, InitLocks procedure
(see lock.c), which I believe is actually a bug.

Patch is attached.

What do you think?

-- 
Best regards,
Aleksander Alekseev
http://eax.me/
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 3d9b8e4..4213c3a 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -495,7 +495,7 @@ pgss_shmem_startup(void)
 	info.hash = pgss_hash_fn;
 	info.match = pgss_match_fn;
 	pgss_hash = ShmemInitHash("pg_stat_statements hash",
-			  pgss_max, pgss_max,
+			  pgss_max,
 			  ,
 			  HASH_ELEM | HASH_FUNCTION | HASH_COMPARE);
 
diff --git a/src/backend/storage/buffer/buf_table.c b/src/backend/storage/buffer/buf_table.c
index 39e8baf..dd5acb7 100644
--- a/src/backend/storage/buffer/buf_table.c
+++ b/src/backend/storage/buffer/buf_table.c
@@ -62,7 +62,7 @@ InitBufTable(int size)
 	info.num_partitions = NUM_BUFFER_PARTITIONS;
 
 	SharedBufHash = ShmemInitHash("Shared Buffer Lookup Table",
-  size, size,
+  size,
   ,
   HASH_ELEM | HASH_BLOBS | HASH_PARTITION);
 }
diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c
index 81506ea..9df73b1 100644
--- a/src/backend/storage/ipc/shmem.c
+++ b/src/backend/storage/ipc/shmem.c
@@ -237,7 +237,7 @@ InitShmemIndex(void)
 	hash_flags = HASH_ELEM;
 
 	ShmemIndex = ShmemInitHash("ShmemIndex",
-			   SHMEM_INDEX_SIZE, SHMEM_INDEX_SIZE,
+			   SHMEM_INDEX_SIZE,
 			   , hash_flags);
 }
 
@@ -250,14 +250,7 @@ InitShmemIndex(void)
  * table at once.  (In practice, all creations are done in the postmaster
  * process; child processes should always be attaching to existing tables.)
  *
- * max_size is the estimated maximum number of hashtable entries.  This is
- * not a hard limit, but the access efficiency will degrade if it is
- * exceeded substantially (since it's used to compute directory size and
- * the hash table buckets will get overfull).
- *
- * init_size is the number of hashtable entries to preallocate.  For a table
- * whose maximum size is certain, this should be equal to max_size; that
- * ensures that no run-time out-of-shared-memory failures can occur.
+ * max_size is the estimated maximum number of hashtable entries.
  *
  * Note: before Postgres 9.0, this function returned NULL for some failure
  * cases.  Now, it always throws error instead, so callers need not check
@@ -265,7 +258,6 @@ InitShmemIndex(void)
  */
 HTAB *
 ShmemInitHash(const char *name, /* table string name for shmem index */
-			  long init_size,	/* initial table size */
 			  long max_size,	/* max size of the table */
 			  HASHCTL *infoP,	/* info about key and bucket size */
 			  int hash_flags)	/* info about infoP */
@@ -299,7 +291,7 @@ ShmemInitHash(const char *name, /* table string name for shmem index */
 	/* Pass location of hashtable header to hash_create */
 	infoP->hctl = (HASHHDR *) location;
 
-	return hash_create(name, init_size, infoP, hash_flags);
+	return hash_create(name, max_size, infoP, hash_flags);
 }
 
 /*
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index b30b7b1..df41b29 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -377,8 +377,7 @@ void
 InitLocks(void)
 {
 	HASHCTL		info;
-	long		init_table_size,
-max_table_size;
+	long 		max_table_size;
 	bool		found;
 
 	/*
@@ -386,7 +385,6 @@ InitLocks(void)
 	 * calculations must agree with LockShmemSize!
 	 */
 	max_table_size = NLOCKENTS();
-	init_table_size = max_table_size / 2;
 
 	/*
 	 * Allocate hash table for LOCK structs.  This stores per-locked-object
@@ -398,14 +396,12 @@ InitLocks(void)
 	info.num_partitions = NUM_LOCK_PARTITIONS;
 
 	LockMethodLockHash = ShmemInitHash("LOCK hash",
-	   init_table_size,
 	   max_table_size,
 	   ,
 	HASH_ELEM | HASH_BLOBS | HASH_PARTITION);
 
 	/* Assume an average of 2 holders per lock */
 	max_table_size *= 2;
-	init_table_size *= 2;
 
 	/*
 	 * Allocate hash table for PROCLOCK structs.  This stores
@@ -417,7 +413,6 @@ InitLocks(void)
 	info.num_partitions = NUM_LOCK_PARTITIONS;
 
 	LockMethodProcLockHash = ShmemInitHash("PROCLOCK hash",
-		   init_table_size,
 		   max_table_size,
 		   ,
  HASH_ELEM | HASH_FUNCTION | HASH_PARTITION);
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 026d2b9..be73a60 100644

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Masahiko Sawada]

On Thu, Mar 24, 2016 at 2:26 PM, Kyotaro HORIGUCHI
 wrote:
> Hello,
>
> At Thu, 24 Mar 2016 13:04:49 +0900, Masahiko Sawada  
> wrote in 

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

[Amit Kapila]

On Thu, Mar 24, 2016 at 8:08 AM, Amit Kapila 
wrote:
>
> On Thu, Mar 24, 2016 at 5:40 AM, Andres Freund  wrote:
> >
> > Have you, in your evaluation of the performance of this patch, done
> > profiles over time? I.e. whether the performance benefits are the
> > immediately, or only after a significant amount of test time? Comparing
> > TPS over time, for both patched/unpatched looks relevant.
> >
>
> I have mainly done it with half-hour read-write tests. What do you want
to observe via smaller tests, sometimes it gives inconsistent data for
read-write tests?
>

I have done some tests on both intel and power m/c (configuration of which
are mentioned at end-of-mail) to see the results at different
time-intervals and it is always showing greater than 50% improvement in
power m/c at 128 client-count and greater than 29% improvement in Intel m/c
at 88 client-count.


Non-default parameters

max_connections = 300
shared_buffers=8GB
min_wal_size=10GB
max_wal_size=15GB
checkpoint_timeout=35min
maintenance_work_mem = 1GB
checkpoint_completion_target = 0.9
wal_buffers = 256MB

pgbench setup

scale factor - 300
used *unlogged* tables : pgbench -i --unlogged-tables -s 300 ..
pgbench -M prepared tpc-b


Results on Intel m/c

client-count - 88

Time (minutes) Base Patch %
5 39978 51858 29.71
10 38169 52195 36.74
20 36992 52173 41.03
30 37042 52149 40.78

Results on power m/c
---
Client-count - 128

Time (minutes) Base Patch %
5 42479 65655 54.55
10 41876 66050 57.72
20 38099 65200 71.13
30 37838 61908 63.61
>
> >
> > Even after changing to scale 500, the performance benefits on this,
> > older 2 socket, machine were minor; even though contention on the
> > ClogControlLock was the second most severe (after ProcArrayLock).
> >
>
> I have tried this patch on mainly 8 socket machine with 300 & 1000 scale
factor.  I am hoping that you have tried this test on unlogged tables and
by the way at what client count, you have seen these results.
>

Do you think in your tests, we don't see increase in performance in your
tests because of m/c difference (sockets/cpu cores) or client-count?


Intel m/c config (lscpu)
-
Architecture:  x86_64
CPU op-mode(s):32-bit, 64-bit
Byte Order:Little Endian
CPU(s):128
On-line CPU(s) list:   0-127
Thread(s) per core:2
Core(s) per socket:8
Socket(s): 8
NUMA node(s):  8
Vendor ID: GenuineIntel
CPU family:6
Model: 47
Model name:Intel(R) Xeon(R) CPU E7- 8830  @ 2.13GHz
Stepping:  2
CPU MHz:   1064.000
BogoMIPS:  4266.62
Virtualization:VT-x
L1d cache: 32K
L1i cache: 32K
L2 cache:  256K
L3 cache:  24576K
NUMA node0 CPU(s): 0,65-71,96-103
NUMA node1 CPU(s): 72-79,104-111
NUMA node2 CPU(s): 80-87,112-119
NUMA node3 CPU(s): 88-95,120-127
NUMA node4 CPU(s): 1-8,33-40
NUMA node5 CPU(s): 9-16,41-48
NUMA node6 CPU(s): 17-24,49-56
NUMA node7 CPU(s): 25-32,57-64

Power m/c config (lscpu)
-
Architecture:  ppc64le
Byte Order:Little Endian
CPU(s):192
On-line CPU(s) list:   0-191
Thread(s) per core:8
Core(s) per socket:1
Socket(s): 24
NUMA node(s):  4
Model: IBM,8286-42A
L1d cache: 64K
L1i cache: 32K
L2 cache:  512K
L3 cache:  8192K
NUMA node0 CPU(s): 0-47
NUMA node1 CPU(s): 48-95
NUMA node2 CPU(s): 96-143
NUMA node3 CPU(s): 144-191

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

Re: [HACKERS] [PROPOSAL] VACUUM Progress Checker.

[Robert Haas]

On Thu, Mar 24, 2016 at 9:01 AM, Robert Haas  wrote:
> On Thu, Mar 24, 2016 at 8:45 AM, Rahila Syed  wrote:
>> Server crash was reported on running  vacuum progress checker view on 32-bit
>> machine.
>> Please find attached a fix for the same.
>>
>> Crash was because 32 bit machine considers int8 as being passed by reference
>> while creating the tuple descriptor. At the time of filling the tuple store,
>> the code (heap_fill_tuple) checks this tuple descriptor before inserting the
>> value into the tuple store. It finds the attribute type pass by reference
>> and hence it treats the value as a pointer when it is not and thus it fails
>> at the time of memcpy.
>>
>> This happens because appropriate conversion function is not employed while
>> storing the value of that particular attribute into the values array before
>> copying it into tuple store.
>>
>> -   values[i+3] =
>> UInt32GetDatum(beentry->st_progress_param[i]);
>> +   values[i+3] =
>> Int64GetDatum(beentry->st_progress_param[i]);
>>
>>
>> Attached patch fixes this.
>
> Uggh, what a stupid mistake on my part.
>
> Committed.  Thanks for the patch.

Oops.  I forgot to credit you in the commit message.  Sorry about that.  :-(

-- 
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] VACUUM Progress Checker.

[Robert Haas]

On Thu, Mar 24, 2016 at 8:45 AM, Rahila Syed  wrote:
> Server crash was reported on running  vacuum progress checker view on 32-bit
> machine.
> Please find attached a fix for the same.
>
> Crash was because 32 bit machine considers int8 as being passed by reference
> while creating the tuple descriptor. At the time of filling the tuple store,
> the code (heap_fill_tuple) checks this tuple descriptor before inserting the
> value into the tuple store. It finds the attribute type pass by reference
> and hence it treats the value as a pointer when it is not and thus it fails
> at the time of memcpy.
>
> This happens because appropriate conversion function is not employed while
> storing the value of that particular attribute into the values array before
> copying it into tuple store.
>
> -   values[i+3] =
> UInt32GetDatum(beentry->st_progress_param[i]);
> +   values[i+3] =
> Int64GetDatum(beentry->st_progress_param[i]);
>
>
> Attached patch fixes this.

Uggh, what a stupid mistake on my part.

Committed.  Thanks for the patch.

-- 
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] VACUUM Progress Checker.

[Rahila Syed]

Hello,

Server crash was reported on running  vacuum progress checker view on
32-bit machine.
Please find attached a fix for the same.

Crash was because 32 bit machine considers int8 as being passed by
reference while creating the tuple descriptor. At the time of filling the
tuple store, the code (heap_fill_tuple) checks this tuple descriptor before
inserting the value into the tuple store. It finds the attribute type pass
by reference and hence it treats the value as a pointer when it is not and
thus it fails at the time of memcpy.

This happens because appropriate conversion function is not employed while
storing the value of that particular attribute into the values array before
copying it into tuple store.

-   values[i+3] =
UInt32GetDatum(beentry->st_progress_param[i]);
+   values[i+3] =
Int64GetDatum(beentry->st_progress_param[i]);


Attached patch fixes this.

Thank you,
Rahila Syed

On Wed, Mar 16, 2016 at 11:30 PM, Robert Haas  wrote:

> On Wed, Mar 16, 2016 at 6:44 AM, Rahila Syed 
> wrote:
> >>Sorta.  Committed after renaming what you called heap blocks vacuumed
> >>back to heap blocks scanned, adding heap blocks vacuumed, removing the
> >>overall progress meter which I don't believe will be anything close to
> >>accurate, fixing some stylistic stuff, arranging to update multiple
> >>counters automatically where it could otherwise produce confusion,
> >>moving the new view near similar ones in the file, reformatting it to
> >>follow the style of the rest of the file, exposing the counter
> >>#defines via a header file in case extensions want to use them, and
> >>overhauling and substantially expanding the documentation
> >
> > We have following lines,
> >
> > /* report that everything is scanned and vacuumed */
> > pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED,
> > blkno);
> > pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED,
> > blkno);
> >
> >
> > which appear before final vacuum cycle happens for any remaining dead
> tuples
> > which may span few pages if I am not mistaken.
> >
> > IMO, reporting final count of heap_blks_scanned is correct here, but
> > reporting final heap_blks_vacuumed can happen after the final VACUUM
> cycle
> > for more accuracy.
>
> You are quite right.  Good catch.  Fixed that, and applied Vinayak's
> patch too, and fixed another mistake I saw while I was at it.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 0f6f891..64c4cc4 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -612,7 +612,7 @@ pg_stat_get_progress_info(PG_FUNCTION_ARGS)
 		{
 			values[2] = ObjectIdGetDatum(beentry->st_progress_command_target);
 			for(i = 0; i < PGSTAT_NUM_PROGRESS_PARAM; i++)
-values[i+3] = UInt32GetDatum(beentry->st_progress_param[i]);
+values[i+3] = Int64GetDatum(beentry->st_progress_param[i]);
 		}
 		else
 		{

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

Re: [HACKERS] WIP: Access method extendability

[Alvaro Herrera]

Teodor Sigaev wrote:

> So, per patch status:
> create-am
>   ready

Teodor agreed to me committing this one instead of him; thanks.  I just
pushed it after some mostly cosmetic adjustments.

I pass the baton back to Teodor for the remaining patches in this
series.

Thanks,

-- 
Álvaro Herrerahttp://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] NOT EXIST for PREPARE

[Yury Zhuravlev]

I have a big question. What need to do with message protocol?
If we write name in Parse message we store prepared statement. 
I see some solutions for this problem but all not ideal:
1. We can add second char token for parse message. But too serious change. 
2. We can try add parameter to tail of message. But in tail we have 
variable length array with parameters. 
3. Detect prefix of prepared name. For example "__". Effects think clear.


I would like to know the community opinion on this matter.

Thanks.

--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres 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] RFC: replace pg_stat_activity.waiting with something more descriptive

[Alexander Korotkov]

Hi!

Since, patch for exposing current wait event information in PGPROC was
committed, it becomes possible to collect wait event statistics using
sampling.  Despite I'm not fan of this approach, it is still useful and
definitely better than nothing.
In PostgresPro, we actually already had it.  Now, it's too late to include
something new to 9.6.  This is why I've rework it and publish at github as
an extension for 9.6: https://github.com/postgrespro/pg_wait_sampling/
Hopefully, it could be considered as contrib for 9.7.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Alter or rename enum value

[Matthias Kurz]

On 9 March 2016 at 20:19, Matthias Kurz  wrote:

> Besides not being able to rename enum values there are two other
> limitations regarding enums which would be nice to get finally fixed:
>
> 1) There is also no possibility to drop a value.
>
> 2) Quoting the docs (
> http://www.postgresql.org/docs/9.5/static/sql-altertype.html):
> "ALTER TYPE ... ADD VALUE (the form that adds a new value to an enum type)
> cannot be executed inside a transaction block." Example:
> # CREATE TYPE bogus AS ENUM('good');
> CREATE TYPE
> # BEGIN;
> BEGIN
> # ALTER TYPE bogus ADD VALUE 'bad';
> ERROR:  ALTER TYPE ... ADD cannot run inside a transaction block
>
> To summarize it:
> For enums to finally be really usable it would nice if we would have (or
> similiar):
> ALTER TYPE name DROP VALUE [ IF EXISTS ] enum_value
> and
> ALTER TYPE name RENAME VALUE [ IF EXISTS ] old_enum_value_name TO
> new_enum_value_name
>
> And all of the operations (adding, renaming, dropping) should also work
> when done within a new transaction on an enum that existed before that
> transaction.
>
> I did some digging and maybe following commits are useful in this context:
> 7b90469b71761d240bf5efe3ad5bbd228429278e
> c9e2e2db5c2090a880028fd8c1debff474640f50
>
> Also there are these discussions where some of the messages contain some
> useful information:
>
> http://www.postgresql.org/message-id/29f36c7c98ab09499b1a209d48eaa615b7653db...@mail2a.alliedtesting.com
> http://www.postgresql.org/message-id/50324f26.3090...@dunslane.net
>
> http://www.postgresql.org/message-id/20130819122938.gb8...@alap2.anarazel.de
>
> Also have a look at this workaround:
> http://en.dklab.ru/lib/dklab_postgresql_enum/
>
> How high is the chance that given the above information someone will
> tackle these 3 issues/requests in the near future? It seems there were some
> internal chances since the introduction of enums in 8.x so maybe this
> changes wouldn't be that disruptive anymore?
>
> Regards,
> Matthias
>
> On 9 March 2016 at 18:13, Tom Lane  wrote:
>
>> Andrew Dunstan  writes:
>> > On 03/09/2016 11:07 AM, Tom Lane wrote:
>> >> I have a vague recollection that we discussed this at the time the enum
>> >> stuff went in, and there are concurrency issues?  Don't recall details
>> >> though.
>>
>> > Rings a vague bell, but should it be any worse than adding new labels?
>>
>> I think what I was recalling is the hazards discussed in the comments for
>> RenumberEnumType.  However, the problem there is that a backend could make
>> inconsistent ordering decisions due to seeing two different pg_enum rows
>> under different snapshots.  Updating a single row to change its name
>> doesn't seem to have a comparable hazard, and it wouldn't affect ordering
>> anyway.  So it's probably no worse than any other object-rename situation.
>>
>> regards, tom lane
>>
>
>
Is there a way or a procedure we can go through to make the these ALTER
TYPE enhancements a higher priority? How do you choose which
features/enhancements to implement (next)?

Re: [HACKERS] Relation extension scalability

[Dilip Kumar]

On Thu, Mar 24, 2016 at 6:13 PM, Amit Kapila 
wrote:

>
> 1.
> +GetPageWithFreeSpaceUsingOldPage(Relation rel, BlockNumber oldPage,
> + Size spaceNeeded)
> {
> ..
> + /*
> + * If fsm_set_and_search found a suitable new block, return that.
> + * Otherwise, search as usual.
> + */
> ..
> }
>
> In the above comment, you are referring wrong function.
>

Fixed

>
> 2.
> +static int
> +fsm_search_from_addr(Relation rel, FSMAddress addr, uint8 minValue)
> +{
> + Buffer buf;
> + int newslot = -1;
> +
> + buf = fsm_readbuf(rel, addr, true);
> + LockBuffer(buf, BUFFER_LOCK_SHARE);
> +
> + if (minValue != 0)
> + {
> + /* Search while we still hold the lock */
> + newslot = fsm_search_avail(buf, minValue,
> +   addr.level == FSM_BOTTOM_LEVEL,
> +   false);
>
> In this new API, I don't understand why we need minValue != 0 check,
> basically if user of API doesn't want to search for space > 0, then what is
> the need of calling this API?  I think this API should use Assert for
> minValue!=0 unless you see reason for not doing so.
>
>
Agree, it should be assert.


> >
> > GetNearestPageWithFreeSpace? (although not sure that's accurate
> description, maybe Nearby would be better)
> >
>
> Better than what is used in patch.
>
> Yet another possibility could be to call it as
> GetPageWithFreeSpaceExtended and call it from GetPageWithFreeSpace with
> value of oldPage as InvalidBlockNumber.
>

Yes I like this.. Changed the same.


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


multi_extend_v13.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] Relation extension scalability

[Amit Kapila]

On Thu, Mar 24, 2016 at 1:48 PM, Petr Jelinek  wrote:
>
> On 24/03/16 07:04, Dilip Kumar wrote:
>>
>>
>> On Thu, Mar 24, 2016 at 10:44 AM, Robert Haas > > wrote:
>>
>> On Wed, Mar 23, 2016 at 9:43 PM, Amit Kapila
>> > wrote:
>> > RecordAndGetPageWithFreeSpace() tries to search from the oldPage
passed to
>> > it, rather than from top, so even if  RecordPageWithFreeSpace()
doesn't
>> > update till root, it will be able to search the newly added page.
I agree
>> > with whatever you have said in another mail that we should
introduce a new
>> > API to do a more targeted search for such cases.
>>
>> OK, let's do that, then.
>>
>>
>> Ok, I have added new API which just find the free block from and start
>> search from last given page.
>>
>> 1. I have named the new function as GetPageWithFreeSpaceUsingOldPage, I
>> don't like this name, but I could not come up with any better, Please
>> suggest one.
>>
>

1.
+GetPageWithFreeSpaceUsingOldPage(Relation rel, BlockNumber oldPage,
+ Size spaceNeeded)
{
..
+ /*
+ * If fsm_set_and_search found a suitable new block, return that.
+ * Otherwise, search as usual.
+ */
..
}

In the above comment, you are referring wrong function.

2.
+static int
+fsm_search_from_addr(Relation rel, FSMAddress addr, uint8 minValue)
+{
+ Buffer buf;
+ int newslot = -1;
+
+ buf = fsm_readbuf(rel, addr, true);
+ LockBuffer(buf, BUFFER_LOCK_SHARE);
+
+ if (minValue != 0)
+ {
+ /* Search while we still hold the lock */
+ newslot = fsm_search_avail(buf, minValue,
+   addr.level == FSM_BOTTOM_LEVEL,
+   false);

In this new API, I don't understand why we need minValue != 0 check,
basically if user of API doesn't want to search for space > 0, then what is
the need of calling this API?  I think this API should use Assert for
minValue!=0 unless you see reason for not doing so.

>
> GetNearestPageWithFreeSpace? (although not sure that's accurate
description, maybe Nearby would be better)
>

Better than what is used in patch.

Yet another possibility could be to call it as GetPageWithFreeSpaceExtended
and call it from GetPageWithFreeSpace with value of oldPage
as InvalidBlockNumber.


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

Re: [HACKERS] NOT EXIST for PREPARE

[Yury Zhuravlev]

Michael Meskes wrote:

While ecpg may not be the choice for new applications, there are a lot
of legacy applications out there that need ecpg to be migrated to
PostgreSQL.


2016 is a good time to rewrite them. ;)
I think Postgres will be more likely if it would be a little less concerned 
about compatibility IMHO.

But I will not insist. My problem with ecpg I decided.

Thanks. 
--

Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres 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] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Thu, Mar 24, 2016 at 2:26 PM, Kyotaro HORIGUCHI
 wrote:
> Hello,
>
> At Thu, 24 Mar 2016 13:04:49 +0900, Masahiko Sawada  
> wrote in 

Re: [HACKERS] Odd system-column handling in postgres_fdw join pushdown patch

[Ashutosh Bapat]

On Thu, Mar 24, 2016 at 9:31 AM, Etsuro Fujita 
wrote:

> On 2016/03/23 13:44, Ashutosh Bapat wrote:
>
>> An FDW can choose not to use those functions, so I don't see a
>> connection between scan list having simple Vars and existence of those
>> functions (actually a single one). But having those function would
>> minimize the code that each FDW has to write, in case they want those
>> functions. E.g. we have to translate Var::varno to tableoid in case
>> that's requested by pulling RTE and then getting oid out from there. If
>> that functionality is available in the core, 1. the code is not
>> duplicated 2. every FDW will get the same tableoid. Similarly for the
>> other columns.
>>
>
> OK.  Then, I'd like to propose a function that would create interger Lists
> of indexes of tableoids, xids and cids plus


Ok,


> an OID List of these tableoids,


I didn't get this.


> in a given fdw_scan_tlist, on the assumption that each expression in the
> fdw_scan_tlist is a simple Var.


I guess this is Ok. In fact, at least for now an expression involving any
of those columns is not pushable to the foreign server, as the expression
can not be evaluated there. So, if we come across such a case in further
pushdowns, we will need to have a different solution for pushing down such
target lists.


> I'd also like to propose another function that would fill system columns
> using these Lists when creating a scan tuple.
>
> Ok.

I had imagined that the code to extract the above lists and filling the
values in scan tuple will be in FDW. We only provide a function to supply
those values. But what you propose might actually be much practical.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: [HACKERS] Postgres_fdw join pushdown - getting server crash in left outer join of three table

[Ashutosh Bapat]

> > Thanks for the report and the testing.  I have committed the patch.
>
>
Thanks.


> Cool, I have refreshed the wiki page for open items accordingly.
>

Thanks.


-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: [HACKERS] Combining Aggregates

[David Rowley]

On 21 January 2016 at 08:06, Robert Haas  wrote:
> I re-reviewed this and have committed most of it with only minor
> kibitizing.  A few notes:

I realised today that the combinefunc is rather undocumented. I've
attached a patch which aims to fix this.

Comments are welcome.

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


add_combinefunc_documents.patch
Description: Binary data

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

[HACKERS] Small patch: fix code duplication in heapam.c

[Aleksander Alekseev]

Hello

I discovered that there is a lot of code duplication in heapam.c.
In particular relation_openrv and relation_openrv_extended procedures
and also heap_openrv and heap_openrv_extended procedures are almost the
same. Here is a patch that fixes this.

-- 
Best regards,
Aleksander Alekseev
http://eax.me/
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 34ba385..e9db6ad 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1188,13 +1188,17 @@ try_relation_open(Oid relationId, LOCKMODE lockmode)
 }
 
 /* 
- *		relation_openrv - open any relation specified by a RangeVar
+ *		relation_openrv_extended - open any relation specified by a RangeVar
  *
- *		Same as relation_open, but the relation is specified by a RangeVar.
+ *		Same as relation_openrv, but with an additional missing_ok argument
+ *		allowing a NULL return rather than an error if the relation is not
+ *		found.  (Note that some other causes, such as permissions problems,
+ *		will still result in an ereport.)
  * 
  */
 Relation
-relation_openrv(const RangeVar *relation, LOCKMODE lockmode)
+relation_openrv_extended(const RangeVar *relation, LOCKMODE lockmode,
+		 bool missing_ok)
 {
 	Oid			relOid;
 
@@ -1213,43 +1217,26 @@ relation_openrv(const RangeVar *relation, LOCKMODE lockmode)
 		AcceptInvalidationMessages();
 
 	/* Look up and lock the appropriate relation using namespace search */
-	relOid = RangeVarGetRelid(relation, lockmode, false);
+	relOid = RangeVarGetRelid(relation, lockmode, missing_ok);
+
+	/* Return NULL on not-found */
+	if (!OidIsValid(relOid))
+		return NULL;
 
 	/* Let relation_open do the rest */
 	return relation_open(relOid, NoLock);
 }
 
 /* 
- *		relation_openrv_extended - open any relation specified by a RangeVar
+ *		relation_openrv - open any relation specified by a RangeVar
  *
- *		Same as relation_openrv, but with an additional missing_ok argument
- *		allowing a NULL return rather than an error if the relation is not
- *		found.  (Note that some other causes, such as permissions problems,
- *		will still result in an ereport.)
+ *		Same as relation_open, but the relation is specified by a RangeVar.
  * 
  */
 Relation
-relation_openrv_extended(const RangeVar *relation, LOCKMODE lockmode,
-		 bool missing_ok)
+relation_openrv(const RangeVar *relation, LOCKMODE lockmode)
 {
-	Oid			relOid;
-
-	/*
-	 * Check for shared-cache-inval messages before trying to open the
-	 * relation.  See comments in relation_openrv().
-	 */
-	if (lockmode != NoLock)
-		AcceptInvalidationMessages();
-
-	/* Look up and lock the appropriate relation using namespace search */
-	relOid = RangeVarGetRelid(relation, lockmode, missing_ok);
-
-	/* Return NULL on not-found */
-	if (!OidIsValid(relOid))
-		return NULL;
-
-	/* Let relation_open do the rest */
-	return relation_open(relOid, NoLock);
+	return relation_openrv_extended(relation, lockmode, false);
 }
 
 /* 
@@ -1275,6 +1262,24 @@ relation_close(Relation relation, LOCKMODE lockmode)
 		UnlockRelationId(, lockmode);
 }
 
+/*
+ * Check Relation after opening. Internal procedure used by heap_open and
+ * heap_openrv_* procedures.
+ */
+static void
+heap_open_check_relation(Relation r)
+{
+	if (r->rd_rel->relkind == RELKIND_INDEX)
+		ereport(ERROR,
+(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("\"%s\" is an index",
+		RelationGetRelationName(r;
+	else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
+		ereport(ERROR,
+(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("\"%s\" is a composite type",
+		RelationGetRelationName(r;
+}
 
 /* 
  *		heap_open - open a heap relation by relation OID
@@ -1291,17 +1296,7 @@ heap_open(Oid relationId, LOCKMODE lockmode)
 	Relation	r;
 
 	r = relation_open(relationId, lockmode);
-
-	if (r->rd_rel->relkind == RELKIND_INDEX)
-		ereport(ERROR,
-(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
-		RelationGetRelationName(r;
-	else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
-		ereport(ERROR,
-(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
-		RelationGetRelationName(r;
+	heap_open_check_relation(r);
 
 	return r;
 }
@@ -1316,22 +1311,7 @@ heap_open(Oid relationId, LOCKMODE lockmode)
 Relation
 heap_openrv(const RangeVar *relation, LOCKMODE lockmode)
 {
-	Relation	r;
-
-	r = relation_openrv(relation, lockmode);
-
-	if (r->rd_rel->relkind == RELKIND_INDEX)
-		ereport(ERROR,
-(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
-		RelationGetRelationName(r;
-	else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
-		ereport(ERROR,
-(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
-		RelationGetRelationName(r;
-
-	return r;
+	return heap_openrv_extended(relation, lockmode, false);
 }
 
 /* 

Re: [HACKERS] Relation extension scalability

[Petr Jelinek]

On 24/03/16 07:04, Dilip Kumar wrote:


On Thu, Mar 24, 2016 at 10:44 AM, Robert Haas > wrote:

On Wed, Mar 23, 2016 at 9:43 PM, Amit Kapila
> wrote:
> RecordAndGetPageWithFreeSpace() tries to search from the oldPage passed to
> it, rather than from top, so even if  RecordPageWithFreeSpace() doesn't
> update till root, it will be able to search the newly added page.  I agree
> with whatever you have said in another mail that we should introduce a new
> API to do a more targeted search for such cases.

OK, let's do that, then.


Ok, I have added new API which just find the free block from and start
search from last given page.

1. I have named the new function as GetPageWithFreeSpaceUsingOldPage, I
don't like this name, but I could not come up with any better, Please
suggest one.



GetNearestPageWithFreeSpace? (although not sure that's accurate 
description, maybe Nearby would be better)


--
  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] NOT EXIST for PREPARE

[Michael Meskes]

> I want to understand the situation. You may want to make the build
> ecpg 
> optional. Personally, I want to.

You lost me here, sorry. What exactly do you want to do? 

While ecpg may not be the choice for new applications, there are a lot
of legacy applications out there that need ecpg to be migrated to
PostgreSQL. So I think, completely removing it is out of the question.

An optional build does not change a thing because it still has to
compile et al. If you mean you'd like to decouple it from the backend
build, that one is difficult because the parser is supposed to accept
exactly the same SQL. And we spend quite a bit of effort to make it
auto-build from the backend parser to make sure we don't lose any
changes. 

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL




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

Re: [HACKERS] snapshot too old, configured by time

[Michael Paquier]

On Tue, Mar 22, 2016 at 5:05 AM, Kevin Grittner  wrote:
> Thanks to all for the feedback; I will try to respond later this
> week.  First I'm trying to get my reviews for other patches posted.

I have been looking at 4a, the test module, and things are looking
good IMO. Something that I think would be adapted would be to define
the options for isolation tests in a variable, like ISOLATION_OPTS to
allow MSVC scripts to fetch those option values more easily.

+submake-test_decoding:
+   $(MAKE) -C $(top_builddir)/src/test/modules/snapshot_too_old
The target name here is incorrect. This should refer to snapshot_too_old.

Regarding the main patch:
+old_snapshot_threshold configuration
parameter
snapshot_valid_limit?

page = BufferGetPage(buf);
+   TestForOldSnapshot(scan->xs_snapshot, rel, page);
This is a sequence repeated many times in this patch, a new routine,
say BufferGetPageExtended with a uint8 flag, one flag being used to
test old snapshots would be more adapted. But this would require
creating a header dependency between the buffer manager and
SnapshotData.. Or more simply we may want a common code path when
fetching a page that a backend is going to use to fetch tuples. I am
afraid of TestForOldSnapshot() being something that could be easily
forgotten in code paths introduced in future patches...

+   if (whenTaken < 0)
+   {
+   elog(DEBUG1,
+"MaintainOldSnapshotTimeMapping called with negative
whenTaken = %ld",
+(long) whenTaken);
+   return;
+   }
+   if (!TransactionIdIsNormal(xmin))
+   {
+   elog(DEBUG1,
+"MaintainOldSnapshotTimeMapping called with xmin = %lu",
+(unsigned long) xmin);
+   return;
+   }
Material for two assertions?
-- 
Michael


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

[HACKERS] MSVC scripts missing some isolation/regression tests

[Michael Paquier]

Hi all,

While looking at the buildfarm and reviewing the old snapshot patch, I
noticed that isolation tests cannot run with MSVC scripts all the
time. Take for example that:
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=currawong=2016-03-24%2004%3A55%3A21

Which leads to the following funny thing in contrib-install-check-C:

Checking test_decoding
(using postmaster on localhost, default port)
== dropping database "contrib_regression" ==
DROP DATABASE
== creating database "contrib_regression" ==
CREATE DATABASE
ALTER DATABASE
== running regression test queries==

=
 All 0 tests passed.
=
The origin of this problem is that those vcregress.pl is not able to
fetch any test names, and creates an temporary instance for nothing.

In order to get to a cleaner situation, I propose the following:
- Fix the Makefile of test_decoding to define ISOLATION_OPTS and
REGRESS_OPTS so as the msvc scripts can fetch the options to run with
isolation_tester correctly, and switch REGRESSCHECKS to REGRESS, then
ISOLATIONCHECKS to ISOLATION.
- Extend vcregress.pl so as it is able to detect ISOLATION[_OPTS] to list tests
- Add more cruft in subdirch...@vcregress.pl to guess if pg_regress,
isolation_tester, or both are needed
For the purpose of test_decoding, we need to be able to run both
pg_regress and isolation_tester.

Now, not testing those things has little impact perhaps... But I'd
rather fix those problems, and at least making the Makefile of
test_decoding more consistent does not sound bad to me. Thoughts?

Regards,
-- 
Michael


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

Re: [HACKERS] Proposal: "Causal reads" mode for load balancing reads without stale data

[Michael Paquier]

On Wed, Mar 23, 2016 at 11:32 PM, Robert Haas  wrote:
> Well, I wouldn't go that far.  It seems pretty clear that remote_apply
> by itself is useful - I can't imagine anybody seriously arguing the
> contrary, whatever they think of this implementation.  My view,
> though, is that by itself that's pretty limiting: you can only have
> one standby, and if that standby falls over then you lose
> availability.  Causal reads fixes both of those problems - admittedly
> that requires some knowledge in the application or the pooler, but
> it's no worse than SSI in that regard.  Still, half a loaf is better
> than none, and I imagine even just getting remote_apply would make a
> few people quite happy.

OK, let's do so then, even if causal reads don't get into 9.6 users
could get advantage of remote_apply on multiple nodes if the N-sync
patch gets in.

Just looking at 0001.

-remote_write, local, and off.
+remote_write, remote_apply,
local, and off.
 The default, and safe, setting
I imagine that a run of pgindent would be welcome for such large lines.

+#define XactCompletionSyncApplyFeedback(xinfo) \
+   (!!(xinfo & XACT_COMPLETION_SYNC_APPLY_FEEDBACK))
That's not directly something this patch should take care of, but the
notation "!!" has better be avoided (see stdbool thread with VS2015).

-   SyncRepWaitForLSN(gxact->prepare_end_lsn);
+   SyncRepWaitForLSN(gxact->prepare_end_lsn, false);
Isn't it important to ensure that a PREPARE LSN is applied as well on
the standby with remote_apply? Say if an application prepares a
transaction, it would commit locally but its LSN may not be applied on
the standby with this patch. That would be a surprising behavior for
the user.

(not commenting on the latch and SIGUSR2 handling, you are still
working on it per your last update).
-- 
Michael


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