Re: [HACKERS] More race conditions in logical replication

[Noah Misch]

On Wed, Jul 12, 2017 at 06:48:28PM -0400, Alvaro Herrera wrote:
> Petr Jelinek wrote:
> 
> > So best idea I could come up with is to make use of the new condition
> > variable API. That lets us wait for variable which can be set per slot.
> 
> Here's a cleaned up version of that patch, which I intend to get in the
> tree tomorrow.  I verified that this allows the subscription tests to
> pass with Tom's sleep additions.

This PostgreSQL 10 open item is past due for your status update.  Kindly send
a status update within 24 hours, and include a date for your subsequent status
update.  Refer to the policy on open item ownership:
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com


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

Re: [HACKERS] hash index on unlogged tables doesn't behave as expected

[Ashutosh Sharma]

On Sat, Jul 15, 2017 at 8:20 AM, Amit Kapila  wrote:
> On Fri, Jul 14, 2017 at 6:02 PM, Michael Paquier
>  wrote:
>> (catching up finally with this thread)
>>
>> On Mon, Jul 10, 2017 at 11:57 AM, Kyotaro HORIGUCHI
>>  wrote:
>>> At Mon, 10 Jul 2017 14:58:13 +0530, Amit Kapila  
>>> wrote in 
>>> 

Re: [HACKERS] hash index on unlogged tables doesn't behave as expected

[Amit Kapila]

On Fri, Jul 14, 2017 at 6:02 PM, Michael Paquier
 wrote:
> (catching up finally with this thread)
>
> On Mon, Jul 10, 2017 at 11:57 AM, Kyotaro HORIGUCHI
>  wrote:
>> At Mon, 10 Jul 2017 14:58:13 +0530, Amit Kapila  
>> wrote in 

Re: [HACKERS] New partitioning - some feedback

[Robert Haas]

On Mon, Jul 10, 2017 at 5:46 PM, David Fetter  wrote:
> With utmost respect, it's less messy than adding '!' to the already
> way too random and mysterious syntax of psql's \ commands.  What
> should '\det!' mean?  What about '\dT!'?

Since \det lists foreign tables, \det! would list foreign tables even
if they are partitions.  Plain \det would show only the ones that are
not partitions.

\dT! wouldn't be meaningful, since \dT lists data types and data types
can't be partitions.  If you're trying to conjure up a rule that every
\d command must accept the same set of modifiers, a quick
look at the output of \? and a little experimentation will quickly
show you that neither S nor + apply to all command types, so I see no
reason why that would need to be true for a new modifier either.

TBH, I think we should just leave this well enough alone.  We're
post-beta2 now, there's no clear consensus on what to do here, and
there will be very little opportunity for users to give us feedback if
we stick a change into an August beta3 before a September final
release.

-- 
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] Pluggable storage

[Robert Haas]

On Fri, Jul 14, 2017 at 8:35 AM, Haribabu Kommi
 wrote:
> To replace tuple with slot, I took trigger and SPI calls as the first step
> in modifying
> from tuple to slot, Here I attached a WIP patch. The notable changes are,
>
> 1. Replace most of the HeapTuple with Slot in SPI interface functions.
> 2. In SPITupleTable, Instead of HeapTuple, it is changed to TupleTableSlot.
> But this change may not be a proper approach, because a duplicate copy of
> TupleTableSlot is generated and stored.
> 3. Changed all trigger interfaces to accept TupleTableSlot Instead of
> HeapTuple.
> 4. ItemPointerData is added as a member to the TupleTableSlot structure.
> 5. Modified the ExecInsert and others to work directly on TupleTableSlot
> instead
> of tuple(not completely).

What problem are you trying to solve with these changes?  I'm not
saying that it's a bad idea, but I think you should spell out the
motivation a little more explicitly.

I think performance is likely to be a key concern here.  Maybe these
interfaces are Just Better with TupleTableSlots and the patch is a win
regardless of pluggable storage -- but if the reverse turns out to be
true, and this slows things down in cases that anyone cares about, I
think that's going to be a problem.

-- 
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] Pluggable storage

[Robert Haas]

On Thu, Jun 22, 2017 at 4:27 PM, Tomas Vondra
 wrote:
> Can you elaborate a bit more about this TID bit pattern issues? I do
> remember that not all TIDs are valid due to safeguards on individual fields,
> like for example
>
> Assert(iptr->ip_posid < (1 << MaxHeapTuplesPerPageBits))
>
> But perhaps there are some other issues?

I think the other issues that I know about have largely already been
mentioned by others, but since this question was addressed to me:

1. TID bitmaps assume that the page and offset parts of the TID
contain just those things.  As Tom pointed out, tidbitmap.c isn't cool
with TIDs that have ip_posid out of range.  More broadly, we assume
lossification is a sensible way of keeping a TID bitmap down to a
reasonable size without losing too much efficiency, and that sorting
tuple references by the block ID is likely to produce a sequential I/O
pattern.  Those things won't necessarily be true if TIDs are treated
as opaque tuple identifiers.

2. Apparently, GIN uses the structure of TIDs to compress posting
lists.  (I'm not personally familiar with this code.)

In general, it's a fairly dangerous thing to suppose that you can
repurpose a value as widely used as a TID and not break anything.  I'm
not saying it can't be done, but we use TIDs in an awful lot of places
and rooting out all of the places somebody may have made an assumption
about the structure of them may not be trivial.  I tend to think it's
an ugly kludge to shove some other kind of value into a TID, anyway.
If we need to store something that's not a TID, I think we should have
a purpose-built mechanism for that, not just hammer on the existing
system until it sorta works.

-- 
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] Pluggable storage

[Robert Haas]

On Thu, Jun 22, 2017 at 9:30 AM, Alexander Korotkov
 wrote:
> If #1 is only about changing tuple and page formats, then could be much
> simpler than the patch upthread?  We can implement "page format access
> methods" with routines for insertion, update, pruning and deletion of tuples
> *in particular page*.  There is no need to redefine high-level logic for
> scanning heap, doing updates and so on...

That assumes that every tuple format does those things in the same
way, which I suspect is not likely to be the case.  I think that
pruning and vacuum are artifacts of the current heap format, and they
may be nonexistent or take some altogether different form in some
other storage engine.  InnoDB isn't much like the PostgreSQL heap, and
neither is SQL Server, IIUC.  If we're creating a heap format that can
only be different in trivial ways from what we have now, and anything
that really changes the paradigm is off-limits, well, that's not
really interesting enough to justify the work of creating a heap
storage API.

-- 
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] segfault in HEAD when too many nested functions call

[Julien Rouhaud]

Hello,

Since b8d7f053c5c2bf2a7e8734fe3327f6a8bc711755 (Andres in Cc), if you
write queries which result in infinite recursion (or just too many
nested function calls), execution ends with segfault instead of intended
exhausted max_stack_depth:

Program received signal SIGSEGV, Segmentation fault.
0x00df851e in DirectFunctionCall1Coll (
func=,
collation=,
arg1=) at fmgr.c:708

Please find attached a trivial patch to fix this.  I'm not sure
ExecMakeTableFunctionResult() is the best or only place that needs to
check the stack depth.

I also attached a simple sql file to reproduce the issue if needed.
Should I add a regression test based on it?

-- 
Julien Rouhaud
http://dalibo.com - http://dalibo.org


segfault.sql
Description: application/sql
diff --git a/src/backend/executor/execSRF.c b/src/backend/executor/execSRF.c
index 138e86ac67..9d257d4d88 100644
--- a/src/backend/executor/execSRF.c
+++ b/src/backend/executor/execSRF.c
@@ -117,6 +117,9 @@ ExecMakeTableFunctionResult(SetExprState *setexpr,
 	MemoryContext oldcontext;
 	bool		first_time = true;
 
+	/* Guard against stack overflow due to overly nested functions call */
+	check_stack_depth();
+
 	callerContext = CurrentMemoryContext;
 
 	funcrettype = exprType((Node *) setexpr->expr);

-- 
Sent 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] Zipfian distribution in pgbench

[Peter Geoghegan]

On Fri, Jul 14, 2017 at 6:37 AM, Alik Khilazhev
 wrote:
> I am attaching results of tests for 32 and 128 clients that were running for 
> 10 minutes, and TPS remains 305 and 115 ktps respectively.
>
> Tests was executed with configuration set for YCSB. And there is very 
> aggressively autovacuum, this can be reason why there is no decline appears 
> with increasing working time.
> Autovacuum config:
>
> autovacuum_max_workers = 8
> autovacuum_naptime = 10s
> autovacuum_vacuum_scale_factor = 0.1
> autovacuum_vacuum_cost_delay = 0ms
> autovacuum_vacuum_cost_limit = 1

I think that what this probably comes down to, more than anything
else, is that you have leftmost hot/bloated leaf pages like this:


  idx  | level | l_item | blkno | btpo_prev |
btpo_next | btpo_flags | type | live_items | dead_items |
avg_item_size | page_size | free_size | highkey
---+---++---+---+---++--+++---+---+---+-
 ...
 pgbench_accounts_pkey | 0 |  1 | 1 | 0 |
2751 | 65 | l|100 | 41 |16 |
   8192 |  5328 | 11 00 00 00 00 00 00 00
 pgbench_accounts_pkey | 0 |  2 |  2751 | 1 |
2746 | 65 | l| 48 | 90 |16 |
   8192 |  5388 | 32 00 00 00 00 00 00 00
...

The high key for the leftmost shows that only values below 0x11 belong
on the first page. This is about 16 or 17 possible distinct values,
and yet the page has 100 live items, and 41 dead items; in total,
there is room for 367 items. It's only slightly better with other
nearby pages. This is especially bad because once the keyspace gets
split up this finely, it's *impossible* to reverse it -- it's more or
less a permanent problem, at least until a REINDEX. You cannot merge
pages back together until one is completely empty, which in this case
and many cases will in fact never happen. Aggressive vacuuming is
probably helpful in part because it prevents the problem from ever
getting completely out of hand. That doesn't seem like a very
practical solution, though.

We should probably be treating unique indexes in a special way, since
inserting a new conflicting tuple necessarily supersedes whatever it
conflicted with. Postgres holds on to the old tuple today, but only
because the transaction might abort, or because an old snapshot might
be interested in old tuple versions. However, the general pattern with
unique indexes is that there must be one tuple visible to new
snapshots, and old versions are almost certainly going to became
garbage very quickly. Unique indexes really are quite special, which
nbtree doesn't take advantage of at all. If you read the coverage of
B-Trees within "Transaction Processing: Concepts and Techniques", and
many other publications, the general trend seems to be that unique
indexes have truly unique keys, based only on the user-visible key
values. They make a sharp distinction between primary and secondary
indexes, which doesn't really exist in Postgres (at least, not at the
access method level).

I suspect that the best long term solution is to add GIN-style
duplicate handling within nbtree for unique indexes, with special
pruning style optimizations to the posting list structure. This would
probably only happen with unique indexes. The useful thing about this
approach is it separates these two problems:

1. Representing what values are in the index for lookup, and their
latest row version.

2. Finding old row versions, in the less common case where you have an
old snapshot.

With a design like that, nbtree would never "cut up the keyspace too
finely" because of a temporary surge of UPDATE insertions. You still
get bloat, but you add it to a place where garbage collection can be
much better targeted. Under this scheme, it doesn't really matter so
much if garbage collection doesn't happen very frequently, because
there could be LP_DEAD style hints for the auxiliary posting list
structure. That could probably work based on atomic ops, and would
greatly reduce the number of pages that UPDATE workloads like this
dirtied.

It probably wouldn't make sense to add things like GIN's compression
of item pointers, since most data within the auxiliary posting list
structure is removed very quickly. I wouldn't expect btree_gin to be
faster for this workload today, because it doesn't have
kill_prior_tuple/LP_DEAD support, and because it doesn't support
unique indexes, and so cannot exploit the special situation that
exists with unique indexes.

-- 
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] GSoC 2017: Foreign Key Arrays

[Alvaro Herrera]

Mark Rofail wrote:
> On Wed, Jul 12, 2017 at 2:30 PM, Mark Rofail  wrote:
> 
> > On Wed, Jul 12, 2017 at 12:53 AM, Alvaro Herrera  > > wrote:
> >>
> >> We have one opclass for each type combination -- int4 to int2, int4 to
> >> int4, int4 to int8, etc.  You just need to add the new strategy to all
> >> the opclasses.
> >>
> >
> > Can you clarify this solution ? I think another solution would be external
> > casting
> >
> If external casting is to be used. If for example the two types in
> question are smallint and integer. Would a function get_common_type(Oid
> leftopr, Oid rightopr) be useful ?, that given the two types return the
> "common" type between the two in this case integer.

Do you mean adding cast decorators to the query constructed by
ri_triggers.c?  That looks like an inferior solution.  What problem do
you see with adding more rows to the opclass?

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


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

[HACKERS] The case for removing replacement selection sort

[Peter Geoghegan]

There was a number of improvements to tuplesort.c external sort
merging made for Postgres 10. One in particular, the changes to merge
heap maintenance that occurred for commit 24598337c8d, really helped
with presorted cases -- cases when there was an (inverse)
physical/logical correlation.

Replacement selection sort for run generation was not killed as part
of the improvements to external sorting in Postgres 9.6, although
there was a reasonably good case for that to be made at the time those
enhancements went in. This was because it was still possible for its
best case to come out ahead. The best case is the case where it
manages to produce a single run, all in one go, by exploiting
presortedness. The examples where this was possible were fairly
narrow, but they existed.

With the additional enhancements made to Postgres 10, I doubt that
there are any remaining cases where it wins. In addition to the point
about heap management and presortedness, you also have to consider
that TSS_SORTEDONTAPE processing is optimized for random access. This
means that one-big-replacement-selection-run cases will not take
advantage of available memory, improvements in Postgres 10 by Heikki
to tape preloading for merging, OS readahead, and so on. Merging is
often quite I/O bound these days, especially when merging naturally
requires few comparisons. TSS_SORTEDONTAPE processing is
inappropriate.

I think we should remove the replacement_sort_tuples GUC, and kill
replacement selection entirely. There is no need to do this for
Postgres 10. I don't feel very strongly about it. It just doesn't make
sense to continue to support replacement selection.

-- 
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] SCRAM auth and Pgpool-II

[Stephen Frost]

Greetings,

* Vladimir Borodin (r...@simply.name) wrote:
> > 14 июля 2017 г., в 1:33, Stephen Frost  написал(а):
> > What would be really nice for such cases is support for Kerberos and
> > delegated Kerberos credentials.  Having pgpool support that would remove
> > the need to deal with passwords at all.
> 
> Since nearly all systems with some kind of load nowadays use connection 
> poolers (pgpool-II or pgbouncer) between applications and postgres, it is a 
> pretty big pain to re-implement all authentication methods supported by 
> postgres in such poolers. Kerberos is cool but not the only thing that should 
> be supported by FDWs or connection poolers. I.e. many users would want to 
> have support for LDAP and SCRAM. And every time when there would be some 
> changes in postgres auth methods, exactly the same work (or even worse) 
> should be done in many (at least two) other products widely used by people.

Honestly, I disagree about the notion that LDAP support should be added
to anything or encouraged.  There's a reason that AD uses Kerberos and
not LDAP and Microsoft continues to (quite reasonably) make it more
difficult for individuals to perform LDAP auth in AD.

The auth methods in PG do not see a huge amount of churn over the years
and so I don't agree with the argument that it would be a problem for
other systems to support Kerberos or similar strong auth methods.

> It seems that postgres either should provide connection pooling feature in 
> core or give external poolers a kind of generic mechanism to transparently 
> proxy auth requests/responses, so that authentication would be fully managed 
> by postgres and that would be the only place where changes in auth methods 
> should be done. Yes, in this case connection pooler actually behaves like man 
> in the middle so it should be done very carefully but it seems that there is 
> no other way.

While this might be possible by having some kind of special trusted
connection between the pooler and PG, I actually don't particularly like
the notion of inventing a bunch of complicated logic and pain so that a
connection pooler can avoid implementing a proper authentication system.

Having PG support connection pooling itself, of course, would be nice
but I'm not aware of anyone currently working on it.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

On Wed, Jul 12, 2017 at 2:30 PM, Mark Rofail  wrote:

> On Wed, Jul 12, 2017 at 12:53 AM, Alvaro Herrera  > wrote:
>>
>> We have one opclass for each type combination -- int4 to int2, int4 to
>> int4, int4 to int8, etc.  You just need to add the new strategy to all
>> the opclasses.
>>
>
> Can you clarify this solution ? I think another solution would be external
> casting
>
>>
>> If external casting is to be used. If for example the two types in
question are smallint and integer. Would a function get_common_type(Oid
leftopr, Oid rightopr) be useful ?, that given the two types return the
"common" type between the two in this case integer.

Best Regards,
 Mark Rofail

[HACKERS] bug in locking an update tuple chain

[Alvaro Herrera]

A customer of ours reported a problem in 9.3.14 while inserting tuples
in a table with a foreign key, with many concurrent transactions doing
the same: after a few insertions worked sucessfully, a later one would
return failure indicating that the primary key value was not present in
the referenced table.  It worked fine for them on 9.3.4.

After some research, we determined that the problem disappeared if
commit this commit was reverted:

Author: Alvaro Herrera 
Branch: master Release: REL9_6_BR [533e9c6b0] 2016-07-15 14:17:20 -0400
Branch: REL9_5_STABLE Release: REL9_5_4 [649dd1b58] 2016-07-15 14:17:20 -0400
Branch: REL9_4_STABLE Release: REL9_4_9 [166873dd0] 2016-07-15 14:17:20 -0400
Branch: REL9_3_STABLE Release: REL9_3_14 [6c243f90a] 2016-07-15 14:17:20 -0400

Avoid serializability errors when locking a tuple with a committed update

I spent some time writing an isolationtester spec to reproduce the
problem.  It turned out that this required six concurrent sessions in
order for the problem to show up at all, but once I had that, figuring
out what was going on was simple: a transaction wants to lock the
updated version of some tuple, and it does so; and some other
transaction is also locking the same tuple concurrently in a compatible
way.  So both are okay to proceed concurrently.  The problem is that if
one of them detects that anything changed in the process of doing this
(such as the other session updating the multixact to include itself,
both having compatible lock modes), it loops back to ensure xmax/
infomask are still sane; but heap_lock_updated_tuple_rec is not prepared
to deal with the situation of "the current transaction has the lock
already", so it returns a failure and the tuple is returned as "not
visible" causing the described problem.

I *think* that the problem did not show up before the commit cited above
because the bug fixed by that commit reduced concurrency, effectively
masking this bug.

In assertion-enabled builds, this happens instead (about 2 out of 3
times I run the script in my laptop):

TRAP: 
FailedAssertion(«!(TransactionIdIsCurrentTransactionId(((!((tup)->t_infomask & 
0x0800) && ((tup)->t_infomask & 0x1000) && !((tup)->t_infomask & 0x0080)) ? 
HeapTupleGetUpdateXid(tup) : ( (tup)->t_choice.t_heap.t_xmax) )))», Archivo: 
«/pgsql/source/REL9_3_STABLE/src/backend/utils/time/combocid.c», Línea: 122)

with this backtrace:

(gdb) bt
#0  0x7f651cd66067 in __GI_raise (sig=sig@entry=6) at 
../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x7f651cd67448 in __GI_abort () at abort.c:89
#2  0x00754ac1 in ExceptionalCondition (
conditionName=conditionName@entry=0x900d68 
"!(TransactionIdIsCurrentTransactionId(( (!((tup)->t_infomask & 0x0800) && 
((tup)->t_infomask & 0x1000) && !
+((tup)->t_infomask & 0x0080)) ? HeapTupleGetUpdateXid(tup) : ( 
(tup)->t_choice.t_heap.t_xmax "...,
errorType=errorType@entry=0x78cdb4 "FailedAssertion",
fileName=fileName@entry=0x900ca8 
"/pgsql/source/REL9_3_STABLE/src/backend/utils/time/combocid.c", 
lineNumber=lineNumber@entry=122)
at /pgsql/source/REL9_3_STABLE/src/backend/utils/error/assert.c:54
#3  0x00781cf8 in HeapTupleHeaderGetCmax (tup=0x7f6513f289d0)
at /pgsql/source/REL9_3_STABLE/src/backend/utils/time/combocid.c:122
#4  0x00495911 in heap_lock_tuple (relation=0x7f651e0d9138, 
tuple=0x7ffe928a7de0, cid=0, mode=LockTupleKeyShare,
nowait=0 '\000', follow_updates=1 '\001', buffer=0x7ffe928a7dcc, 
hufd=0x7ffe928a7dd0)
at /pgsql/source/REL9_3_STABLE/src/backend/access/heap/heapam.c:4439
#5  0x005b2f74 in ExecLockRows (node=0x290f070) at 
/pgsql/source/REL9_3_STABLE/src/backend/executor/nodeLockRows.c:134

The attached patch fixes the problem.  When locking some old tuple version of
the chain, if we detect that we already hold that lock
(test_lockmode_for_conflict returns HeapTupleSelfUpdated), do not try to lock
it again but instead skip ahead to the next version.  This fixes the synthetic
case in my isolationtester as well as our customer's production case.


I attach the isolationtester spec file.  In its present form it's rather
noisy, because I added calls to report the XIDs for each transaction, so
that I could exactly replicate the WAL sequence that I obtained from the
customer in order to reproduce the issue.  That would have to be removed
in order for the test to be included in the repository, of course.  This
spec doesn't work at all with 9.3's isolationtester, because back then
isolationtester had the limitation that only one session could be
waiting; to verify this bug, I backpatched 38f8bdcac498 ("Modify the
isolation tester so that multiple sessions can wait.") so that it'd work
at all.  This means that we cannot include the test in branches older
than 9.6.

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

Re: [HACKERS] PG 10 release notes

[Adrien Nayrat]

On 07/13/2017 04:36 PM, Adrien Nayrat wrote:
> Hello hackers,
> 
> From: Peter Geoghegan 
>> Date: Wed, 5 Jul 2017 15:19:57 -0700
>> Subject: Re: [BUGS] BUG #14722: Segfault in tuplesort_heap_siftup, 32 bit 
>> overflow
>> On pgsql-b...@postgresql.org
> 
> On 07/06/2017 12:19 AM, Peter Geoghegan wrote:
>> In Postgres 10, tuplesort external sort run merging became much faster
>> following commit 24598337c8d. It might be noticeable if such a machine
>> were using Postgres 10 [...]
> 
> Should-we mention this improvement in release notes?
> 
> Regards,
> 

Patch attached.

Magic sorting is not my cup of tea, so I only put "Improve external sort". Maybe
we should add an explanation.



Regards,

-- 
Adrien NAYRAT

http://dalibo.com - http://dalibo.org
diff --git a/doc/src/sgml/release-10.sgml b/doc/src/sgml/release-10.sgml
index debaa80..e654c17 100644
--- a/doc/src/sgml/release-10.sgml
+++ b/doc/src/sgml/release-10.sgml
@@ -856,6 +856,16 @@
 
   
 
+   
+Improve external sort (Peter Geoghegan)
+   
+
+  
+
+  
+



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors

[Marina Polyakova]

Well, the short version may be to only do a full transaction retry and
to document that for now savepoints are not handled, and to let that
for future work if need arises.


I agree with you.


For progress the output must be short and readable, and probably we do
not care about whether retries came from this or that, so I would let
that out.

For log and aggregated log possibly that would make more sense, but it
must stay easy to parse.


Ok!

--
Marina Polyakova
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] WIP Patch: Pgbench Serialization and deadlock errors

[Marina Polyakova]

Ok, fine. My point was just to check before proceeding.


And I'm very grateful for that :)

--
Marina Polyakova
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] [WIP] Zipfian distribution in pgbench

[Fabien COELHO]

Algorithm works with theta less than 1. The only problem here is that 
theta can not be 1, because of next line of code


cell->alpha = 1. / (1 - theta);

That’s why I put such restriction. Now I see 2 possible solutions for that:
1) Exclude 1, and allow everything in range (0;+∞).


Yep.


2) Or just increase/decrease theta by very small number if it is 1.


Nope, this seems quite arbitrary.

I've executed scripts that you attached with different theta and number 
of outcomes(not n, n remains the same = 100) and I found out that for 
theta = 0.1 and big number of outcomes it gives distribution very 
similar to zipfian(for number of outcomes = 100 000, bias -6% to 8% in 
whole range and for NOO = 1000 000, bias is -2% to 2%).


Ok, so you did not get the large bias for i=3. Strange.

By, number of outcomes(NOO) I mean how many times random_zipfian was 
called. For example: pgbench -f compte_bench.sql -t 10 So, I think 
it works but works worse for small number of outcomes. And also we need 
to find optimal theta for better results.


Hmmm. I've run one million outcomes each time.

I'll check on the next version.

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

Re: [HACKERS] SCRAM auth and Pgpool-II

[Vladimir Borodin]

> 14 июля 2017 г., в 1:33, Stephen Frost  написал(а):
> 
> What would be really nice for such cases is support for Kerberos and
> delegated Kerberos credentials.  Having pgpool support that would remove
> the need to deal with passwords at all.

Since nearly all systems with some kind of load nowadays use connection poolers 
(pgpool-II or pgbouncer) between applications and postgres, it is a pretty big 
pain to re-implement all authentication methods supported by postgres in such 
poolers. Kerberos is cool but not the only thing that should be supported by 
FDWs or connection poolers. I.e. many users would want to have support for LDAP 
and SCRAM. And every time when there would be some changes in postgres auth 
methods, exactly the same work (or even worse) should be done in many (at least 
two) other products widely used by people.

It seems that postgres either should provide connection pooling feature in core 
or give external poolers a kind of generic mechanism to transparently proxy 
auth requests/responses, so that authentication would be fully managed by 
postgres and that would be the only place where changes in auth methods should 
be done. Yes, in this case connection pooler actually behaves like man in the 
middle so it should be done very carefully but it seems that there is no other 
way.


--
May the force be with you…
https://simply.name

Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors

[Fabien COELHO]

And I'm not sure that we should do all the stuff for savepoints rollbacks 
because:

- as I see it now it only makes sense for the deadlock failures;
- if there's a failure what savepoint we should rollback to and start the 
execution again?


ISTM that it is the point of having savepoint in the first place, the 
ability to restart the transaction at that point if something failed?


Maybe to go to the last one, if it is not successful go to the previous 
one etc. Retrying the entire transaction may take less time..


Well, I do not know that. My 0.02 € is that if there was a savepoint then 
this is natural the restarting point of a transaction which has some 
recoverable error.


Well, the short version may be to only do a full transaction retry and to 
document that for now savepoints are not handled, and to let that for 
future work if need arises.



Maybe something like:
  ...
  number of failures: 12 (0.004%)
  number of retries: 64 (deadlocks: 29, serialization: 35)


Ok! How to you like the idea to use the same format (the total number of 
transactions with failures and the number of retries for each failure type) 
in other places (log, aggregation log, progress) if the values are not 
"default" (= no failures and no retries)?


For progress the output must be short and readable, and probably we do not 
care about whether retries came from this or that, so I would let that 
out.


For log and aggregated log possibly that would make more sense, but it 
must stay easy to parse.


--
Fabien.
--
Sent 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 Patch: Pgbench Serialization and deadlock errors

[Fabien COELHO]

If the answer is no, then implement something in pgbench directly.


The structure of variables is different, the container structure of the 
variables is different, so I think that the answer is no.


Ok, fine. My point was just to check before proceeding.

--
Fabien.


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

Re: [HACKERS] SCRAM auth and Pgpool-II

[Chapman Flack]

On 07/13/2017 10:46 PM, Chapman Flack wrote:

> Neither is suitable on an unencrypted channel (as has been repeatedly

Please forgive my thinko about md5. I had overlooked the second
salted md5 used in the protocol, and that had to be some years ago
when I was sure I had looked for one in the code. But it's been there
since 2001, so I simply overlooked it somehow. Not sure how. I've had
an unnecessarily jaundiced view of "md5" auth for years as a result.

I feel much better now. Sorry for the noise.

-Chap


-- 
Sent 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 Patch: Pgbench Serialization and deadlock errors

[Marina Polyakova]

Given that the number of variables of a pgbench script is expected to
be pretty small, I'm not sure that the sorting stuff is worth the
effort.


I think it is a good insurance if there're many variables..


My suggestion is really to look at both implementations and to answer
the question "should pgbench share its variable implementation with
psql?".

If the answer is yes, then the relevant part of the implementation
should be moved to fe_utils, and that's it.

If the answer is no, then implement something in pgbench directly.


The structure of variables is different, the container structure of the 
variables is different, so I think that the answer is no.


--
Marina Polyakova
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] WIP Patch: Pgbench Serialization and deadlock errors

[Marina Polyakova]

Not necessarily? It depends on where the locks triggering the issue
are set, if they are all set after the savepoint it could work on a
second attempt.


Don't you mean the deadlock failures where can really help rollback to


Yes, I mean deadlock failures can rollback to a savepoint and work on
a second attempt.

And could you, please, give an example where a rollback to savepoint 
can help to end its subtransaction successfully after a serialization 
failure?


I do not know whether this is possible with about serialization 
failures.
It might be if the stuff before and after the savepoint are somehow 
unrelated...


If you mean, for example, the updates of different tables - a rollback 
to savepoint doesn't help.


And I'm not sure that we should do all the stuff for savepoints 
rollbacks because:

- as I see it now it only makes sense for the deadlock failures;
- if there's a failure what savepoint we should rollback to and start 
the execution again? Maybe to go to the last one, if it is not 
successful go to the previous one etc.

Retrying the entire transaction may take less time..

[...] I mean that the sum of transactions with serialization failure 
and transactions with deadlock failure can be greater then the totally 
sum of transactions with failures.


Hmmm. Ok.

A "failure" is a transaction (in the sense of pgbench) that could not
made it to the end, even after retries. If there is a rollback and the
a retry which works, it is not a failure.

Now deadlock or serialization errors, which trigger retries, are worth
counting as well, although they are not "failures". So my format
proposal was over optimistic, and the number of deadlocks and
serializations should better be on a retry count line.

Maybe something like:
  ...
  number of failures: 12 (0.004%)
  number of retries: 64 (deadlocks: 29, serialization: 35)


Ok! How to you like the idea to use the same format (the total number of 
transactions with failures and the number of retries for each failure 
type) in other places (log, aggregation log, progress) if the values are 
not "default" (= no failures and no retries)?


--
Marina Polyakova
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] [WIP] Zipfian distribution in pgbench

[Alik Khilazhev]

> On 13 Jul 2017, at 23:13, Peter Geoghegan  wrote:
> 
> I just figured out that "result.txt" is only a 60 second pgbench run.
> Is the same true for other tests?

Yes, other tests ran 60 seconds too.

> 
> It would be much more interesting to see runs that lasted 10 minutes
> or more.


I am attaching results of tests for 32 and 128 clients that were running for 10 
minutes, and TPS remains 305 and 115 ktps respectively. 

Tests was executed with configuration set for YCSB. And there is very 
aggressively autovacuum, this can be reason why there is no decline appears 
with increasing working time.
Autovacuum config:

autovacuum_max_workers = 8 
autovacuum_naptime = 10s
autovacuum_vacuum_scale_factor = 0.1
autovacuum_vacuum_cost_delay = 0ms
autovacuum_vacuum_cost_limit = 1

  idx  | level | l_item | blkno | btpo_prev | btpo_next | 
btpo_flags | type | live_items | dead_items | avg_item_size | page_size | 
free_size | highkey 
---+---++---+---+---++--+++---+---+---+-
 pgbench_accounts_pkey | 2 |  1 |   290 | 0 | 0 |   
   2 | r| 10 |  0 |15 |  8192 |  7956 | 
 pgbench_accounts_pkey | 1 |  1 | 3 | 0 |   289 |   
   0 | i|296 |  0 |15 |  8192 |  2236 | 
09 96 01 00 00 00 00 00
 pgbench_accounts_pkey | 1 |  2 |   289 | 3 |   575 |   
   0 | i|285 |  0 |15 |  8192 |  2456 | 
11 2c 03 00 00 00 00 00
 pgbench_accounts_pkey | 1 |  3 |   575 |   289 |   860 |   
   0 | i|285 |  0 |15 |  8192 |  2456 | 
19 c2 04 00 00 00 00 00
 pgbench_accounts_pkey | 1 |  4 |   860 |   575 |  1145 |   
   0 | i|285 |  0 |15 |  8192 |  2456 | 
21 58 06 00 00 00 00 00
 pgbench_accounts_pkey | 1 |  5 |  1145 |   860 |  1430 |   
   0 | i|285 |  0 |15 |  8192 |  2456 | 
29 ee 07 00 00 00 00 00
 pgbench_accounts_pkey | 1 |  6 |  1430 |  1145 |  1715 |   
   0 | i|285 |  0 |15 |  8192 |  2456 | 
31 84 09 00 00 00 00 00
 pgbench_accounts_pkey | 1 |  7 |  1715 |  1430 |  2000 |   
   0 | i|285 |  0 |15 |  8192 |  2456 | 
39 1a 0b 00 00 00 00 00
 pgbench_accounts_pkey | 1 |  8 |  2000 |  1715 |  2285 |   
   0 | i|285 |  0 |15 |  8192 |  2456 | 
41 b0 0c 00 00 00 00 00
 pgbench_accounts_pkey | 1 |  9 |  2285 |  2000 |  2570 |   
   0 | i|285 |  0 |15 |  8192 |  2456 | 
49 46 0e 00 00 00 00 00
 pgbench_accounts_pkey | 1 | 10 |  2570 |  2285 | 0 |   
   0 | i|177 |  0 |15 |  8192 |  4616 | 
 pgbench_accounts_pkey | 0 |  1 | 1 | 0 |  2751 |   
  65 | l|100 | 41 |16 |  8192 |  5328 | 
11 00 00 00 00 00 00 00
 pgbench_accounts_pkey | 0 |  2 |  2751 | 1 |  2746 |   
  65 | l| 48 | 90 |16 |  8192 |  5388 | 
32 00 00 00 00 00 00 00
 pgbench_accounts_pkey | 0 |  3 |  2746 |  2751 |  2749 |   
  65 | l| 47 |  6 |16 |  8192 |  7088 | 
5f 00 00 00 00 00 00 00
 pgbench_accounts_pkey | 0 |  4 |  2749 |  2746 |  2745 |   
  65 | l| 57 |  5 |16 |  8192 |  6908 | 
97 00 00 00 00 00 00 00
 pgbench_accounts_pkey | 0 |  5 |  2745 |  2749 |  2748 |   
  65 | l| 91 | 11 |16 |  8192 |  6108 | 
eb 00 00 00 00 00 00 00
 pgbench_accounts_pkey | 0 |  6 |  2748 |  2745 |  2752 |   
   1 | l| 65 |  0 |16 |  8192 |  6848 | 
28 01 00 00 00 00 00 00
 pgbench_accounts_pkey | 0 |  7 |  2752 |  2748 | 2 |   
  65 | l| 73 |  1 |16 |  8192 |  6668 | 
6f 01 00 00 00 00 00 00
 pgbench_accounts_pkey | 0 |  8 | 2 |  2752 |  2753 |   
  65 | l| 69 |  5 |16 |  8192 |  6668 | 
b3 01 00 00 00 00 00 00
 pgbench_accounts_pkey | 0 |  9 |  2753 | 2 |  2747 |   
  65 | l| 91 |  5 |16 |  8192 |  6228 | 
0a 02 00 00 00 00 00 00
 pgbench_accounts_pkey | 0 | 10 |  2747 |  2753 |  2754 |   
  65 | l| 87 |

[HACKERS] Re: [BUGS] BUG #14634: On Windows pg_basebackup should write tar to stdout in binary mode

[Heikki Linnakangas]

On 07/14/2017 05:27 AM, Haribabu Kommi wrote:

On Fri, Jul 14, 2017 at 2:54 AM, Heikki Linnakangas  wrote:


On 05/03/2017 07:32 AM, Haribabu Kommi wrote:


[Adding -hackers mailing list]

On Fri, Apr 28, 2017 at 6:28 PM,  wrote:


Executing command pg_basebackup -D -F t writes its output to stdout,
which
is open in text mode, causing LF to be converted to CR LF thus corrupting
the resulting archive.

To write the tar to stdout, on Windows stdout's mode should be
temporarily
switched to binary.

https://msdn.microsoft.com/en-us/library/tw4k6df8.aspx


Thanks for reporting the issue.
With the attached patch, I was able to extract the tar file that gets
generated when the tar file is written into stdout. I tested the
the compressed tar also.

This bug needs to be fixed in back branches also.


Seems reasonable. One question:

In the patch, you used "_setmode" function, while the calls in
src/bin/pg_dump/pg_backup_archiver.c use "setmode". There are a few
places in the backend that also use "_setmode". What's the difference?
Should we change some of them to be consistent?


Actually there is no functional difference between these two functions.
one is a POSIX variant and another one is ISO C++ variant [1]. The support
of POSIX variant is deprecated in windows, because of this reason we should
use the _setmode instead of setmode.

I attached the patch to change the pg_dump code to use _setmode function
instead of _setmode to be consistent with other functions.


Ok, committed and backpatched both patches. Thanks!

- Heikki



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

Re: [HACKERS] [WIP] Zipfian distribution in pgbench

[Alik Khilazhev]

> On 13 Jul 2017, at 19:14, Fabien COELHO  wrote:
> 
> Documentation says that the closer theta is from 0 the flatter the 
> distribution
> but the implementation requires at least 1, including strange error messages:
> 
>  zipfian parameter must be greater than 1.00 (not 1.00)
> 
> Could theta be allowed between 0 & 1 ? I've tried forcing with theta = 0.1
> and it worked well, so I'm not sure that I understand the restriction.
> I also tried with theta=0.001 but it seemed less good.

Algorithm works with theta less than 1. The only problem here is that theta can 
not be 1, because of next line of code

cell->alpha = 1. / (1 - theta);

That’s why I put such restriction. Now I see 2 possible solutions for that:
1) Exclude 1, and allow everything in range (0;+∞).
2) Or just increase/decrease theta by very small number if it is 1. 

> I have also tried to check the distribution wrt the explanations, with the 
> attached scripts, n=100, theta=1.01/1.5/3.0: It does not seem to work, 
> there is repeatable +15% bias on i=3 and repeatable -3% to -30% bias for 
> values in i=10-100, this for different values of theta (1.01,1.5, 3.0).
> 
> If you try the script, beware to set parameters (theta, n) consistently.

I've executed scripts that you attached with different theta and number of 
outcomes(not n, n remains the same = 100) and I found out that for theta = 0.1 
and big number of outcomes it gives distribution very similar to zipfian(for 
number of outcomes = 100 000, bias -6% to 8% in whole range and for NOO = 1000 
000, bias is -2% to 2%).

By, number of outcomes(NOO) I mean how many times random_zipfian was called. 
For example:
pgbench -f compte_bench.sql -t 10

So, I think it works but works worse for small number of outcomes. And also we 
need to find optimal theta for better results.

— 
Thanks and Regards,
Alik Khilazhev
Postgres Professional:
http://www.postgrespro.com 
The Russian Postgres Company

Re: [HACKERS] hash index on unlogged tables doesn't behave as expected

[Michael Paquier]

(catching up finally with this thread)

On Mon, Jul 10, 2017 at 11:57 AM, Kyotaro HORIGUCHI
 wrote:
> At Mon, 10 Jul 2017 14:58:13 +0530, Amit Kapila  
> wrote in 

Re: [HACKERS] Proposal : For Auto-Prewarm.

[Mithun Cy]

On Thu, Jul 6, 2017 at 10:52 AM, Amit Kapila  wrote:
>>> 3.
>> -- I do not think that is true pages of the unlogged table are also
>> read into buffers for read-only purpose. So if we miss to dump them
>> while we shut down then the previous dump should be used.
>>
>
> I am not able to understand what you want to say. Unlogged tables
> should be empty in case of crash recovery.  Also, we never flush
> unlogged buffers except for shutdown checkpoint, refer BufferAlloc and
> in particular below comment:
>
> * Make sure BM_PERMANENT is set for buffers that must be written at every
> * checkpoint.  Unlogged buffers only need to be written at shutdown
> * checkpoints, except for their "init" forks, which need to be treated
> * just like permanent relations.

+ if (buf_state & BM_TAG_VALID &&
+ ((buf_state & BM_PERMANENT) || dump_unlogged))
I have changed it now the final call to dump_now during shutdown or if
called through autoprewarm_dump_now() only we dump blockinfo of
unlogged tables.

>>
>> -- autoprewarm_dump_now is directly called from the backend. In such
>> case, we have to handle signals registered for backend in dump_now().
>> For bgworker dump_block_info_periodically caller of dump_now() handles
>> SIGTERM, SIGUSR1 which we are interested in.
>>
>
> Okay, but what about signal handler for c
> (procsignal_sigusr1_handler).  Have you verified that it will never
> set the InterruptPending flag?

Okay now CHECK_FOR_INTERRUPTS is called for both.

>
>>>
>>> 7.
>>> +dump_now(bool is_bgworker)
>>> {
>>> ..
>>> + (void) durable_rename(transient_dump_file_path, AUTOPREWARM_FILE, ERROR);
>>>
>>> ..
>>> }
>>>
>>> How will transient_dump_file_path be unlinked in the case of error in
>>> durable_rename?  I think you need to use PG_TRY..PG_CATCH to ensure
>>> same?
>>
>> -- If durable_rename is failing that seems basic functionality of
>> autoperwarm is failing so I want it to be an ERROR. I do not want to
>> remove the temp file as we always truncate before reusing it again. So
>> I think there is no need to catch all ERROR in dump_now() just to
>> remove the temp file.
>>
> I am not getting your argument here, do you mean to say that if
> writing to a transient file is failed then we should remove the
> transient file but if the rename is failed then there is no need to
> remove it?  It sounds strange to me, but maybe you have reason to do
> it like that.

my intention is to unlink when ever possible and when ever control is
within the function. I thought it is okay if we error inside called
function. If temp file is left there that will not be a problem as it
will be reused(truncated first) for next dump. If you think it is
needed I will add a try() catch() around, to catch any error and then
remove the file.
-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


autoprewarm_19.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] WIP Patch: Pgbench Serialization and deadlock errors

[Fabien COELHO]

Note that there is something for psql (src/bin/psql/variable.c) which 
may or may not be shared. It should be checked before recoding 
eventually the same thing.


Thank you very much for pointing this file! As I checked this is another 
structure: here there's a simple list, while in pgbench we should know 
if the list is sorted and the number of elements in the list. How do you 
think, is it a good idea to name a variables structure in pgbench in the 
same way (VariableSpace) or it should be different not to be confused 
(Variables, for example)?


Given that the number of variables of a pgbench script is expected to be 
pretty small, I'm not sure that the sorting stuff is worth the effort.


My suggestion is really to look at both implementations and to answer the 
question "should pgbench share its variable implementation with psql?".


If the answer is yes, then the relevant part of the implementation should 
be moved to fe_utils, and that's it.


If the answer is no, then implement something in pgbench directly.

--
Fabien.


--
Sent 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 Patch: Pgbench Serialization and deadlock errors

[Fabien COELHO]

Hello Marina,


Not necessarily? It depends on where the locks triggering the issue
are set, if they are all set after the savepoint it could work on a
second attempt.


Don't you mean the deadlock failures where can really help rollback to


Yes, I mean deadlock failures can rollback to a savepoint and work on a 
second attempt.


And could you, please, give an example where a rollback to savepoint can 
help to end its subtransaction successfully after a serialization 
failure?


I do not know whether this is possible with about serialization failures.
It might be if the stuff before and after the savepoint are somehow 
unrelated...


[...] I mean that the sum of transactions with serialization failure and 
transactions with deadlock failure can be greater then the totally sum 
of transactions with failures.


Hmmm. Ok.

A "failure" is a transaction (in the sense of pgbench) that could not made 
it to the end, even after retries. If there is a rollback and the a retry 
which works, it is not a failure.


Now deadlock or serialization errors, which trigger retries, are worth 
counting as well, although they are not "failures". So my format proposal 
was over optimistic, and the number of deadlocks and serializations should 
better be on a retry count line.


Maybe something like:
  ...
  number of failures: 12 (0.004%)
  number of retries: 64 (deadlocks: 29, serialization: 35)

--
Fabien.


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

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

[Magnus Hagander]

On Thu, Jul 13, 2017 at 9:31 AM, Thomas Munro  wrote:

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

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

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

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

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


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


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

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

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

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

[HACKERS] Adding -E switch to pg_dumpall

[Michael Paquier]

Hi all,

While looking at a user problem, I got surprised that pg_dumpall does
not have a -E switch. This has been discussed a bit in the past like
here:
https://www.postgresql.org/message-id/75e4c42d37e6a74e9fb57c2e9f1300d601070...@tiger.nexperience.com

Now it is possible to enforce the encoding used by using
PGCLIENTENCODING but I think that a switch would be useful as well,
particularly for Windows where "set" needs to be used. Are there any
objections to a patch adding support for that? Such a patch should do:
- Call PQsetClientEncoding if an encoding is defined after getting a connection.
- Pass down -E to pg_dump calls if something is set.

Thoughts?
-- 
Michael


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

Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors

[Marina Polyakova]

I was not convinced by the overall memory management around variables
to begin with, and it is even less so with their new copy management.
Maybe having a clean "Variables" data structure could help improve 
the

situation.


Note that there is something for psql (src/bin/psql/variable.c) which
may or may not be shared. It should be checked before recoding
eventually the same thing.


Thank you very much for pointing this file! As I checked this is another 
structure: here there's a simple list, while in pgbench we should know 
if the list is sorted and the number of elements in the list. How do you 
think, is it a good idea to name a variables structure in pgbench in the 
same way (VariableSpace) or it should be different not to be confused 
(Variables, for example)?


--
Marina Polyakova
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] WIP Patch: Pgbench Serialization and deadlock errors

[Marina Polyakova]

On 13-07-2017 19:32, Fabien COELHO wrote:

Hello,


Hi!

[...] I didn't make rollbacks to savepoints after the failure because 
they cannot help for serialization failures at all: after rollback to 
savepoint a new attempt will be always unsuccessful.


Not necessarily? It depends on where the locks triggering the issue
are set, if they are all set after the savepoint it could work on a
second attempt.


Don't you mean the deadlock failures where can really help rollback to 
savepoint? And could you, please, give an example where a rollback to 
savepoint can help to end its subtransaction successfully after a 
serialization failure?


"SimpleStats attempts": I disagree with using this floating poiunt 
oriented structures to count integers. I would suggest "int64 tries" 
instead, which should be enough for the purpose.


I'm not sure that it is enough. Firstly it may be several transactions 
in script so to count the average attempts number you should know the 
total number of runned transactions. Secondly I think that stddev for 
attempts number can be quite interesting and often it is not close to 
zero.


I would prefer to have a real motivation to add this complexity in the
report and in the code. Without that, a simple int seems better for
now. It can be improved later if the need really arises.


Ok!


Some variables, such as "int attempt_number", should be in the client
structure, not in the client? Generally, try to use block variables 
if
possible to keep the state clearly disjoints. If there could be NO 
new
variable at the doCustom level that would be great, because that 
would
ensure that there is no machine state mixup hidden in these 
variables.


Do you mean the code cleanup for doCustom function? Because if I do so 
there will be two code styles for state blocks and their variables in 
this function..


I think that any variable shared between state is a recipee for bugs
if it is not reset properly, so they should be avoided. Maybe there
are already too many of them, then too bad, not a reason to add more.
The status before the automaton was a nightmare.


Ok!


I would suggest a more compact one-line report about failures:

  "number of failures: 12 (0.001%, deadlock: 7, serialization: 5)"


I think, there may be a misunderstanding. Because script can contain 
several transactions and get both failures.


I do not understand. Both failures number are on the compact line I 
suggested.


I mean that the sum of transactions with serialization failure and 
transactions with deadlock failure can be greater then the totally sum 
of transactions with failures. But if you think it's ok I'll change it 
and write the appropriate note in documentation.


--
Marina Polyakova
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] Authentication mechanisms categorization

[Michael Paquier]

On Fri, Jul 14, 2017 at 12:16 PM, Álvaro Hernández Tortosa
 wrote:
> If the parameter authmethod would rather be "authmethods", i.e., a list,
> I think it would be significantly more flexible.

Yes, but the handling of a list becomes messier if there are some
other connection parameters that are dependent on the authentication
method. Say if a list made of scram-sha-256 and scram-sha-3 as methods
is sent, and a parameter named saslchannel lists scram-sha-256-plus is
used, this becomes unusable with scram-sha-3. Using individual names
for a parameter makes interactions with other parameters easier to
handle and less bug-prone. That's also by experience more flexible for
the application.

> I agree with a list of methods and all the values already existing for
> sslmode, this might be more than enough, specially if the channel binding
> SCRAM mechanisms would get a different authmethod than their non-channel
> binding partners (like scram-sha-256-plus). This makes the list argument for
> the authmethods, in my opinion, stronger.

For the channel binding patch, I have actually implemented saslname to
enforce the name of the SASL mechanism name to use (SCRAM-SHA-256 or
its -PLUS) as well as saslchannelbinding to enforce the channel
binding type. That's very handy, and at the end I saw that having a
list does not add much value in a feature that should be as simple as
possible as the client will use one match at the end for
authentication, and let the client know if it failed or succeeded (yes
I hate sslmode=prefer which does up to two attempts at once). But
that's as far as my opinion stands.

It is not possible to know the future, but we cannot discard as well
the fact that a future authentication method, say hoge could as well
support scram-sha-256, in which case cases like that using a list
"authmethods=hoge,sasl authmechanisms=scram-sha-256" would mean that
scram-sha-256 needs to be enforced for both things, but the dependency
handling makes things unnecessary complicated in libpq. My argument
here is crazy though.
-- 
Michael


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

Re: [HACKERS] Authentication mechanisms categorization

[Álvaro Hernández Tortosa]

On 14/07/17 11:09, Michael Paquier wrote:

On Sat, Jul 8, 2017 at 2:19 PM, Álvaro Hernández Tortosa  
wrote:

 There has been some prior discussion, that we recently continued at
pgday.ru, about what to do if a client wants to use a "strong"
authentication mechanism but a rogue server forces the client to use a
weaker authentication mechanism. This is the case if the client expects
SCRAM to be used but a rogue server just replies with
AuthenticationCleartextPassword, for example. Client drivers will
authenticate using this latter mechanism, transparently (at least pgjdbc
implementation does this, and I believe libpq also). This somehow defeats
the purpose of some mechanisms like SCRAM.

Yeah :(


 It was discussed to add a parameter to the driver like "SCRAM-only", but
I think this may not be ideal. "SCRAM-only" means that code needs to be
written to prevent every other authentication mechanism, explicitly, which
is far from ideal. Much worse, it defeats using other auth mechanisms that
might be OK for the user. Also, this doesn't consider whether SCRAM is good
without channel binding.

 I think it would be better to make a categorization of authentication
mechanisms and then have an agreement among libpq and drivers to set a
minimum level of security based on the user's request. Some initial ideas
are:

- Three security levels: Basic, Medium, Advanced.
- Prevents MITM / does not.
- Given this X possible attacks, a matrix of which mechanisms avoid which
attacks (something similar to the table comparing the possible effects of
the different isolation levels).

 This is not trivial: for example, SCRAM may be OK without channel
binding in the presence of SSL, but without SSL channel binding is a must to
prevent MITM. Similarly, are other auth mechanisms like Kerberos (I'm not an
expert here) as "safe" as SCRAM with our without channel binding?

The use of channel binding is linked to SSL, which gets already
controlled by sslmode.


H yes. As of today. Maybe tomorrow there's a channel binding 
mechanism that does not make use of SSL. But this is also unlikely.



  Users won't get trapped in this area by using
"require" instead of the default of "prefer". I would love to see the
default value changed actually from "prefer" to "require" here.
"prefer" as a default is a trap for users. There were discussions
about that not long ago but this gave nothing.


 I believe this should be discussed and find a common agreement to be
implemented by libpq and all the drivers, including a single naming scheme
for the parameter and possible values. Opinions?

I think that we don't need to have anything complicated here: let's
have at least a connection parameter, and perhaps an environment
variable which enforces the type of the authentication method to use:
scram-sha-256, md5, etc. I don't think that there is any need to
support a list of methods, any application could just enforce the
parameter to a different value if the previous one failed.

Categorization is something that can lose value over the ages,
something considered as strong now could be weak in 10 years. By
supporting only a list of dedicated names users have the same
flexibility, and we just need to switch the default we consider safe.
Controlling SSL is already a separate and existing parameter, so I
don't think that it should be part of this scheme. Having
documentation giving a recommended combination, say
"authmethod=scram-sha-256 sslmode=require" would be enough IMO.


If the parameter authmethod would rather be "authmethods", i.e., a 
list, I think it would be significantly more flexible.


I agree with a list of methods and all the values already existing 
for sslmode, this might be more than enough, specially if the channel 
binding SCRAM mechanisms would get a different authmethod than their 
non-channel binding partners (like scram-sha-256-plus). This makes the 
list argument for the authmethods, in my opinion, stronger.



Álvaro



--

Álvaro Hernández Tortosa


---
<8K>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] pg_stop_backup(wait_for_archive := true) on standby server

[Michael Paquier]

On Fri, Jul 14, 2017 at 12:34 AM, Stephen Frost  wrote:
> Michael, all,
>
> * Michael Paquier (michael.paqu...@gmail.com) wrote:
>> On Thu, Jul 13, 2017 at 7:13 AM, Masahiko Sawada  
>> wrote:
>> > Sorry, I missed lots of typo in the last patch. All comments from you
>> > are incorporated into the attached latest patch and I've checked it
>> > whether there is other typos. Please review it.
>>
>> Thanks for providing a new version of the patch very quickly. I have
>> spent some time looking at it again and testing it, and this version
>> looks correct to me. Stephen, as a potential committer, would you look
>> at it to address this open item?
>
> Yes, this weekend.

Thanks for the confirmation, Stephen!
-- 
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] Adding support for Default partition in partitioning

[Beena Emerson]

Hello,

On Thu, Jul 13, 2017 at 1:22 AM, Jeevan Ladhe
 wrote:
>
>> - Should probably be merged with the patch to add default partitioning
>> for ranges.
>
>
> Beena is already rebasing her patch on my latest patches, so I think getting
> them merged here won't be an issue, mostly will be just like one more patch
> on top my patches.
>

I have posted the updated patch which can be applied over the v22
patches submitted here.
https://www.postgresql.org/message-id/CAOG9ApGEZxSQD-ZD3icj_CwTmprSGG7sZ_r3k9m4rmcc6ozr%3Dg%40mail.gmail.com

Thank you,

Beena Emerson

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] Default Partition for Range

[Beena Emerson]

On Tue, Jul 4, 2017 at 4:21 PM, Rahila Syed  wrote:
>
> Hello Beena,
>
> Thanks for the updated patch. It passes the basic tests which I performed.
>
> Few comments:
> 1. In   check_new_partition_bound(),
>
>> /* Default case is not handled here */
>>if (spec->is_default)
>>break;
> The default partition check here can be performed in common for both range
> and list partition.

Removed the check here.

>
> 2. RANGE_DATUM_FINITE = 0, /* actual datum stored elsewhere */
> +   RANGE_DATUM_DEFAULT,/* default */
>
> More elaborative comment?

I am not sure what to add. Do you have something in mind?

>
> 3.  Inside make_one_range_bound(),
>
>>+
>>+   /* datums are NULL for default */
>>+   if (datums == NULL)
>>+   for (i = 0; i < key->partnatts; i++)
>>+   bound->content[i] = RANGE_DATUM_DEFAULT;
>>+
> IMO, returning bound at this stage will make code clearer as further
> processing
> does not happen for default partition.

Done.

>
> 4.
> @@ -549,6 +568,7 @@ RelationBuildPartitionDesc(Relation rel)
> sizeof(RangeDatumContent
> *));
> boundinfo->indexes = (int *) palloc((ndatums + 1) *
> sizeof(int));
> +   boundinfo->default_index = -1;
> This should also be done commonly for both default list and range partition.

Removed the line here. Common allocation is done by Jeevan's patch.

>
> 5.
>   if (!spec->is_default && partdesc->nparts > 0)
> {
> ListCell   *cell;
>
> Assert(boundinfo &&
>boundinfo->strategy == PARTITION_STRATEGY_LIST &&
>(boundinfo->ndatums > 0 ||
> partition_bound_accepts_nulls(boundinfo) ||
> partition_bound_has_default(boundinfo)));
> The assertion condition partition_bound_has_default(boundinfo) is rendered
> useless
> because of if condition change and can be removed perhaps.

This cannot be removed.
This check is required when this code is run for a non-default
partition and default is the only existing partition.

>
> 6. I think its better to have following  regression tests coverage
>
> --Testing with inserting one NULL and other non NULL values for multicolumn
> range partitioned table.
>
> postgres=# CREATE TABLE range_parted (
> postgres(# a int,
> postgres(# b int
> postgres(# ) PARTITION BY RANGE (a, b);
> CREATE TABLE
> postgres=#
> postgres=# CREATE TABLE part1 (
> postgres(# a int NOT NULL CHECK (a = 1),
> postgres(# b int NOT NULL CHECK (b >= 1 AND b <= 10)
> postgres(# );
> CREATE TABLE
>
> postgres=# ALTER TABLE range_parted ATTACH PARTITION part1 FOR VALUES FROM
> (1, 1) TO (1, 10);
> ALTER TABLE
> postgres=# CREATE TABLE partr_def1 PARTITION OF range_parted DEFAULT;
> CREATE TABLE
>
> postgres=# insert into range_parted values (1,NULL);
> INSERT 0 1
> postgres=# insert into range_parted values (NULL,9);
> INSERT 0 1

added.





-- 

Beena Emerson

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] Default Partition for Range

[Beena Emerson]

Thank you for your review Dilip and Rahila.

PFA the updated patch which is rebased to Jeevan's latest list
partition patch [1] and also handles your comments.

https://www.postgresql.org/message-id/CAOgcT0OARciE2X%2BU0rjSKp9VuC279dYcCGkc3nCWKhHQ1_m2rw%40mail.gmail.com

-- 

Beena Emerson

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


default_range_partition_v7.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] Default Partition for Range

[Beena Emerson]

On Mon, Jul 3, 2017 at 8:00 PM, Dilip Kumar  wrote:
> On Fri, Jun 30, 2017 at 11:56 AM, Beena Emerson  
> wrote:
>> Hello Dilip,
>>
>> On Wed, Jun 21, 2017 at 6:27 PM, Dilip Kumar  wrote:
>>> On Tue, Jun 20, 2017 at 6:57 PM, Dilip Kumar  wrote:
 This is basically crashing in RelationBuildPartitionDesc, so I think
>
>>
>> Thank you for your review and analysis.
>>
>> I have updated the patch.
>> - bound->content is set to RANGE_DATUM_DEFAULT for each of the keys
>> and not just the first one.
>> - Improve the way of handling DEFAULT bounds in RelationBuildPartitionDesc,
>>
>> There is a test for multiple column range in alter_table.sql
>
> Thanks for update patch,  After this fix segmentation fault is solved.
>
> I have some more comments.
>
> 1.
>
> lower = make_one_range_bound(key, -1, spec->lowerdatums, true);
>   upper = make_one_range_bound(key, -1, spec->upperdatums, false);
>
> + /* Default case is not handled here */
> + if (spec->is_default)
> + break;
> +
>
> I think we can move default check just above the make range bound it
> will avoid unnecessary processing.
>

Removed the check here. Default is checked beforehand.

>
> 2.
> RelationBuildPartitionDesc
> {
> 
>
>/*
> * If either of them has infinite element, we can't equate
> * them.  Even when both are infinite, they'd have
> * opposite signs, because only one of cur and prev is a
> * lower bound).
> */
> if (cur->content[j] != RANGE_DATUM_FINITE ||
>   prev->content[j] != RANGE_DATUM_FINITE)
> {
> is_distinct = true;
> break;
> }
> After default range partitioning patch the above comment doesn't sound
> very accurate and
> can lead to the confusion, now here we are not only considering
> infinite bounds but
> there is also a possibility that prev bound can be DEFAULT, so
> comments should clearly
> mention that.

Modified.

>
> 3.
>
> + bound->datums = datums ? (Datum *) palloc0(key->partnatts * sizeof(Datum))
> + : NULL;
>   bound->content = (RangeDatumContent *) palloc0(key->partnatts *
> sizeof(RangeDatumContent));
>   bound->lower = lower;
>
>   i = 0;
> +
> + /* datums are NULL for default */
> + if (datums == NULL)
> + for (i = 0; i < key->partnatts; i++)
> + bound->content[i] = RANGE_DATUM_DEFAULT;
>
> To me, it will look cleaner if we keep bound->content=NULL for
> default, instead of allocating memory
> and initializing each element,  But it's just a suggestions and you
> can decide whichever way
> seems better to you.  Then the other places e.g.
> + if (cur->content[i] == RANGE_DATUM_DEFAULT)
> + {
> + /*
>
> will change like
>
> + if (cur->content == NULL)
> + {
> + /*

I disagree. We use the content value RANGE_DATUM_DEFAULT during
comparing in partition_rbound_cmp and the code will not be very
intuiative if we use NULL instead of DEFAULT.


-- 

Beena Emerson

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] Authentication mechanisms categorization

[Michael Paquier]

On Sat, Jul 8, 2017 at 2:19 PM, Álvaro Hernández Tortosa  
wrote:
> There has been some prior discussion, that we recently continued at
> pgday.ru, about what to do if a client wants to use a "strong"
> authentication mechanism but a rogue server forces the client to use a
> weaker authentication mechanism. This is the case if the client expects
> SCRAM to be used but a rogue server just replies with
> AuthenticationCleartextPassword, for example. Client drivers will
> authenticate using this latter mechanism, transparently (at least pgjdbc
> implementation does this, and I believe libpq also). This somehow defeats
> the purpose of some mechanisms like SCRAM.

Yeah :(

> It was discussed to add a parameter to the driver like "SCRAM-only", but
> I think this may not be ideal. "SCRAM-only" means that code needs to be
> written to prevent every other authentication mechanism, explicitly, which
> is far from ideal. Much worse, it defeats using other auth mechanisms that
> might be OK for the user. Also, this doesn't consider whether SCRAM is good
> without channel binding.
>
> I think it would be better to make a categorization of authentication
> mechanisms and then have an agreement among libpq and drivers to set a
> minimum level of security based on the user's request. Some initial ideas
> are:
>
> - Three security levels: Basic, Medium, Advanced.
> - Prevents MITM / does not.
> - Given this X possible attacks, a matrix of which mechanisms avoid which
> attacks (something similar to the table comparing the possible effects of
> the different isolation levels).
>
> This is not trivial: for example, SCRAM may be OK without channel
> binding in the presence of SSL, but without SSL channel binding is a must to
> prevent MITM. Similarly, are other auth mechanisms like Kerberos (I'm not an
> expert here) as "safe" as SCRAM with our without channel binding?

The use of channel binding is linked to SSL, which gets already
controlled by sslmode. Users won't get trapped in this area by using
"require" instead of the default of "prefer". I would love to see the
default value changed actually from "prefer" to "require" here.
"prefer" as a default is a trap for users. There were discussions
about that not long ago but this gave nothing.

> I believe this should be discussed and find a common agreement to be
> implemented by libpq and all the drivers, including a single naming scheme
> for the parameter and possible values. Opinions?

I think that we don't need to have anything complicated here: let's
have at least a connection parameter, and perhaps an environment
variable which enforces the type of the authentication method to use:
scram-sha-256, md5, etc. I don't think that there is any need to
support a list of methods, any application could just enforce the
parameter to a different value if the previous one failed.

Categorization is something that can lose value over the ages,
something considered as strong now could be weak in 10 years. By
supporting only a list of dedicated names users have the same
flexibility, and we just need to switch the default we consider safe.
Controlling SSL is already a separate and existing parameter, so I
don't think that it should be part of this scheme. Having
documentation giving a recommended combination, say
"authmethod=scram-sha-256 sslmode=require" would be enough IMO.
-- 
Michael


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

Re: [HACKERS] Replacing lfirst() with lfirst_node() appropriately in planner.c

[Ashutosh Bapat]

On Thu, Jul 13, 2017 at 9:15 PM, Alvaro Herrera
 wrote:
> Ashutosh Bapat wrote:
>
>> Happened to stumble across some instances of lfirst() which could use
>> lfirst_node() in planner.c. Here's patch which replaces calls to
>> lfirst() extracting node pointers by lfirst_node() in planner.c.
>
> Sounds good.
>
>> Are we carrying out such replacements in master or this will be
>> considered for v11?
>
> This is for pg11 definitely; please add to the open commitfest.

Thanks. Added. https://commitfest.postgresql.org/14/1195/
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database 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] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING

[Ashutosh Bapat]

On Fri, Jul 14, 2017 at 2:04 PM, Kyotaro HORIGUCHI
 wrote:
> Thank you for the comments.
>
> At Thu, 13 Jul 2017 16:54:42 +0530, Ashutosh Bapat 
>  wrote in 
> 

Re: [HACKERS] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING

[Kyotaro HORIGUCHI]

Thank you for the comments.

At Thu, 13 Jul 2017 16:54:42 +0530, Ashutosh Bapat 
 wrote in 

Re: [HACKERS] pl/perl extension fails on Windows

[Ashutosh Sharma]

On Thu, Jul 13, 2017 at 6:04 PM, Andrew Dunstan
 wrote:
>
>
> On 07/13/2017 08:08 AM, Ashutosh Sharma wrote:
>>
>> After doing some study, I could understand that Util.c is generated
>> from Util.xs by xsubpp compiler at build time. This is being done in
>> Mkvcbuild.pm file in postgres. If I manually replace
>> 'dXSBOOTARGSAPIVERCHK' macro with 'dXSBOOTARGSNOVERCHK' macro in
>> src/pl/plperl/Util.c, the things work fine. The diff is as follows,
>>
>> XS_EXTERNAL(boot_PostgreSQL__InServer__Util)
>> {
>> #if PERL_VERSION_LE(5, 21, 5)
>> dVAR; dXSARGS;
>> #else
>> -dVAR; dXSBOOTARGSAPIVERCHK;
>> +dVAR; dXSBOOTARGSNOVERCHK;
>>
>> I need to further investigate, but let me know if you have any ideas.
>
>
>
>
> Good job hunting this down!
>
>
> One suggestion I saw in a little googling was that we add this to the XS
> file after the inclusion of XSUB.h:
>
> #undef dXSBOOTARGSAPIVERCHK
> #define dXSBOOTARGSAPIVERCHK dXSBOOTARGSNOVERCHK
>

Thanks for that suggestion. It was really helpful. I think, the point
is, in XSUB.h, the macro 'dXSBOOTARGSXSAPIVERCHK' has been redefined
for novercheck but surprisingly it hasn't been done for
'dXSBOOTARGSAPIVERCHK' macro and that could be the reason why we want
to redefine it in the XS file after including XSUB.h file. Attached is
the patch that redefines 'dXSBOOTARGSAPIVERCHK' in Util.xs and SPI.xs
files. Please have a look into the attached patch and let me know your
comments. Thanks.

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com


redefine_dXSBOOTARGSAPIVERCHK_macro.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