Re: [HACKERS] Patch: Optimize memory allocation in function 'bringetbitmap'

[Jinyu Zhang]

Update the patch_brin_optimze_mem according to your comment. 






At 2015-10-16 10:13:35, "Alvaro Herrera"  wrote:
>zhangjinyu wrote:
>
>> However I wonder if it would be simpler to have the dtup structure have
>> the pointers, so that you can pass it as NULL in the first call and then
>> followup calls reuse the one allocated in the first call.
>> Jinyu:  the memory is allocated from perRangeCxt  and perRangeCxt will be
>> reset in loop,
>> so this way don't work. 
>
>You're right.  I think we can do better: have brin_deform_tuple accept
>another argument of type BrinMemTuple *, which can be NULL.  If NULL,
>the function calls brin_new_memtuple to allocate a new one (just like
>current code); if not NULL, that one is used.  Have brin_new_memtuple
>also allocate the values/allnulls/hasnulls arrays, which are now part of
>the BrinMemTuple struct.  Then, bringetbitmap calls brin_new_memtuple
>once before changing to perRangeCxt, then reuse the returned inside the
>loop (we probably need brin_memtuple_initialize to clear out the struct
>at the end of each loop iteration).  Other callers of brin_deform_tuple
>just pass NULL to get the current behavior.
>
>-- 
>Á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


patch_brin_optimize_mem
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] Allow ssl_renegotiation_limit in PG 9.5

[Shay Rojansky]

>
> > > If not, the only solution I can see is for PostgreSQL to not protest
> if it
> > > sees the
> > > parameter in the startup packet.
> > >
> >
> > Yeah, that's the ideal solution here as far as I'm concerned.
>
> Well, it seems that's where we're ending up then. Could you prepare a
> patch?
>

Yes, will do so in the coming days, thanks!

Re: [HACKERS] Proposal: SET ROLE hook

[Andres Freund]

On 2015-10-16 10:30:20 -0700, Joe Conway wrote:
> On 10/16/2015 09:28 AM, Andres Freund wrote:
> > Alternatively you can just have a elevate_user() function that does the
> > logging and escalating? That seems like the same amount of code and it'd
> > work with released versions of postgres?
> > 
> > Sure, that has some disadvantages over your approach, but for the
> > presented use case with humans needing to escalate I don't see any.
> 
> Hmmm, do you mean essentially skip the "GRANT postgres to joe" and use a
> SECURITY DEFINER C function that does the set role to postgres under the
> covers with "GRANT EXECUTE on FUNCTION elevate_user() to joe"?

Yes.

> Being able to use something like that on existing versions would be
> very nice, but it feels kind of grotty.

Hm. To me it doesn't feel too bad - security definer functions are there
to allow to do things that users would normally not be allowed to do...

Greetings,

Andres Freund


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

Re: [HACKERS] Getting sorted data from foreign server

[Robert Haas]

On Thu, Oct 15, 2015 at 6:28 AM, Ashutosh Bapat
 wrote:
> Attached is the patch which takes care of above comments.

I spent some time on this patch today.  But it's still not right.

I've attached a new version which fixes a serious problem with your
last version - having postgresGetForeignPaths do the costing of the
sorted path itself instead of delegating that to
estimate_path_cost_size is wrong.  In your version, 10% increment gets
applied to the network transmission costs as well as the cost of
generating the tupes - but only when use_remote_estimate == false.  I
fixed this and did some cosmetic cleanup.

But you'll notice if you try this some of postgres_fdw's regression
tests fail.  This is rather mysterious:

***
*** 697,715 
   Sort
 Output: t1.c1, t1.c2, t1.c3, t1.c4, t1.c5, t1.c6, t1.c7, t1.c8
 Sort Key: t1.c1
!->  Nested Loop Semi Join
   Output: t1.c1, t1.c2, t1.c3, t1.c4, t1.c5, t1.c6, t1.c7, t1.c8
!  Join Filter: (t1.c3 = t2.c3)
   ->  Foreign Scan on public.ft1 t1
 Output: t1.c1, t1.c2, t1.c3, t1.c4, t1.c5, t1.c6, t1.c7, t1.c8
 Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8
FROM "S 1"."T 1" WHERE (("C 1" < 20))
!  ->  Materialize
 Output: t2.c3
!->  Foreign Scan on public.ft2 t2
   Output: t2.c3
!  Filter: (date(t2.c4) = '01-17-1970'::date)
!  Remote SQL: SELECT c3, c4 FROM "S 1"."T 1"
WHERE (("C 1" > 10))
! (15 rows)

  EXECUTE st2(10, 20);
   c1 | c2 |  c3   |  c4  |c5
  | c6 | c7 | c8
--- 697,718 
   Sort
 Output: t1.c1, t1.c2, t1.c3, t1.c4, t1.c5, t1.c6, t1.c7, t1.c8
 Sort Key: t1.c1
!->  Hash Join
   Output: t1.c1, t1.c2, t1.c3, t1.c4, t1.c5, t1.c6, t1.c7, t1.c8
!  Hash Cond: (t1.c3 = t2.c3)
   ->  Foreign Scan on public.ft1 t1
 Output: t1.c1, t1.c2, t1.c3, t1.c4, t1.c5, t1.c6, t1.c7, t1.c8
 Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8
FROM "S 1"."T 1" WHERE (("C 1" < 20))
!  ->  Hash
 Output: t2.c3
!->  HashAggregate
   Output: t2.c3
!  Group Key: t2.c3
!  ->  Foreign Scan on public.ft2 t2
!Output: t2.c3
!Filter: (date(t2.c4) = '01-17-1970'::date)
!Remote SQL: SELECT c3, c4 FROM "S 1"."T
1" WHERE (("C 1" > 10))
! (18 rows)

What I think is happening here is that the planner notices that
instead of doing a parameterized nestloop, it could pull down the data
already sorted from the remote side, cheaply unique-ify it by using
the ordering provided by the remote side, and then do a standard hash
join.  That might well be a sensible approach, but the ORDER BY that
would make it correct doesn't show up in the Remote SQL.  I don't know
why that's happening, but it's not good.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 697de60..cb5c3ae 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -193,9 +193,12 @@ is_foreign_expr(PlannerInfo *root,
 	if (!foreign_expr_walker((Node *) expr, _cxt, _cxt))
 		return false;
 
-	/* Expressions examined here should be boolean, ie noncollatable */
-	Assert(loc_cxt.collation == InvalidOid);
-	Assert(loc_cxt.state == FDW_COLLATE_NONE);
+	/*
+	 * The collation of the expression should be none or originate from a
+	 * foreign var.
+	 */
+	Assert(loc_cxt.state == FDW_COLLATE_NONE ||
+			loc_cxt.state == FDW_COLLATE_SAFE);
 
 	/*
 	 * An expression which includes any mutable functions can't be sent over
@@ -1877,3 +1880,50 @@ printRemotePlaceholder(Oid paramtype, int32 paramtypmod,
 
 	appendStringInfo(buf, "((SELECT null::%s)::%s)", ptypename, ptypename);
 }
+
+/*
+ * Deparse ORDER BY clause according to the given pathkeys for given base
+ * relation. From given pathkeys expressions belonging entirely to the given
+ * base relation are obtained and deparsed.
+ */
+void
+appendOrderByClause(StringInfo buf, PlannerInfo *root, RelOptInfo *baserel,
+	List *pathkeys)
+{
+	ListCell			*lcell;
+	deparse_expr_cxt	context;
+	int	nestlevel;
+	char*delim = " ";
+
+	/* Set up context struct for recursion */
+	context.root = root;
+	context.foreignrel = baserel;
+	context.buf = buf;
+	context.params_list = NULL;
+
+	/* Make sure any constants in the exprs are printed portably */
+	nestlevel = set_transmission_modes();
+
+	appendStringInfo(buf, " ORDER BY");
+	foreach(lcell, pathkeys)
+	{
+		PathKey*pathkey = lfirst(lcell);
+		Expr*em_expr;
+
+		em_expr = find_em_expr_for_rel(pathkey->pk_eclass, baserel);
+		Assert(em_expr != NULL);
+
+		appendStringInfoString(buf, delim);

Re: [HACKERS] Obsolete use of volatile in walsender.c, walreceiver.c, walreceiverfuncs.c?

[Robert Haas]

On Thu, Oct 15, 2015 at 10:36 PM, Michael Paquier
 wrote:
> On Fri, Oct 16, 2015 at 9:07 AM, Thomas Munro
>  wrote:
>> On Wed, Oct 7, 2015 at 8:52 AM, Robert Haas  wrote:
>>> On Thu, Oct 1, 2015 at 11:01 PM, Michael Paquier
>>>  wrote:
> Right, see attached.

 It seems to me that we could as well simplify checkpoint.c and
 logical.c. In those files volatile casts are used as well to protect
 from reordering for spinlock operations. See for example 0002 on top
 of 0001 that is Thomas' patch.
>>>
>>> These patches look good to me, so I have committed them.
>>
>> Thanks.  Also, spin.h's comment contains an out of date warning about
>> this.  Here's a suggested fix for that, and a couple more volatrivia
>> patches.
>
> I have looked at the rest of the code, and it seems that we can get
> rid of volatile in a couple of extra places like in the attached as
> those are used with spin locks. This applies on top of Thomas' set.

OK, committed his, and yours.

I back-patched his spin.h comment fix to 9.5 since that's a factual
error, but the rest of this seems like optimization so I committed it
only to master.

-- 
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] remaining open items

[Robert Haas]

On Thu, Oct 15, 2015 at 1:48 PM, Peter Geoghegan  wrote:
> On Wed, Oct 14, 2015 at 3:14 AM, Andres Freund  wrote:
>>> What would happen if we didn't do anything at all?
>>
>> Nothing, really. It's essentially some code beautification. A worthwhile
>> goal, but certainly not a release blocker.
>
> While I agree with this assessment, I think that there is value in
> doing it before release, to ease keeping the branches in sync. That
> seems like the better time to backpatch to 9.5. That was the thinking
> behind putting it on the open items list.

That makes sense, but I think it's time to remove anything that
doesn't smell like an actual release blocker.  I'll go do 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: SET ROLE hook

[Jerry Sievers]

Joe Conway  writes:

> In many environments there is a policy requiring users to login using
> unprivileged accounts, and then escalate their privileges if and when
> they require it. In PostgreSQL this could be done by granting the
> superuser role to an unprivileged user with noinherit, and then making
> the superuser role nologin. Something like:
>
> 8<-
> psql -U postgres
> create user joe noinherit;
> grant postgres to joe;
> alter user postgres nologin;

A bit off-topic, but I've often had to create an intermediate role with
no-inherit between the gifted role to permit accessing it without the
grantee losing ability to inherit some other(s).

To wit;

We have an owner role for a given DB who is supposed to own most/all
objects.  We might want 1 or more other roles/users to be able to run as
owner guy to create objects...  We want to enforce that  objects so
created are automatically owned by the correct owner role.

create role owner;
create role owner_role no inherit in role owner;
create role read_write;
create role jerry in role read_write, owner_role;

I can by default run as the read_write guy and mangle data to my little
heart's content (the super-dev that I am) but if a new table is needs
creation I must do set role owner.  This insures that the table is owned
by owner and not jerry afterward.

Often wondered why not;

grant owner to jerry no inherit;

FWIW

> psql -U joe
> -- do stuff *not requiring* escalated privs
> set role postgres;
> -- do stuff *requiring* escalated privs
> reset role;
> 8<-
>
> One of the problems with this is we would ideally like to know whenever
> joe escalates himself to postgres. Right now that is not really possible
> without doing untenable things such as logging every statement.
>
> In order to address this issue, I am proposing a new SET ROLE hook. The
> attached patch (role-esc-hook.diff) is I believe all we need. Then
> extension authors could implement logging of privilege escalation.
>
> A proof of concept extension patch is also attached. That one is not
> meant to be applied, just illustrates one potential use of the hook. I
> just smashed it on top of passwordcheck for the sake of convenience.
>
> With both patches applied, the following scenario:
> 8<
> psql -U joe postgres
> psql (9.6devel)
> Type "help" for help.
>
> postgres=> set role postgres;
> SET
> postgres=# select rolname, rolpassword from pg_authid;
>  rolname  | rolpassword
> --+-
>  joe  |
>  postgres |
> (2 rows)
>
> postgres=# set log_statement = none;
> SET
> postgres=# reset role;
> RESET
> 8<
>
> Generates the following in the log:
>
> 8<
> LOG:  Role joe transitioning to Superuser Role postgres
> STATEMENT:  set role postgres;
> LOG:  statement: select rolname, rolpassword from pg_authid;
> LOG:  statement: set log_statement = none;
> LOG:  Superuser Role postgres transitioning to Role joe
> STATEMENT:  reset role;
> 8<
>
> Note that we cannot prevent joe from executing
>   set log_statement = none;
> but we at least get the evidence in the log and can ask joe why he felt
> the need to do that. We could also set up alerts based on the logged
> events, etc.
>
> This particular hook will not capture role changes due to SECURITY
> DEFINER functions, but I think that is not only ok but preferred.
> Escalation during a SECURITY DEFINER function is a preplanned sanctioned
> event, unlike an ad hoc unconstrained role change to superuser. And
> given the demo patch, we could see any SECURITY DEFINER function created
> by the superuser when it gets created in the log (which again is subject
> to auditing, alerts, etc.)
>
> Ultimately I would also like to see a general hook available which would
> fire for all changes to GUC settings, but I think this one is still very
> useful as it is highly targeted.
>
> Comments?
>
> Thanks,
>
> Joe

-- 
Jerry Sievers
Postgres DBA/Development Consulting
e: postgres.consult...@comcast.net
p: 312.241.7800


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

[HACKERS] Re: [HACKERS] Re: [HACKERS] Windows service is not starting so there’s message in log: FATAL: "could not create shared memory segment “Global/PostgreSQL.851401618”: Permission denied”

[Robert Haas]

On Thu, Oct 15, 2015 at 11:32 PM, Amit Kapila  wrote:
> Another some what related point is currently we are using random()
> function to ensure a unique name for dsm and it seems to me that
> it is always going to generate same number on first invocation (at least
> thats what happening on windows) due to which you are seeing the
> error.  Another options could be to append current pid or data directory
> path as we are doing in win32_shmem.c.  I think this could be an
> optimization which can be done in addition to the fix attached (we can
> do this as a separate patch as well, if we agreed to do anything).

Maybe we need to be using PostmasterRandom() rather than random() for
the control segment name.

But regardless, this patch isn't the right fix.
dsm_impl_op(DSM_OP_CREATE, ...) is supposed to return false in the
event of a segment-already-exists condition, without ereporting.  If
it hits any OTHER error, then it should ereport().  In the Windows
implementation, the code that caters to this is here:

if (errno == EEXIST)
{
/*
 * On Windows, when the segment already exists, a handle for the
 * existing segment is returned.  We must close it before
 * returning.  We don't do _dosmaperr here, so errno won't be
 * modified.
 */
CloseHandle(hmap);
return false;
}

Kyotaro Horiguchi's analysis seems to me to be going in about the
right direction.

-- 
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] checkpoint_segments upgrade recommendation?

[Peter Eisentraut]

The release notes say that checkpoint_segments has been replaced by
max_wal_size and min_wal_size, but there is no indication on how to
convert between the old and new settings.  I think a lot of people will
have checkpoint_segments delicately tuned, so we should at least give
them a hint on how to carry that forward in spirit.


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Fri, Oct 16, 2015 at 5:00 AM, Etsuro Fujita
 wrote:
> As for #2, I updated the patch, which uses a local join execution plan for
> an EvalPlanQual rechech, according to the comment from Robert [1]. Attached
> is an updated version of the patch.  This is a WIP patch, but it would be
> appreciated if I could get feedback earlier.

I don't see how this can be right.  You're basically just pretending
EPQ doesn't exist in the remote join case, which isn't going to work
at all.  Those bits of code that look at es_epqTuple, es_epqTupleSet,
and es_epqScanDone are not optional.  You can't just skip over those
as if they don't matter.

Again, the root of the problem here is that the EPQ machinery provides
1 slot per RTI, and it uses scanrelid to figure out which EPQ slot is
applicable for a given scan node.  Here, we have scanrelid == 0, so it
gets confused.  But it's not the case that a pushed-down join has NO
scanrelid.  It actually has, in effect, *multiple* scanrelids.  So we
should pick any one of those, say the lowest-numbered one, and use
that to decide which EPQ slot to use.  The attached patch shows
roughly what I have in mind, although this is just crap code to
demonstrate the basic idea and doesn't pretend to adjust everything
that needs fixing.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/executor/execScan.c b/src/backend/executor/execScan.c
index a96e826..5c4a4f4 100644
--- a/src/backend/executor/execScan.c
+++ b/src/backend/executor/execScan.c
@@ -26,6 +26,32 @@
 static bool tlist_matches_tupdesc(PlanState *ps, List *tlist, Index varno, TupleDesc tupdesc);
 
 
+static Index
+get_proxy_scanrelid(ScanState *node)
+{
+	Scan	*scan = (Scan *) node->ps.plan;
+
+	Assert(scan->scanrelid == 0);
+
+	switch (nodeTag(scan))
+	{
+		case T_ForeignScan:
+			{
+ForeignScan *fs = (ForeignScan *) scan;
+return bms_first_member(fs->fs_relids);
+			}
+
+		case T_CustomScan:
+			{
+CustomScan *cs = (CustomScan *) scan;
+return bms_first_member(cs->custom_relids);
+			}
+
+		default:
+			elog(FATAL, "unexpected node type: %d", (int) nodeTag(scan));
+	}
+}
+
 /*
  * ExecScanFetch -- fetch next potential tuple
  *
@@ -49,6 +75,8 @@ ExecScanFetch(ScanState *node,
 		 */
 		Index		scanrelid = ((Scan *) node->ps.plan)->scanrelid;
 
+		if (scanrelid == 0)
+			scanrelid = get_proxy_scanrelid(node);
 		Assert(scanrelid > 0);
 		if (estate->es_epqTupleSet[scanrelid - 1])
 		{
@@ -347,6 +375,8 @@ ExecScanReScan(ScanState *node)
 	{
 		Index		scanrelid = ((Scan *) node->ps.plan)->scanrelid;
 
+		if (scanrelid == 0)
+			scanrelid = get_proxy_scanrelid(node);
 		Assert(scanrelid > 0);
 
 		estate->es_epqScanDone[scanrelid - 1] = false;

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

Re: [HACKERS] remaining open items

[Robert Haas]

On Thu, Oct 15, 2015 at 12:22 PM, Alvaro Herrera
 wrote:
> Robert Haas wrote:
>> - Oversize item computation needs more testing (c.f. ereport(ERROR)
>> calls in brin_getinsertbuffer).  This is pretty vague, and there's no
>> thread linked.  If there's a stability issue here, presumably it falls
>> to Alvaro to fix it.  But I don't know who added this item or what
>> really needs to be done.
>
> I added it, sorry it's vague.  It means that I should test with
> values of increasing size and see if all the errors are correctly
> covered, i.e. we don't get a PANIC because of a failure in PageAddItem.

So, are you going to do that?

>> - DDL deparsing testing module should have detected that transforms
>> were not supported, but it failed to notice that.  There's no thread
>> linked to this one either, but the description of the issue seems
>> clear enough.  Alvaro, any chance that you can, as the comment says,
>> whack it until it does?
>
> I've been looking at this on and off, without anything useful to share
> yet.  One approach that was suggested (which I don't like much, but I
> admit is a possible approach) is that we just need to remain vigilant
> that all features that add DDL properly test the event trigger side of
> it, just as we remain vigilant about pg_dump support without having any
> explicit test that it works.

I really, really hope that's not where we end up.  Just shoot me now.

>> - Strange behavior on track_commit_timestamp.  As I've said on the
>> thread, I think that the idea that the redo routines care about the
>> value of the GUC at the time redo is performed (rather than at the
>> time redo is generated), is broken.  Fujii's latest post provides some
>> examples of how this creates weird results.  I really think this
>> should be changed.
>
> We have discussed this; Petr is going to post a patch shortly.

Cool.

> The other item on me is the documentation patch by Emre Hasegeli for
> usage of the inclusion opclass framework in BRIN.  I think it needs some
> slight revision by some native English speaker and I'm not completely in
> love with the proposed third column in the table it adds, but otherwise
> is factually correct as far as I can tell.

I'm not clear whether you are asking for help with this, or ...?

-- 
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] buildfarm failures on crake and sittella

[Andrew Dunstan]

On 10/16/2015 11:13 AM, Robert Haas wrote:

Andrew,

The FileTextArrayFDW-build failure on crake, and the RedisFDW-build
failure on sittella, are expected results of my commit
5043193b78919a1bd144563aadc2f7f726549913.  If those FDWs do not push
quals down, they just need to be updated to pass an additional NIL
argument to make_foreignscan().  If they do, they need to pass a list
of the pushed-down quals in that new argument, as described in the
above-mentioned commit.

Can you either update those FDWs or disable those builds for now so
the BF is happy?



I expect to get to it tomorrow.

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] proposal: DROP DATABASE variant that kills active sessions

[Robert Haas]

On Fri, Oct 16, 2015 at 6:22 AM, Pavel Stehule  wrote:
> in GoodData we have this feature implemented - little bit different named -
> DROP DATABASE FORCE
>
> It is useful in complex environment with mix of pooled and not pooled
> connections - and in our environment - about 2K databases per server with
> lot of dropped / created databases per server / per day.
>
> I can publish this patch, if there will be any interest.

I think this would be a useful feature.  What would one do about
prepared transactions?

-- 
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] checkpoint_segments upgrade recommendation?

[Robert Haas]

On Fri, Oct 16, 2015 at 2:52 PM, Peter Eisentraut  wrote:
> The release notes say that checkpoint_segments has been replaced by
> max_wal_size and min_wal_size, but there is no indication on how to
> convert between the old and new settings.  I think a lot of people will
> have checkpoint_segments delicately tuned, so we should at least give
> them a hint on how to carry that forward in spirit.

Yeah, it would be nice to have some guidance about that.  But do we
know what the guidance should be?

-- 
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] Parallel Seq Scan

[Robert Haas]

On Fri, Oct 16, 2015 at 2:29 AM, Amit Kapila  wrote:
>> Yeah, but I think the scenario is legitimate.  When a query gets run
>> from within PL/pgsql, parallelism is an option, at least as we have
>> the code today.  So if a Gather were present, and the query used a
>> parameter, then you could have this issue.  For example:
>>
>> SELECT * FROM bigtable WHERE unindexed_column = some_plpgsql_variable;
>>
>
> I don't think for such statements the control flow will set up an unshared
> param list.  I have tried couple of such statements [1] and found that
> always such parameters are set up by setup_param_list().  I think there
> are only two possibilities which could lead to setting up of unshared
> params:
>
> 1. Usage of cursors - This is already prohibited for parallel-mode.
> 2. Usage of read-write-param - This only happens for expressions like
> x := array_append(x, foo) (Refer exec_check_rw_parameter()).  Read-write
> params are not used for SQL statements. So this also won't be used for
> parallel-mode
>
> There is a chance that I might be missing some case where unshared
> params will be required for parallel-mode (as of today), but if not then
> I think we can live without current changes.

*shrug*

The gather-test stuff isn't failing for no reason.  Either PL/pgsql
shouldn't be passing CURSOR_OPT_PARALLEL_OK, or having a parallel plan
get generated there should work.  There's not a third option.

-- 
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: DROP DATABASE variant that kills active sessions

[Fabrízio de Royes Mello]

On Fri, Oct 16, 2015 at 4:12 PM, Robert Haas  wrote:
>
> On Fri, Oct 16, 2015 at 6:22 AM, Pavel Stehule 
wrote:
> > in GoodData we have this feature implemented - little bit different
named -
> > DROP DATABASE FORCE
> >
> > It is useful in complex environment with mix of pooled and not pooled
> > connections - and in our environment - about 2K databases per server
with
> > lot of dropped / created databases per server / per day.
> >
> > I can publish this patch, if there will be any interest.
>
> I think this would be a useful feature.  What would one do about
> prepared transactions?
>

Isn't "rollback all prepared" before an option?

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] remaining open items

[Alvaro Herrera]

Robert Haas wrote:
> On Thu, Oct 15, 2015 at 12:22 PM, Alvaro Herrera
>  wrote:
> > Robert Haas wrote:
> >> - Oversize item computation needs more testing (c.f. ereport(ERROR)
> >> calls in brin_getinsertbuffer).  This is pretty vague, and there's no
> >> thread linked.  If there's a stability issue here, presumably it falls
> >> to Alvaro to fix it.  But I don't know who added this item or what
> >> really needs to be done.
> >
> > I added it, sorry it's vague.  It means that I should test with
> > values of increasing size and see if all the errors are correctly
> > covered, i.e. we don't get a PANIC because of a failure in PageAddItem.
> 
> So, are you going to do that?

Yes.

> > I've been looking at this on and off, without anything useful to share
> > yet.  One approach that was suggested (which I don't like much, but I
> > admit is a possible approach) is that we just need to remain vigilant
> > that all features that add DDL properly test the event trigger side of
> > it, just as we remain vigilant about pg_dump support without having any
> > explicit test that it works.
> 
> I really, really hope that's not where we end up.  Just shoot me now.

Me too.  I'm going to do something about this, though I can't promise a
deadline.

> > The other item on me is the documentation patch by Emre Hasegeli for
> > usage of the inclusion opclass framework in BRIN.  I think it needs some
> > slight revision by some native English speaker and I'm not completely in
> > love with the proposed third column in the table it adds, but otherwise
> > is factually correct as far as I can tell.
> 
> I'm not clear whether you are asking for help with this, or ...?

I enlisted the help of Ian Barwick for this one.

-- 
Á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] pageinspect patch, for showing tuple data

[Nikolay Shaplov]

So what's next?
We need something else to discuss?
We need somebody else's opinion to rule this out?
Or it's ready to commit, and just not marked this way?

I am going to make report based on this patch in Vienna. It would be
nice to have it committed until then :)

On 02.10.2015 07:10, Michael Paquier wrote:
> On Thu, Oct 1, 2015 at 8:13 PM, Nikolay Shaplov wrote:
>> В письме от 30 сентября 2015 13:49:00 пользователь Michael Paquier
> написал:
>>> - errmsg("input page too small (%d
> bytes)",
>>> raw_page_size)));
>>> +errmsg("input page too small (%d
>>> bytes)", raw_page_size)));
>>> Please be careful of spurious diffs. Those just make the life of
> committers
>>> more difficult than it already is.
>> Oh, that's not diff. That's I've fixed indent on the code I did not write
> :-)
>
> pgindent would have taken care of that if needed. There is no need to add
> noise in the code for this patch.
>
>>> + 
>>> + General idea about output columns: lp_*
>>> attributes
>>> + are about where tuple is located inside the page;
>>> + t_xmin, t_xmax,
>>> + t_field3, t_ctid are
> about
>>> + visibility of this tuplue inside or outside of the transaction;
>>> + t_infomask2, t_infomask
> has
>>> some
>>> + information about properties of attributes stored in tuple data.
>>> + t_hoff sais where tuple data begins and
>>> + t_bits sais which attributes are NULL and
> which
>>> are
>>> + not. Please notice that t_bits here is not an actual value that is
>>> stored
>>> + in tuple data, but it's text representation  with '0' and '1'
>>> charactrs.
>>> + 
>>> I would remove that as well. htup_details.h contains all this
> information.
>> Made it even more shorter. Just links and comments about representation of
>> t_bits.
> I would completely remove this part.
>
>> There also was a bug in original pageinspect, that did not get t_bits
> right
>> when there was OID in the tuple.  I've fixed it too.
> Aha. Good catch! By looking at HeapTupleHeaderGetOid if the tuple has an
> OID it will be stored at the end of HeapTupleHeader and t_hoff includes it,
> so bits_len is definitely larger of 4 bytes should an OID be present.
>if (tuphdr->t_infomask & HEAP_HASNULL)
>{
> -   bits_len = tuphdr->t_hoff -
> -   offsetof(HeapTupleHeaderData, t_bits);
> +  int bits_len =
> +  ((tuphdr->t_infomask2 & HEAP_NATTS_MASK)/8 +
> 1)*8;
>
> values[11] = CStringGetTextDatum(
> -   bits_to_text(tuphdr->t_bits,
> bits_len * 8));
> +  bits_to_text(tuphdr->t_bits,
> bits_len));
> }
> And here is what you are referring to in your patch. I think that we should
> instead check for HEAP_HASOID and reduce bits_len by the size of Oid should
> one be present. As this is a bug that applies to all the existing versions
> of Postgres it would be good to extract it as a separate patch and then
> apply your own patch on top of it instead of including in your feature.
> Attached is a patch, this should be applied down to 9.0 I guess. Could you
> rebase your patch on top of it?
>
>> Here is next patch in attachment.
> Cool, thanks.
>
> -test=# SELECT * FROM heap_page_items(get_raw_page('pg_class', 0));
> +
> +test=# select * from heap_page_items(get_raw_page('pg_range', 0));
> This example in the docs is far too long in character length... We should
> get that reduced.
>
> +  Please notice that t_bits in tuple header
> structure
> +  is a binary bitmap, but heap_page_items returns
> +  t_bits as human readable text representation of
> +  original t_bits bitmap.
> This had better remove the mention to "please notice". Still as this is
> already described in htup_details.h there is no real point to describe it
> again here: that's a bitmap of NULLs.
>
>



-- 
Sent 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: DROP DATABASE variant that kills active sessions

[Andres Freund]

On 2015-10-16 16:32:25 -0300, Fabrízio de Royes Mello wrote:
> On Fri, Oct 16, 2015 at 4:12 PM, Robert Haas  wrote:
> > I think this would be a useful feature.  What would one do about
> > prepared transactions?
> >
> 
> Isn't "rollback all prepared" before an option?

Not necessarily - what if shared objects are affected by the
transaction?

I think it's fine to just not support FORCE in that case - I don't think
it's all that common.

Greetings,

Andres Freund


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

Re: [HACKERS] plpython is broken for recursive use

[Tom Lane]

I wrote:
> Anyway, the real problem here is the decision to pass procedure arguments
> by assigning them to keys in the global dict.  That is just brain-dead,
> both because it means that recursive calls can't possibly work and because
> it creates the bizarre scoping behavior mentioned in
> http://www.postgresql.org/docs/devel/static/plpython-funcs.html
> I suppose we cannot change that in back branches for fear of creating
> subtle compatibility problems, but surely we can do better going forward?

I looked into this a little more. The Python function we use to execute
plpython code, PyEval_EvalCode, has arguments to supply both a global and
a local namespace dict.  Right now we just pass proc->globals for both,
which sounds and is weird.  However, what we're actually executing in
that context is just an argument-less function call, eg
"__plpython_procedure_foo_nnn()".  So I believe that no use of the passed
"local" namespace actually happens: the user-supplied code executes with
proc->globals as its global namespace and some transient dict created by
the function call action as a local namespace.

This seems like a very Rube-Goldbergian way of setting up a local
namespace for the user-defined code.  I think perhaps what we should do
is:

1. Compile the user-supplied code directly into a code object, without
wrapping it in a "def".  (Hence, PLy_procedure_munge_source goes away
entirely, which would be nice.)  Forget about generating a code object
containing a call, too.

2. During function call startup, create a dict to be the local namespace
for that call.  Shove the argument values into entries in that dict, not
the global one.

3. Run the user-supplied code directly with PyEval_EvalCode, passing
proc->globals as its global namespace and the transient dict as its
local namespace.

This would fix the problem with recursive calls and probably be a lot
cleaner as well.  It would not be 100% backwards compatible, because
the technique shown in
http://www.postgresql.org/docs/devel/static/plpython-funcs.html
of declaring an argument "global" would not work, nor be necessary,
anymore.  That does not seem like a bad thing, but it would be a reason
not to back-patch.

Thoughts?

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

[HACKERS] shm_mq fix for non-blocking mode

[Robert Haas]

The shm_mq code handles blocking mode and non-blocking mode
asymmetrically in a couple of places, with the unfortunate result that
if you are using non-blocking mode, and your counterparty dies before
attaching the queue, operations on the queue continue to return
SHM_MQ_WOULD_BLOCK instead of, as they should, returning
SHM_MQ_DETACHED.  The attached patch fixes the problem.  Thanks to my
colleague Rushabh Lathia for helping track this down.

(There's are some further bugs in this area outside the shm_mq code
... but I'm still trying to figure out exactly what they are and what
we should do about them.  This much, however, seems clear-cut.)

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/storage/ipc/shm_mq.c b/src/backend/storage/ipc/shm_mq.c
index 80956ce..61f9298 100644
--- a/src/backend/storage/ipc/shm_mq.c
+++ b/src/backend/storage/ipc/shm_mq.c
@@ -142,6 +142,8 @@ static shm_mq_result shm_mq_send_bytes(shm_mq_handle *mq, Size nbytes,
   const void *data, bool nowait, Size *bytes_written);
 static shm_mq_result shm_mq_receive_bytes(shm_mq *mq, Size bytes_needed,
 	 bool nowait, Size *nbytesp, void **datap);
+static bool shm_mq_counterparty_gone(volatile shm_mq *mq,
+		 BackgroundWorkerHandle *handle);
 static bool shm_mq_wait_internal(volatile shm_mq *mq, PGPROC *volatile * ptr,
 	 BackgroundWorkerHandle *handle);
 static uint64 shm_mq_get_bytes_read(volatile shm_mq *mq, bool *detached);
@@ -499,6 +501,8 @@ shm_mq_receive(shm_mq_handle *mqh, Size *nbytesp, void **datap, bool nowait)
 	{
 		if (nowait)
 		{
+			if (shm_mq_counterparty_gone(mq, mqh->mqh_handle))
+return SHM_MQ_DETACHED;
 			if (shm_mq_get_sender(mq) == NULL)
 return SHM_MQ_WOULD_BLOCK;
 		}
@@ -794,6 +798,11 @@ shm_mq_send_bytes(shm_mq_handle *mqh, Size nbytes, const void *data,
 			 */
 			if (nowait)
 			{
+if (shm_mq_counterparty_gone(mq, mqh->mqh_handle))
+{
+	*bytes_written = sent;
+	return SHM_MQ_DETACHED;
+}
 if (shm_mq_get_receiver(mq) == NULL)
 {
 	*bytes_written = sent;
@@ -948,6 +957,45 @@ shm_mq_receive_bytes(shm_mq *mq, Size bytes_needed, bool nowait,
 }
 
 /*
+ * Test whether a counterparty who may not even be alive yet is definitely gone.
+ */
+static bool
+shm_mq_counterparty_gone(volatile shm_mq *mq, BackgroundWorkerHandle *handle)
+{
+	bool	detached;
+	pid_t	pid;
+
+	/* Acquire the lock just long enough to check the pointer. */
+	SpinLockAcquire(>mq_mutex);
+	detached = mq->mq_detached;
+	SpinLockRelease(>mq_mutex);
+
+	/* If the queue has been detached, counterparty is definitely gone. */
+	if (detached)
+		return true;
+
+	/* If there's a handle, check worker status. */
+	if (handle != NULL)
+	{
+		BgwHandleStatus status;
+
+		/* Check for unexpected worker death. */
+		status = GetBackgroundWorkerPid(handle, );
+		if (status != BGWH_STARTED && status != BGWH_NOT_YET_STARTED)
+		{
+			/* Mark it detached, just to make it official. */
+			SpinLockAcquire(>mq_mutex);
+			mq->mq_detached = true;
+			SpinLockRelease(>mq_mutex);
+			return true;
+		}
+	}
+
+	/* Counterparty is not definitively gone. */
+	return false;
+}
+
+/*
  * This is used when a process is waiting for its counterpart to attach to the
  * queue.  We exit when the other process attaches as expected, or, if
  * handle != NULL, when the referenced background process or the postmaster

-- 
Sent 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: DROP DATABASE variant that kills active sessions

[Pavel Stehule]

2015-10-16 21:12 GMT+02:00 Robert Haas :

> On Fri, Oct 16, 2015 at 6:22 AM, Pavel Stehule 
> wrote:
> > in GoodData we have this feature implemented - little bit different
> named -
> > DROP DATABASE FORCE
> >
> > It is useful in complex environment with mix of pooled and not pooled
> > connections - and in our environment - about 2K databases per server with
> > lot of dropped / created databases per server / per day.
> >
> > I can publish this patch, if there will be any interest.
>
> I think this would be a useful feature.  What would one do about
> prepared transactions?
>

It doesn't solve it - GoodData doesn't use it - so I didn't solve this
question - it emulates manual maintenance. In our solution can be some
opened issues.

Patch attached

Regards

Pavel



>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
*** ./src/backend/commands/dbcommands.c.orig	2015-02-16 18:29:19.296790829 +0100
--- ./src/backend/commands/dbcommands.c	2015-02-17 10:59:50.999374567 +0100
***
*** 456,462 
  		 * to wait 5 sec. We try to raise warning after 1 minute and and raise
  		 * a error after 5 minutes.
  		 */
! 		if (!CountOtherDBBackends(src_dboid, , , true))
  			break;
  
  		if (loops++ % 12 == 0)
--- 456,462 
  		 * to wait 5 sec. We try to raise warning after 1 minute and and raise
  		 * a error after 5 minutes.
  		 */
! 		if (!CountOtherDBBackends(src_dboid, , , true, false))
  			break;
  
  		if (loops++ % 12 == 0)
***
*** 787,793 
   * DROP DATABASE
   */
  void
! dropdb(const char *dbname, bool missing_ok)
  {
  	Oid			db_id;
  	bool		db_istemplate;
--- 787,793 
   * DROP DATABASE
   */
  void
! dropdb(const char *dbname, bool missing_ok, bool force)
  {
  	Oid			db_id;
  	bool		db_istemplate;
***
*** 795,800 
--- 795,801 
  	HeapTuple	tup;
  	int			notherbackends;
  	int			npreparedxacts;
+ 	int		loops = 0;
  
  	/*
  	 * Look up the target database's OID, and get exclusive lock on it. We
***
*** 864,875 
  	 *
  	 * As in CREATE DATABASE, check this after other error conditions.
  	 */
! 	if (CountOtherDBBackends(db_id, , , false))
! 		ereport(ERROR,
! (errcode(ERRCODE_OBJECT_IN_USE),
!  errmsg("database \"%s\" is being accessed by other users",
! 		dbname),
!  errdetail_busy_db(notherbackends, npreparedxacts)));
  
  	/*
  	 * Remove the database's tuple from pg_database.
--- 865,897 
  	 *
  	 * As in CREATE DATABASE, check this after other error conditions.
  	 */
! 	for (;;)
! 	{
! 		/*
! 		 * CountOtherDBBackends check usage of database by other backends and try
! 		 * to wait 5 sec. We try to raise warning after 1 minute and and raise
! 		 * a error after 5 minutes.
! 		 */
! 		if (!CountOtherDBBackends(db_id, , , true, force))
! 			break;
! 
! 		if (force && loops++ % 12 == 0)
! 			ereport(WARNING,
! 	(errcode(ERRCODE_OBJECT_IN_USE),
! errmsg("source database \"%s\" is being accessed by other users",
! 	   dbname),
! 	 errdetail_busy_db(notherbackends, npreparedxacts)));
! 
! 		/* without "force" flag raise exception immediately, or after 5 minutes */
! 		if (!force || loops % 60 == 0)
! 			ereport(ERROR,
! 	(errcode(ERRCODE_OBJECT_IN_USE),
! errmsg("source database \"%s\" is being accessed by other users",
! 	   dbname),
! 	 errdetail_busy_db(notherbackends, npreparedxacts)));
! 
! 		CHECK_FOR_INTERRUPTS();
! 	}
  
  	/*
  	 * Remove the database's tuple from pg_database.
***
*** 1007,1013 
  	 *
  	 * As in CREATE DATABASE, check this after other error conditions.
  	 */
! 	if (CountOtherDBBackends(db_id, , , false))
  		ereport(ERROR,
  (errcode(ERRCODE_OBJECT_IN_USE),
   errmsg("database \"%s\" is being accessed by other users",
--- 1029,1035 
  	 *
  	 * As in CREATE DATABASE, check this after other error conditions.
  	 */
! 	if (CountOtherDBBackends(db_id, , , false, false))
  		ereport(ERROR,
  (errcode(ERRCODE_OBJECT_IN_USE),
   errmsg("database \"%s\" is being accessed by other users",
***
*** 1132,1138 
  	 *
  	 * As in CREATE DATABASE, check this after other error conditions.
  	 */
! 	if (CountOtherDBBackends(db_id, , , false))
  		ereport(ERROR,
  (errcode(ERRCODE_OBJECT_IN_USE),
   errmsg("database \"%s\" is being accessed by other users",
--- 1154,1160 
  	 *
  	 * As in CREATE DATABASE, check this after other error conditions.
  	 */
! 	if (CountOtherDBBackends(db_id, , , false, false))
  		ereport(ERROR,
  (errcode(ERRCODE_OBJECT_IN_USE),
   errmsg("database \"%s\" is being accessed by other users",
*** ./src/backend/nodes/copyfuncs.c.orig	2015-02-17 10:34:40.385775458 +0100
--- ./src/backend/nodes/copyfuncs.c	2015-02-17 10:35:31.041859691 +0100
***
*** 3175,3180 
--- 3175,3181 
  
  	COPY_STRING_FIELD(dbname);
  	COPY_SCALAR_FIELD(missing_ok);
+ 	

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2015-10-16 8:12 GMT+02:00 Craig Ringer :

> On 16 October 2015 at 02:47, Pavel Stehule 
> wrote:
>
> >  postgres=# do $$
> > x = plpy.SPIError('Nazdarek');
> > x.spidata = (100, "Some detail", "some hint", None, None);
> > raise x;
> > $$ language plpythonu;
>
> Shouldn't that look more like
>
> raise plpy.SPIError(msg="Message", sqlstate="0P001", hint="Turn it on
> and off again") ?
>
> Keyword args are very much the norm for this sort of thing. I recall
> them being pretty reasonable to deal with in the CPython API too, but
> otherwise a trivial Python wrapper in the module can easily adapt the
> interface.
>

I wrote a constructor for SPIError with keyword parameters support - see
attached patch

The code is working

 postgres=# do $$
raise plpy.SPIError("pokus",hint = "some info");
$$ language plpythonu;
ERROR:  plpy.SPIError: pokus
HINT:  some info
CONTEXT:  Traceback (most recent call last):
  PL/Python anonymous code block, line 2, in 
raise plpy.SPIError("pokus",hint = "some info");
PL/Python anonymous code block

but the implementation is pretty ugly :( - I didn't write C extensions for
Python before, and the extending exception class with some methods isn't
well supported and well documented.

Any help is welcome

Regards

Pavel


>
>
> --
>  Craig Ringer   http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services
>
diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out
new file mode 100644
index 1f52af7..c9b5e69
*** a/src/pl/plpython/expected/plpython_error.out
--- b/src/pl/plpython/expected/plpython_error.out
*** EXCEPTION WHEN division_by_zero THEN
*** 408,413 
--- 408,420 
  	-- NOOP
  END
  $$ LANGUAGE plpgsql;
+ ERROR:  spiexceptions.DivisionByZero: None
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python function "plpy_raise_spiexception", line 2, in 
+ raise plpy.spiexceptions.DivisionByZero()
+ PL/Python function "plpy_raise_spiexception"
+ SQL statement "SELECT plpy_raise_spiexception()"
+ PL/pgSQL function inline_code_block line 3 at SQL statement
  /* setting a custom sqlstate should be handled
   */
  CREATE FUNCTION plpy_raise_spiexception_override() RETURNS void AS $$
*** EXCEPTION WHEN SQLSTATE 'SILLY' THEN
*** 422,424 
--- 429,487 
  	-- NOOP
  END
  $$ LANGUAGE plpgsql;
+ ERROR:  spiexceptions.DivisionByZero: None
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python function "plpy_raise_spiexception_override", line 4, in 
+ raise exc
+ PL/Python function "plpy_raise_spiexception_override"
+ SQL statement "SELECT plpy_raise_spiexception_override()"
+ PL/pgSQL function inline_code_block line 3 at SQL statement
+ CREATE FUNCTION nested_error_ereport() RETURNS text
+ 	AS
+ 'def fun1():
+ 	raise plpy.SPIError("HiHi");
+ 
+ def fun2():
+ 	fun1()
+ 
+ def fun3():
+ 	fun2()
+ 
+ fun3()
+ return "not reached"
+ '
+ 	LANGUAGE plpythonu;
+ SELECT nested_error_ereport();
+ ERROR:  plpy.SPIError: HiHi
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python function "nested_error_ereport", line 10, in 
+ fun3()
+   PL/Python function "nested_error_ereport", line 8, in fun3
+ fun2()
+   PL/Python function "nested_error_ereport", line 5, in fun2
+ fun1()
+   PL/Python function "nested_error_ereport", line 2, in fun1
+ raise plpy.SPIError("HiHi");
+ PL/Python function "nested_error_ereport"
+ \set VERBOSITY verbose
+ do $$
+ x = plpy.SPIError("pokus",hint = "some info", sqlstate='AA234'); raise x;
+ $$ language plpythonu;
+ ERROR:  AA234: plpy.SPIError: pokus
+ HINT:  some info
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 2, in 
+ x = plpy.SPIError("pokus",hint = "some info", sqlstate='AA234'); raise x;
+ PL/Python anonymous code block
+ LOCATION:  PLy_elog, plpy_elog.c:119
+ do $$
+ raise plpy.SPIError("pokus",hint = "some info", sqlstate='AA234');
+ $$ language plpythonu;
+ ERROR:  AA234: plpy.SPIError: pokus
+ HINT:  some info
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 2, in 
+ raise plpy.SPIError("pokus",hint = "some info", sqlstate='AA234');
+ PL/Python anonymous code block
+ LOCATION:  PLy_elog, plpy_elog.c:119
+ \set VERBOSITY default
diff --git a/src/pl/plpython/plpy_elog.c b/src/pl/plpython/plpy_elog.c
new file mode 100644
index 15406d6..6825732
*** a/src/pl/plpython/plpy_elog.c
--- b/src/pl/plpython/plpy_elog.c
*** PyObject   *PLy_exc_fatal = NULL;
*** 21,29 
  PyObject   *PLy_exc_spi_error = NULL;
  
  
- static void PLy_traceback(char **xmsg, char **tbmsg, int *tb_depth);
  static void PLy_get_spi_error_data(PyObject *exc, int *sqlerrcode, char **detail,
! 	   char **hint, char **query, int *position);
  static char *get_source_line(const char *src, int lineno);
  
  
--- 21,30 
  PyObject   *PLy_exc_spi_error = NULL;
  
  
  static 

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2015-10-16 8:12 GMT+02:00 Craig Ringer :

> On 16 October 2015 at 02:47, Pavel Stehule 
> wrote:
>
> >  postgres=# do $$
> > x = plpy.SPIError('Nazdarek');
> > x.spidata = (100, "Some detail", "some hint", None, None);
> > raise x;
> > $$ language plpythonu;
>
> Shouldn't that look more like
>
> raise plpy.SPIError(msg="Message", sqlstate="0P001", hint="Turn it on
> and off again") ?
>

postgres=# do $$
raise plpy.SPIError(msg="Message", sqlstate="0P001", hint="Turn it on and
off again");
$$ language plpythonu;
ERROR:  TypeError: SPIError does not take keyword arguments
CONTEXT:  Traceback (most recent call last):
  PL/Python anonymous code block, line 2, in 
raise plpy.SPIError(msg="Message", sqlstate="0P001", hint="Turn it on
and off again");
PL/Python anonymous code block
Time: 1.193 ms



>
> Keyword args are very much the norm for this sort of thing. I recall
> them being pretty reasonable to deal with in the CPython API too, but
> otherwise a trivial Python wrapper in the module can easily adapt the
> interface.
>
>
postgres=# do $$
class cSPIError(plpy.SPIError):
def __init__( self, message, detail = None, hint = None):
self.spidata = (0, detail, hint, None, None,)
self.args = ( message, )

x = cSPIError('Nazdarek', hint = 'some hint')
raise x
$$ language plpythonu;
ERROR:  cSPIError: Nazdarek
HINT:  some hint
CONTEXT:  Traceback (most recent call last):
  PL/Python anonymous code block, line 8, in 
raise x
PL/Python anonymous code block

This code is working, so it needs explicit constructor for class SPIError

Regards

Pavel


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

[HACKERS] Re: [HACKERS] Re: [HACKERS] Windows service is not starting so there’s message in log: FATAL: "could not create shared memory segment “Global/PostgreSQL.851401618”: Permission denied”

[Amit Kapila]

On Fri, Oct 16, 2015 at 12:16 PM, Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp> wrote:
> This is wrong, current code does well for this case. I should
> broke the code during investigating the problem.
>
> > > So, to make the windows version behave as the same,
> > > dsm_impl_windows should return false if GetLastError() ==
> > > ERROR_ALREADY_EXISTS regardless of hmap is valid. The current
> > > behavior is wrong because two or more postmaster *share* the same
> > > DSM segment instead of having their own segments.
>
> > > The patch attached will fix *both of* the problems.
>
> So, the patch fixes only the "Permission denied" case.
>

Why do you think it is bad to display even log for "Permission denied"
case?
It seems all other implementations does the same and I think it is
useful information and we should log it.  Don't you think the patch
sent by me is good enough to fix the reported issue?


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

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

[Michael Paquier]

On Wed, Oct 14, 2015 at 5:58 PM, Beena Emerson  wrote:
>
>
> On Wed, Oct 14, 2015 at 10:38 AM, Michael Paquier
>  wrote:
>>
>> On Wed, Oct 14, 2015 at 3:02 AM, Masahiko Sawada wrote:
>>
>> > It would be good even if there are some restriction such as the
>> > nesting level, the group setting.
>> > The another new approach that I came up with is,
>> > * Add new parameter synchronous_replication_method (say s_r_method)
>> > which can have two names: 'priority', 'quorum'
>> > * If s_r_method = 'priority', the value of s_s_names (e.g. 'n1,n2,n3')
>> > is handled using priority. It's same as '[n1,n2,n3]' in dedicated
>> > language.
>> > * If s_r_method = 'quorum', the value of s_s_names is handled using
>> > quorum commit, It's same as '(n1,n2,n3)' in dedicated language.
>> > * Setting of synchronous_standby_names is same as today. That is, the
>> > storing the nesting value is not supported.
>> > * If we want to support more complex syntax like what we are
>> > discussing, we can add the new value to s_r_method, for example
>> > 'complex', 'json'.
>>
>> If we go that path, I think that we still would need an extra
>> parameter to control the number of nodes that need to be taken from
>> the set defined in s_s_names whichever of quorum or priority is used.
>> Let's not forget that in the current configuration the first node
>> listed in s_s_names and *connected* to the master will be used to
>> acknowledge the commit.
>
>
> Would it be better to just use a simple language instead of 3 different
> parameters?
>
> s_s_names = 2[X,Y,Z]  # 2 priority
> s_s_names = 1(A,B,C) # 1 quorum
> s_s_names = R,S,T # default behavior: 1 priorty?

Yeah, the main use case for this feature would just be that for most users:
s_s_names = 2[dc1_standby,1(dc2_standby1, dc2_standby2)]
Meaning that we wait for dc1_standby, which is a standby on data
center 1, and one of the dc2_standby* set which are standbys in data
center 2.
So the following minimal characteristics would be needed:
- support for priority selectivity for N nodes
- support for quorum selectivity for N nodes
- support for nested set of nodes, at least 2 level deep.
The requirement to define a group of nodes also would not be needed.
If we have that, I would say that we already do better than OrXXXe and
MyXXL, to cite two of them. And if we can get that for 9.6 or even
9.7, that would be really great.
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] [PATCH v3] GSSAPI encryption support

[Craig Ringer]

On 16 October 2015 at 01:07, Robbie Harwood  wrote:

> The short - and probably most important - answer is that no, I haven't
> tested it, and it would be difficult for me to do so quickly.

IIRC it's pretty easy to fire up AWS instances that're primary domain
controllers, and then join a Pg box to them with Kerberos. I realise
that's different to having the time and desire to do it, though.

> Looking at
> http://www.postgresql.org/docs/9.4/static/protocol-message-formats.html
> suggests that SSPI follows a separate codepath from the GSS code;
> certainly it's a different auth request, which means it shouldn't
> trigger the encryption path.

It's a different auth request, but the handling in be-auth.c is
co-mingled to handle the cases:

* We're compiled with Kerberos, handling SSPI from Windows
* We're compiled with Kerberos, handling a GSSAPI request
* We're compiled with SSPI, and we're using Windows SSPI to handle a
GSSAPI auth request
* We're compiled with SSPI, and we're using it to handle a SSP client request

SSPI is a wrapper. For Windows Domain members it carries
GSSAPI/Kerberos data with the spnego extension. (For local loopback it
does NTLM, but you don't need to care about that).

> There is no reason that using GSSAPI from, e.g., MIT Kerberos for
> Windows, should not work here.

Except that *nobody* does it. The EDB installer builds Pg with SSPI,
and that's what the overwhelming majority of Windows users use.

Personally I don't care if this patch doesn't add support for GSSAPI
encryption for sspi connections, only that it doesn't break them.

>  Even more broadly than that, GSSAPI is a
> RFC-standardized protocol with rigorously tested interop etc. etc.,
> though whether anyone outside of MIT cares about MIT Kerberos for
> Windows I have no idea.

It's maintained enough that AFAIK it works, and Pg supports compiling
with it. No idea if anyone tests it with Pg anymore.

> I think the important takeaway here is that I haven't actually changed
> how *authentication* works; these changes only affect GSSAPI
> post-authentication add encryption functions as defined by that
> specification.  So if the authentication was working before, and the
> GSSAPI implementation is following RFC, I would hope that this would
> work still.

Hope so.

I'll put this on my "hope to test it at some stage" list, but I don't
have a prebuilt environment for it and have a lot to get ready for the
next CF, so it'll be a while...


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


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

Re: [HACKERS] Parallel Seq Scan

[Amit Kapila]

On Thu, Oct 15, 2015 at 6:52 PM, Robert Haas  wrote:
>
> On Thu, Oct 15, 2015 at 7:00 AM, Amit Kapila 
wrote:
> >
> > From what I understood by looking at code in this area, I think the
check
> > params != estate->paramLI and code under it is required for parameters
> > that are setup by setup_unshared_param_list().  Now unshared params
> > are only created for Cursors and expressions that are passing a R/W
> > object pointer; for cursors we explicitly prohibit the parallel plan
> > generation
> > and I am not sure if it makes sense to generate parallel plans for
> > expressions
> > involving R/W object pointer, if we don't generate parallel plan where
> > expressions involve such parameters, then SerializeParamList() should
not
> > be affected by the check mentioned by you.  Is by anychance, this is
> > happening because you are testing by forcing gather node on top of
> > all kind of plans?
>
> Yeah, but I think the scenario is legitimate.  When a query gets run
> from within PL/pgsql, parallelism is an option, at least as we have
> the code today.  So if a Gather were present, and the query used a
> parameter, then you could have this issue.  For example:
>
> SELECT * FROM bigtable WHERE unindexed_column = some_plpgsql_variable;
>

I don't think for such statements the control flow will set up an unshared
param list.  I have tried couple of such statements [1] and found that
always such parameters are set up by setup_param_list().  I think there
are only two possibilities which could lead to setting up of unshared
params:

1. Usage of cursors - This is already prohibited for parallel-mode.
2. Usage of read-write-param - This only happens for expressions like
x := array_append(x, foo) (Refer exec_check_rw_parameter()).  Read-write
params are not used for SQL statements. So this also won't be used for
parallel-mode

There is a chance that I might be missing some case where unshared
params will be required for parallel-mode (as of today), but if not then
I think we can live without current changes.

[1] -
1.
create or replace function parallel_func_params() returns integer
as $$
declare
param_val int;
ret_val int;
begin
 param_val := 1000;
 Select c1 into ret_val from t1 where c1 = param_val;
 return ret_val;
end;
$$ language plpgsql;

For such a query, it will go in setup_param_list()

2.
create or replace function parallel_func_params_1() returns integer
as $$
declare
param_val int;
ret_val int;
begin
 param_val := 1000;
 Execute 'Select count(c1) from t1 where c1 = $1' Into ret_val Using
param_val;
 return ret_val;
end;
$$ language plpgsql;


3.
create or replace function parallel_func_params_2() returns integer
as $$
declare
param_val int;
ret_val int;
row_var t1%ROWTYPE;
begin
 param_val := 1000;
 Select * into row_var from t1 where c1 = param_val;
 return ret_val;
end;
$$ language plpgsql;


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

Re: [HACKERS] Re: [HACKERS] Windows service is not starting so there’s message in log: FATAL: "could not create shared memory segment “Global/PostgreSQL.851401618”: Permission denied”

[Kyotaro HORIGUCHI]

This is wrong, current code does well for this case. I should
broke the code during investigating the problem.

> > So, to make the windows version behave as the same,
> > dsm_impl_windows should return false if GetLastError() ==
> > ERROR_ALREADY_EXISTS regardless of hmap is valid. The current
> > behavior is wrong because two or more postmaster *share* the same
> > DSM segment instead of having their own segments.

> > The patch attached will fix *both of* the problems.

So, the patch fixes only the "Permission denied" case.

regards,


At Fri, 16 Oct 2015 14:37:47 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20151016.143747.121283630.horiguchi.kyot...@lab.ntt.co.jp>
> > > Currently we are using error level as ERROR for creating dsm during
> > > postmaster startup which is not right and rather we should use error
> > > level as LOG.  Can you please try with the attached patch and see
> > > if the issue is fixed for you.
> > 
> > It seems somewhat strange. Looking into the create operation in
> > dms_impl_posix(), it should return false but not emit error when
> > the shared memory object already exists.
> > 
> > So, to make the windows version behave as the same,
> > dsm_impl_windows should return false if GetLastError() ==
> > ERROR_ALREADY_EXISTS regardless of hmap is valid. The current
> > behavior is wrong because two or more postmaster *share* the same
> > DSM segment instead of having their own segments.
> > 
> > On the other hand, in the case of GetLastError() ==
> > ERROR_ACCESS_DENIED, which occurs between postmasters exexuted as
> > service, it can reasonablly be assumed that the segment is
> > already created by someone else. I think this is no problem
> > because postgres processes don't need to access someone else's
> > segments.
> > 
> > Finally, the valid fix would be that make it return false if
> > GetLastError() == ERROR_ALREADY_EXISTS or ERROR_ACCESS_DENIED.
> > 
> > The patch attached will fix *both of* the problems.
> > 
> > 
> > regards,
> > 
> > 
> > At Fri, 16 Oct 2015 09:02:41 +0530, Amit Kapila  
> > wrote in 
> > 
> > > > I think that function dsm_impl_windows() with EACCES error should not
> > > > do ereport() with FATAL level. It works, but it is likely to make an
> > > > infinite loop if the user will receive EACCES error.
> > > >
> > > >
> > > Currently we are using error level as ERROR for creating dsm during
> > > postmaster startup which is not right and rather we should use error
> > > level as LOG.  Can you please try with the attached patch and see
> > > if the issue is fixed for you.
> > > 
> > > Another some what related point is currently we are using random()
> > > function to ensure a unique name for dsm and it seems to me that
> > > it is always going to generate same number on first invocation (at least
> > > thats what happening on windows) due to which you are seeing the
> > > error.  Another options could be to append current pid or data directory
> > > path as we are doing in win32_shmem.c.  I think this could be an
> > > optimization which can be done in addition to the fix attached (we can
> > > do this as a separate patch as well, if we agreed to do anything).
> 
> -- 
> Kyotaro Horiguchi
> NTT Open Source Software Center



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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Craig Ringer]

On 16 October 2015 at 02:47, Pavel Stehule  wrote:

>  postgres=# do $$
> x = plpy.SPIError('Nazdarek');
> x.spidata = (100, "Some detail", "some hint", None, None);
> raise x;
> $$ language plpythonu;

Shouldn't that look more like

raise plpy.SPIError(msg="Message", sqlstate="0P001", hint="Turn it on
and off again") ?

Keyword args are very much the norm for this sort of thing. I recall
them being pretty reasonable to deal with in the CPython API too, but
otherwise a trivial Python wrapper in the module can easily adapt the
interface.



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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> >> On 2015/10/15 11:36, Kouhei Kaigai wrote:
> >>> In case of scanrelid==0, expectation to ForeignScan/CustomScan is to
> >>> behave as if local join exists here. It requires ForeignScan to generate
> >>> joined-tuple as a result of remote join, that may contains multiple junk
> >>> TLEs to carry whole-var references of base foreign tables.
> >>> According to the criteria, the desirable behavior is clear as below:
> >>>
> >>> 1. FDW/CSP picks up base relation's tuple from the EPQ slots.
> >>>  It shall be setup by whole-row reference if earlier row-lock 
> >>> semantics,
> >>>  or by RefetchForeignRow if later row-lock semantics.
> >>>
> >>> 2. Fill up ss_ScanTupleSlot according to the xxx_scan_tlist.
> >>>  We may be able to provide a common support function here, because 
> >>> this
> >>>  list keeps relation between a particular attribute of the 
> >>> joined-tuple
> >>>  and its source column.
> >>>
> >>> 3. Apply join-clause and base-restrict that were pushed down.
> >>>  setrefs.c initializes expressions kept in fdw_exprs/custom_exprs to 
> >>> run
> >>>  on the ss_ScanTupleSlot. It is the easiest way to check here.
> >>>
> >>> 4. If joined-tuple is still visible after the step 3, FDW/CSP returns
> >>>  joined-tuple. Elsewhere, returns an empty slot.
> >>>
> >>> It is entirely compatible behavior even if local join is located on
> >>> the point of ForeignScan/CustomScan with scanrelid==0.
> >>>
> >>> Even if remote join is parametalized by other relation, we can simply
> >>> use param-info delivered from the corresponding outer scan at the step-3.
> >>> EState should have the parameters already updated, FDW driver needs to
> >>> care about nothing.
> >>>
> >>> It is quite less invasive approach towards the existing EPQ recheck
> >>> mechanism.
> 
> I wrote:
> >> I see.  That's an idea, but I guess that step 2 and 3 would need to add
> >> a lot of code to the core.  Why don't you use a local join execution
> >> plan that we discussed?  I think that that would make the series of
> >> processing much simpler.  I'm now revising the patch that I created for
> >> that.  If it's okay, I'd like to propose an updated version of the patch
> >> in a few days.
> 
> On 2015/10/15 20:19, Kouhei Kaigai wrote:
> > I have to introduce why above idea is simpler and suitable for v9.5
> > timeline.
> > As I've consistently proposed for this two months, the step-2 and 3
> > are assumed to be handled in the callback routine to be kicked from
> > ForeignRecheck().
> 
> Honestly, I still don't think I would see the much value in doing so.
> As Robert mentioned in [1], I think that if we're inside EPQ,
> pushed-down quals and/or pushed-down joins should be locally rechecked
> in the same way as other cases such as IndexRecheck.  So, I'll propose
> the updated version of the patch.
>
You have never answered my question for two months.

I never deny to execute the pushed-down qualifiers locally.
It is likely the best tactics in most cases.
But, why you try to enforce all the people a particular manner?

Here are various kind of FDW drivers. How do you guarantee it is
the best solution for all the people? It is basically impossible.
(Please google "Probatio diabolica")

You try to add two special purpose fields in ForeignScan;
fdw_recheck_plan and fdw_recheck_quals.
It requires FDW drivers to have pushed-down qualifier in a particular
data format, and also requires FDW drivers to process EPQ recheck by
alternative local plan, even if a part of FDW drivers can process
these jobs by its own implementation better.

I've repeatedly pointed out this issue, but never get reasonable
answer from you.

Again, I also admit alternative plan may be reasonable tactics for
most of FDW drivers. However, only FDW author can "decide" it is
the best tactics to handle the task for their module, not us.

I don't think it is a good interface design to enforce people to
follow a particular implementation manner. It should be discretion
of the extension.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

I briefly browsed the patch apart from my preference towards the approach.

It has at least two oversight.

*** 48,59  ExecScanFetch(ScanState *node,
+   /*
+* Execute recheck plan and get the next tuple if foreign join.
+*/
+   if (scanrelid == 0)
+   {
+   (*recheckMtd) (node, slot);
+   return slot;
+   }

Ensure the slot is empty if recheckMtd returned false, as base relation
case doing so.


*** 347,352  ExecScanReScan(ScanState *node)
{
Index   scanrelid = ((Scan *) node->ps.plan)->scanrelid;
  
+   if (scanrelid == 0)
+   return; /* nothing to do */
+ 
Assert(scanrelid > 0);
  
estate->es_epqScanDone[scanrelid - 1] = false;

Why nothing to do?
Base relations managed by ForeignScan are tracked in fs_relids bitmap.
As you introduced a few days before, if ForeignScan has parametalized
remote join, EPQ slot contains invalid tuples based on old outer tuple.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 


> -Original Message-
> From: Etsuro Fujita [mailto:fujita.ets...@lab.ntt.co.jp]
> Sent: Friday, October 16, 2015 6:01 PM
> To: Robert Haas
> Cc: Kyotaro HORIGUCHI; Kaigai Kouhei(海外 浩平); pgsql-hackers@postgresql.org;
> Shigeru Hanada
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
> 
> On 2015/10/14 17:31, Etsuro Fujita wrote:
> > As KaiGai-san also pointed out before, I think we should address this in
> > each of the following cases:
> >
> > 1) remote qual (scanrelid>0)
> > 2) remote join (scanrelid==0)
> 
> As for #2, I updated the patch, which uses a local join execution plan
> for an EvalPlanQual rechech, according to the comment from Robert [1].
> Attached is an updated version of the patch.  This is a WIP patch, but
> it would be appreciated if I could get feedback earlier.
> 
> For tests, apply the patches:
> 
> foreign-recheck-for-foreign-join-1.patch
> usermapping_matching.patch [2]
> add_GetUserMappingById.patch [2]
> foreign_join_v16_efujita.patch [3]
> 
> Since that as I said upthread, what I'd like to discuss is changes to
> the PG core, I didn't do anything about the postgres_fdw patches.
> 
> Best regards,
> Etsuro Fujita
> 
> [1]
> http://www.postgresql.org/message-id/CA+TgmoaAzs0dR23R7PTBseQfwOtuVCPNBqDHxe
> bo9gi+dmx...@mail.gmail.com
> [2]
> http://www.postgresql.org/message-id/CAEZqfEe9KGy=1_waGh2rgZPg0o4pqgD+iauYaj
> 8wtze+cyj...@mail.gmail.com
> [3] http://www.postgresql.org/message-id/55cb2d45.7040...@lab.ntt.co.jp

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

Re: [HACKERS] TODO list updates

[Amit Kapila]

On Fri, Oct 16, 2015 at 8:34 AM, Bruce Momjian  wrote:

> I have spend the past few days updating our TODO list, removing
> completed and now-unnecessary items:
>
> https://wiki.postgresql.org/wiki/Todo
>
>
Thanks.  It can help encourage many new entrants to community.


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

Re: [HACKERS] Windows service is not starting so there’s message in log: FATAL: "could not create shared memory segment “Global/PostgreSQL.851401618”: Permission denied”

[Dmitry Vasilyev]

On Пт, 2015-10-16 at 09:02 +0530, Amit Kapila wrote:
> On Thu, Oct 15, 2015 at 8:35 PM, Dmitry Vasilyev  pro.ru> wrote:
> > I think that function dsm_impl_windows() with EACCES error should
> > not
> > do ereport() with FATAL level. It works, but it is likely to make
> > an
> > infinite loop if the user will receive EACCES error.
> > 
> > 
> Currently we are using error level as ERROR for creating dsm during
> postmaster startup which is not right and rather we should use error
> level as LOG.  Can you please try with the attached patch and see
> if the issue is fixed for you.
> 
> Another some what related point is currently we are using random()
> function to ensure a unique name for dsm and it seems to me that
> it is always going to generate same number on first invocation (at
> least
> thats what happening on windows) due to which you are seeing the
> error.  Another options could be to append current pid or data
> directory
> path as we are doing in win32_shmem.c.  I think this could be an
> optimization which can be done in addition to the fix attached (we
> can
> do this as a separate patch as well, if we agreed to do anything).
> 
> 
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com


In that case your patch is working. There was LOG level message with Permission 
denied (from the operation point it’s not clear how to respond to message like 
this).
But as I understand if operating system can’t create shared memory we will try 
to distinguish that chunk up to infinity.
As I see it’s better to stop instance in this case.
So I guess it’s a good idea to remain all as it is now (FATAL level) but add 
PostmasterPid to name (as you suggest). There’s patch in attachments.


Dmitry Vasilyev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c
index 921f029..55335bf 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -68,6 +68,7 @@
 #include "utils/guc.h"
 #include "utils/memutils.h"
 #include "postmaster/postmaster.h"
+#include "miscadmin.h"
 
 #ifdef USE_DSM_POSIX
 static bool dsm_impl_posix(dsm_op op, dsm_handle handle, Size request_size,
@@ -631,7 +632,7 @@ dsm_impl_windows(dsm_op op, dsm_handle handle, Size request_size,
 	 * similar to main shared memory. We can change here once issue mentioned
 	 * in GetSharedMemName is resolved.
 	 */
-	snprintf(name, 64, "%s.%u", SEGMENT_NAME_PREFIX, handle);
+	snprintf(name, 64, "%s.%d.%u", SEGMENT_NAME_PREFIX, PostmasterPid, handle);
 
 	/*
 	 * Handle teardown cases.  Since Windows automatically destroys the object

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

[HACKERS] proposal: DROP DATABASE variant that kills active sessions

[Filip Rembiałkowski]

DROP DATABASE mydb CONCURRENTLY;

That would perform forced shutdown

1) reject any new backends to mydb
2) terminate old backends
3) drop db

40 upvotes here http://dba.stackexchange.com/a/11895/3710 inspired me
to propose this improvement.

If you think it's a good idea please include it as a low-priority TODO item.

thanks


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

Re: [HACKERS] Dangling Client Backend Process

[Rajeev rastogi]

On 14 October 2015 14:03, Kyotaro HORIGUCHI:

>Subject: Re: [HACKERS] Dangling Client Backend Process
>
>At Wed, 14 Oct 2015 11:08:37 +0530, Amit Kapila 
>wrote in
>
>> On Tue, Oct 13, 2015 at 3:54 PM, Rajeev rastogi
>> 
>> wrote:
>> > If we add the event WL_POSTMASTER_DEATH also, client backend process
>> > handling will become same as other backend process. So postmaster
>> > death can be detected in the same way.
>> >
>> > But I am not sure if WL_POSTMASTER_DEATH event was not added
>> > intentionally for some reason. Please confirm.
>> >
>> > Also is it OK to add this even handling in generic path of Libpq?
>> >
>> > Please let me know if I am missing something?
>> >
>> >
>> I feel this is worth investigation, example for what kind of cases
>> libpq is used for non-blocking sockets, because for such cases above
>> idea will not work.
>
>Blocking mode of a port is changed using socket_set_nonblocking(). I
>found two points that the function is called with true.
>pq_getbyte_if_available() and socket_flush_if_writable(). They seems to
>be used only in walsender *so far*.

Yes and in that case I assume the proposed solution will work in all cases.

>> Here, I think the bigger point is that, Tom was not in favour of this
>> proposal (making backends exit on postmaster death ) at that time, not
>> sure whether he has changed his mind.

>If I recall correctly, he concerned about killing the backends running
>transactions which could be saved. I have a sympathy with the opinion.
>But also think it reasonable to kill all backends immediately so that
>new postmaster can run...

Yes it will be really helpful to know the earlier reason for "not making 
backend exit on postmaster death". 
Please let me know if there is any thread, which I can refer to find the same.

IMHO there could be below major issues, if we don't kill client backend process 
on postmaster death:
1. Postmaster cannot be re-started again as pointed by Kyotaro and Andres Also.
2. If from existing client session, we try to do some operation which has 
dependency with backend process, then that operation will either fail or may 
lead to undefined behavior sometime.
3. Also unintentionally all operation done by application will not be 
checkpointed and also will not be flushed by bgworker. 
4. Replicating of WAL to standby will be stopped and if synchronous standby is 
configured then command from master will be hanged.

Thanks and Regards,
Kumar Rajeev Rastogi





-- 
Sent 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: DROP DATABASE variant that kills active sessions

[Pavel Stehule]

Hi

2015-10-16 12:13 GMT+02:00 Filip Rembiałkowski <
filip.rembialkow...@gmail.com>:

> DROP DATABASE mydb CONCURRENTLY;
>
> That would perform forced shutdown
>
> 1) reject any new backends to mydb
> 2) terminate old backends
> 3) drop db
>
> 40 upvotes here http://dba.stackexchange.com/a/11895/3710 inspired me
> to propose this improvement.
>
> If you think it's a good idea please include it as a low-priority TODO
> item.
>

in GoodData we have this feature implemented - little bit different named -
DROP DATABASE FORCE

It is useful in complex environment with mix of pooled and not pooled
connections - and in our environment - about 2K databases per server with
lot of dropped / created databases per server / per day.

I can publish this patch, if there will be any interest.

last note: little bit related topic - we have some patches for CREATE
DATABASE - longer waiting for locking template1 - although there are some
opened issues - like any CREATE DATABASE requires checkpoint, and it is
slower in our environment.

Regards

Pavel




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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/10/14 17:31, Etsuro Fujita wrote:

As KaiGai-san also pointed out before, I think we should address this in
each of the following cases:

1) remote qual (scanrelid>0)
2) remote join (scanrelid==0)


As for #2, I updated the patch, which uses a local join execution plan 
for an EvalPlanQual rechech, according to the comment from Robert [1]. 
Attached is an updated version of the patch.  This is a WIP patch, but 
it would be appreciated if I could get feedback earlier.


For tests, apply the patches:

foreign-recheck-for-foreign-join-1.patch
usermapping_matching.patch [2]
add_GetUserMappingById.patch [2]
foreign_join_v16_efujita.patch [3]

Since that as I said upthread, what I'd like to discuss is changes to 
the PG core, I didn't do anything about the postgres_fdw patches.


Best regards,
Etsuro Fujita

[1] 
http://www.postgresql.org/message-id/ca+tgmoaazs0dr23r7ptbseqfwotuvcpnbqdhxebo9gi+dmx...@mail.gmail.com
[2] 
http://www.postgresql.org/message-id/CAEZqfEe9KGy=1_wagh2rgzpg0o4pqgd+iauyaj8wtze+cyj...@mail.gmail.com

[3] http://www.postgresql.org/message-id/55cb2d45.7040...@lab.ntt.co.jp
*** a/contrib/file_fdw/file_fdw.c
--- b/contrib/file_fdw/file_fdw.c
***
*** 525,530  fileGetForeignPaths(PlannerInfo *root,
--- 525,531 
  	 total_cost,
  	 NIL,		/* no pathkeys */
  	 NULL,		/* no outer rel either */
+ 	 NULL,		/* no alternative path */
  	 coptions));
  
  	/*
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***
*** 560,565  postgresGetForeignPaths(PlannerInfo *root,
--- 560,566 
     fpinfo->total_cost,
     NIL, /* no pathkeys */
     NULL,		/* no outer rel either */
+    NULL,		/* no alternative path */
     NIL);		/* no fdw_private list */
  	add_path(baserel, (Path *) path);
  
***
*** 727,732  postgresGetForeignPaths(PlannerInfo *root,
--- 728,734 
  	   total_cost,
  	   NIL,		/* no pathkeys */
  	   param_info->ppi_req_outer,
+ 	   NULL,	/* no alternative path */
  	   NIL);	/* no fdw_private list */
  		add_path(baserel, (Path *) path);
  	}
*** a/src/backend/executor/execScan.c
--- b/src/backend/executor/execScan.c
***
*** 48,59  ExecScanFetch(ScanState *node,
  		 * conditions.
  		 */
  		Index		scanrelid = ((Scan *) node->ps.plan)->scanrelid;
  
  		Assert(scanrelid > 0);
  		if (estate->es_epqTupleSet[scanrelid - 1])
  		{
- 			TupleTableSlot *slot = node->ss_ScanTupleSlot;
- 
  			/* Return empty slot if we already returned a tuple */
  			if (estate->es_epqScanDone[scanrelid - 1])
  return ExecClearTuple(slot);
--- 48,67 
  		 * conditions.
  		 */
  		Index		scanrelid = ((Scan *) node->ps.plan)->scanrelid;
+ 		TupleTableSlot *slot = node->ss_ScanTupleSlot;
+ 
+ 		/*
+ 		 * Execute recheck plan and get the next tuple if foreign join.
+ 		 */
+ 		if (scanrelid == 0)
+ 		{
+ 			(*recheckMtd) (node, slot);
+ 			return slot;
+ 		}
  
  		Assert(scanrelid > 0);
  		if (estate->es_epqTupleSet[scanrelid - 1])
  		{
  			/* Return empty slot if we already returned a tuple */
  			if (estate->es_epqScanDone[scanrelid - 1])
  return ExecClearTuple(slot);
***
*** 347,352  ExecScanReScan(ScanState *node)
--- 355,363 
  	{
  		Index		scanrelid = ((Scan *) node->ps.plan)->scanrelid;
  
+ 		if (scanrelid == 0)
+ 			return;/* nothing to do */
+ 
  		Assert(scanrelid > 0);
  
  		estate->es_epqScanDone[scanrelid - 1] = false;
*** a/src/backend/executor/nodeForeignscan.c
--- b/src/backend/executor/nodeForeignscan.c
***
*** 24,29 
--- 24,30 
  
  #include "executor/executor.h"
  #include "executor/nodeForeignscan.h"
+ #include "executor/tuptable.h"
  #include "foreign/fdwapi.h"
  #include "utils/memutils.h"
  #include "utils/rel.h"
***
*** 80,85  ForeignRecheck(ForeignScanState *node, TupleTableSlot *slot)
--- 81,103 
  	 */
  	econtext = node->ss.ps.ps_ExprContext;
  
+ 	if (node->fdw_recheck_plan != NULL)
+ 	{
+ 		TupleTableSlot *result;
+ 		MemoryContext oldcontext;
+ 
+ 		/* Must be in query context to call recheck plan */
+ 		oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
+ 		/* Execute recheck plan */
+ 		result = ExecProcNode(node->fdw_recheck_plan);
+ 		MemoryContextSwitchTo(oldcontext);
+ 		if (TupIsNull(result))
+ 			return false;
+ 		/* Store result in the given slot */
+ 		ExecCopySlot(slot, result);
+ 		return true;
+ 	}
+ 
  	/* Does the tuple meet the remote qual condition? */
  	econtext->ecxt_scantuple = slot;
  
***
*** 200,205  ExecInitForeignScan(ForeignScan *node, EState *estate, int eflags)
--- 218,229 
  	ExecAssignScanProjectionInfoWithVarno(>ss, tlistvarno);
  
  	/*
+ 	 * Initialize recheck plan.
+ 	 */
+ 	scanstate->fdw_recheck_plan = ExecInitNode(node->fdw_recheck_plan,
+ 			   estate, eflags);
+ 
+ 	/*
  	 * Initialize 

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

>> On 2015/10/15 11:36, Kouhei Kaigai wrote:
>>> In case of scanrelid==0, expectation to ForeignScan/CustomScan is to
>>> behave as if local join exists here. It requires ForeignScan to generate
>>> joined-tuple as a result of remote join, that may contains multiple junk
>>> TLEs to carry whole-var references of base foreign tables.
>>> According to the criteria, the desirable behavior is clear as below:
>>>
>>> 1. FDW/CSP picks up base relation's tuple from the EPQ slots.
>>>  It shall be setup by whole-row reference if earlier row-lock semantics,
>>>  or by RefetchForeignRow if later row-lock semantics.
>>>
>>> 2. Fill up ss_ScanTupleSlot according to the xxx_scan_tlist.
>>>  We may be able to provide a common support function here, because this
>>>  list keeps relation between a particular attribute of the joined-tuple
>>>  and its source column.
>>>
>>> 3. Apply join-clause and base-restrict that were pushed down.
>>>  setrefs.c initializes expressions kept in fdw_exprs/custom_exprs to run
>>>  on the ss_ScanTupleSlot. It is the easiest way to check here.
>>>
>>> 4. If joined-tuple is still visible after the step 3, FDW/CSP returns
>>>  joined-tuple. Elsewhere, returns an empty slot.
>>>
>>> It is entirely compatible behavior even if local join is located on
>>> the point of ForeignScan/CustomScan with scanrelid==0.
>>>
>>> Even if remote join is parametalized by other relation, we can simply
>>> use param-info delivered from the corresponding outer scan at the step-3.
>>> EState should have the parameters already updated, FDW driver needs to
>>> care about nothing.
>>>
>>> It is quite less invasive approach towards the existing EPQ recheck
>>> mechanism.

I wrote:
>> I see.  That's an idea, but I guess that step 2 and 3 would need to add
>> a lot of code to the core.  Why don't you use a local join execution
>> plan that we discussed?  I think that that would make the series of
>> processing much simpler.  I'm now revising the patch that I created for
>> that.  If it's okay, I'd like to propose an updated version of the patch
>> in a few days.

On 2015/10/15 20:19, Kouhei Kaigai wrote:
> I have to introduce why above idea is simpler and suitable for v9.5
> timeline.
> As I've consistently proposed for this two months, the step-2 and 3
> are assumed to be handled in the callback routine to be kicked from
> ForeignRecheck().

Honestly, I still don't think I would see the much value in doing so.
As Robert mentioned in [1], I think that if we're inside EPQ,
pushed-down quals and/or pushed-down joins should be locally rechecked
in the same way as other cases such as IndexRecheck.  So, I'll propose
the updated version of the patch.

Thanks for the explanation!

Best regards,
Etsuro Fujita

[1]
http://www.postgresql.org/message-id/CA+Tgmoau7jVTLF0Oh9a_Mu9S=vrw7i6u_h7jspzbxv0xtyo...@mail.gmail.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] Allow ssl_renegotiation_limit in PG 9.5

[Albe Laurenz]

Shay Rojansky wrote:
> Just to give some added reasoning...
> 
> As Andres suggested, Npgsql sends ssl_renegotiation_limit=0 because we've 
> seen renegotiation bugs with
> the standard .NET SSL implementation (which Npgsql uses). Seems like everyone 
> has a difficult time
> with renegotiation.

As far as I remember, that was introduced because of renegotiation bugs with 
Mono:
http://lists.pgfoundry.org/pipermail/npgsql-devel/2010-February/001074.html
http://fxjr.blogspot.co.at/2010/03/ssl-renegotiation-patch.html

Of course, with renegotiation disabled, nobody knows whether there would also 
have been renegotiation
problems since Npgsql switched from Mono SSL to .NET SSL ...

> As Tom suggested, it gets sent in the startup packet so that DISCARD/RESET 
> ALL resets back to 0 and
> not to the default 512MB (in older versions). Npgsql itself issues DISCARD 
> ALL in its connection pool
> implementation to reset the connection to its original opened state, and of 
> course users may want to
> do it themselves. Having SSL renegotiation accidentally turned on because a 
> user sent RESET ALL, when
> the SSL implementation is known to have issues, is something to be avoided...

Maybe Npgsql could switch to sending the statement after the connection has been
established and resending it after each RESET ALL?
You could add documentation that people should not use RESET ALL with Npgsql 
unless they
are ready to disable renegotiation afterwards.

If not, the only solution I can see is for PostgreSQL to not protest if it sees 
the
parameter in the startup packet.

Yours,
Laurenz Albe

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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Jeevan Chalke]

On Thu, Oct 15, 2015 at 10:44 PM, Robert Haas  wrote:

> On Thu, Oct 15, 2015 at 3:04 AM, Kyotaro HORIGUCHI
>  wrote:
> > I confirmed that an epqtuple of foreign parameterized scan is
> > correctly rejected by fdw_recheck_quals with modified outer
> > tuple.
> >
> > I have no objection to this and have two humble comments.
> >
> > In file_fdw.c, the comment for the last parameter just after the
> > added line seems to be better to be aligned with other comments.
>
> I've pgindented the file.  Any other space we might choose would just
> be changed by the next pgindent run, so there's no point in trying to
> vary.
>
> > In subselect.c, the added break is in the added curly-braces but
> > it would be better to place it after the closing brace, like the
> > other cases.
>
> Changed that, and committed.
>

With the latest sources having this commit, when I follow same steps,
I get
 ERROR:  unrecognized node type: 525
error.

It looks like, we have missed to handle T_RestrictInfo.
I am getting this error from expression_tree_mutator().


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



-- 
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Jeevan Chalke]

On Fri, Oct 16, 2015 at 3:10 PM, Jeevan Chalke <
jeevan.cha...@enterprisedb.com> wrote:

>
>
> On Thu, Oct 15, 2015 at 10:44 PM, Robert Haas 
> wrote:
>
>> On Thu, Oct 15, 2015 at 3:04 AM, Kyotaro HORIGUCHI
>>  wrote:
>> > I confirmed that an epqtuple of foreign parameterized scan is
>> > correctly rejected by fdw_recheck_quals with modified outer
>> > tuple.
>> >
>> > I have no objection to this and have two humble comments.
>> >
>> > In file_fdw.c, the comment for the last parameter just after the
>> > added line seems to be better to be aligned with other comments.
>>
>> I've pgindented the file.  Any other space we might choose would just
>> be changed by the next pgindent run, so there's no point in trying to
>> vary.
>>
>> > In subselect.c, the added break is in the added curly-braces but
>> > it would be better to place it after the closing brace, like the
>> > other cases.
>>
>> Changed that, and committed.
>>
>
> With the latest sources having this commit, when I follow same steps,
> I get
>  ERROR:  unrecognized node type: 525
> error.
>
> It looks like, we have missed to handle T_RestrictInfo.
> I am getting this error from expression_tree_mutator().
>

Ignore this.
It was caused due to some compilation issue on my system.

It is working as expected in the latest sources.

Sorry for the noise and inconvenience caused.


-- 
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Re: [HACKERS] Parallel Seq Scan

[Amit Kapila]

On Fri, Oct 16, 2015 at 9:51 AM, Robert Haas  wrote:
>
> On Mon, Oct 5, 2015 at 8:20 AM, Amit Kapila 
wrote:
> > [ new patch for heapam.c changes ]
>
> I went over this in a fair amount of detail and reworked it somewhat.
> The result is attached as parallel-heapscan-revised.patch.  I think
> the way I did this is a bit cleaner than what you had, although it's
> basically the same thing.  There are fewer changes to initscan, and we
> don't need one function to initialize the starting block that must be
> called in each worker and then another one to get the next block, and
> generally the changes are a bit more localized.  I also went over the
> comments and, I think, improved them.  I tweaked the logic for
> reporting the starting scan position as the last position report; I
> think the way you had it the last report would be for one block
> earlier.  I'm pretty happy with this version and hope to commit it
> soon.
>

static BlockNumber
+heap_parallelscan_nextpage(HeapScanDesc scan)
+{
+ BlockNumber page = InvalidBlockNumber;
+ BlockNumber sync_startpage = InvalidBlockNumber;
+ BlockNumber report_page = InvalidBlockNumber;

..
..
+ * Report scan location.  Normally, we report the current page number.
+ * When we reach the end of the scan, though, we report the starting page,
+ * not the ending page, just so the starting positions for later scans
+ * doesn't slew backwards.  We only report the position at the end of the
+ * scan once, though: subsequent callers will have report nothing, since
+ * they will have page == InvalidBlockNumber.
+ */
+ if (scan->rs_syncscan)
+ {
+ if (report_page == InvalidBlockNumber)
+ report_page = page;
+ if (report_page != InvalidBlockNumber)
+ ss_report_location(scan->rs_rd, report_page);
+ }
..
}

I think due to above changes it will report sync location on each page
scan, don't we want to report it once at end of scan?

Other than that, patch looks okay to me.

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

Re: [HACKERS] Parallel Seq Scan

[Robert Haas]

On Fri, Oct 16, 2015 at 7:42 AM, Amit Kapila  wrote:
> I think due to above changes it will report sync location on each page
> scan, don't we want to report it once at end of scan?

I think reporting for each page is correct.  Isn't that what the
non-parallel case does?

-- 
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] Parallel Seq Scan

[Robert Haas]

On Thu, Oct 15, 2015 at 11:38 PM, Haribabu Kommi
 wrote:
> Some more tests that failed in similar configuration settings.
> 1. Table that is created under a begin statement is not visible in the worker.
> 2. permission problem in worker side for set role command.

The second problem, too, I have already posted a bug fix for, on a
thread which also contains a whole bunch of other bug fixes.  I'll get
those committed today so you can avoid wasting time finding bugs I've
already found and fixed.  Thanks for testing.

-- 
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] pam auth - add rhost item

[Euler Taveira]

On 15-10-2015 05:41, kolo hhmow wrote:

I have already explained this in my previous post. Did you read this?

>
Yes, I do.


So why postgresql give users an abbility to use a pam modules, when in
other side there is advice to not use them?
Anyway.

>
Where is such advise? I can't see it in docs [1].


I do not see any complication with this approach. Just use one
configuration entry in pg_hba.conf, and rest entries in some database
backend of pam module, which is most convenient with lot of entries than
editing pg_hba.conf.


Why don't you use a group role? I need just one entry in pg_hba.conf.


[1] http://www.postgresql.org/docs/current/static/auth-methods.html#AUTH-PAM
[2] http://www.postgresql.org/docs/current/static/role-membership.html


--
   Euler Taveira   Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento


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

Re: [HACKERS] remaining open items

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> We've got a few open items remaining at
> https://wiki.postgresql.org/wiki/PostgreSQL_9.5_Open_Items - we should
> try to get rid of them.  Of the 8 items there, 3 are documentation
> issues.  It looks to me like one of those is for Stephen to deal with,
> one for Andres, and one for Alvaro.  Is there any reason we can't get
> those taken care of and move on?

I'll get the documentation issue dealt with.  I've got a bunch of
pending documentation improvements that need to be done and hope to post
them very shortly for comment.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Error creating gin index on jsonb columns

[Glenn Zhu]

Hi Tom,

Thanks for the reply.

We are currently running 9.4.4.  9.4.5 notes have two references to gin
index but does not seem to address the issue.

We are however able to create same index on some other databases.  So it
maybe data related or size of table related?  So far, the failed cases
reported are from large tables, with number of rows between 150 to 350
millions.

What would you need in terms of test cases?

Thank you very much.
-glenn


On Fri, Oct 16, 2015 at 6:28 AM, Tom Lane  wrote:

> Glenn Zhu  writes:
> > We are getting an error on the following statement:
> > CREATE INDEX CONCURRENTLY customer_jsonb_fields_idx ON customer USING gin
> > (jsonb_fields jsonb_path_ops);
>
> > ERROR:  invalid memory alloc request size 2013265920
>
> > Anyone know what is causing it?
>
> Sounds like a bug from here.  What PG version is this exactly?  If it's
> not the latest minor releases, try updating.  If it's still there, see if
> you can extract a test case that you can share.
>
> regards, tom lane
>



-- 
Glenn Zhu
SaaS Operations | ❖ Medallia, Inc.

Re: [HACKERS] Error creating gin index on jsonb columns

[Tom Lane]

Glenn Zhu  writes:
> Currently, after hitting the error, the indexes were still created but
> marked with status of "invalid"

That's just what CREATE INDEX CONCURRENTLY would do with any error.
(It might be worth checking whether a non-CONCURRENTLY build hits the
same error, though I'm betting it will.)

> If it's a bug, what information would be needed to trigger a bug fix?

A reproducible test case would move things along quite a bit; without that
we're just guessing.

> Is there a formal channel?

Well, you could move the thread to pgsql-bugs but you might as well
keep it where it is.

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] pam auth - add rhost item

[kolo hhmow]

On Fri, Oct 16, 2015 at 2:47 PM, Euler Taveira  wrote:

> On 15-10-2015 05:41, kolo hhmow wrote:
>
>> I have already explained this in my previous post. Did you read this?
>>
> >
> Yes, I do.
>
> So why postgresql give users an abbility to use a pam modules, when in
>> other side there is advice to not use them?
>> Anyway.
>>
> >
> Where is such advise? I can't see it in docs [1].
>

Not in docs. You gave such advice:
"Therefore, advise PAM users to use HBA is a way to not complicate the
actual feature".


>
> I do not see any complication with this approach. Just use one
>> configuration entry in pg_hba.conf, and rest entries in some database
>> backend of pam module, which is most convenient with lot of entries than
>> editing pg_hba.conf.
>>
>> Why don't you use a group role? I need just one entry in pg_hba.conf.
>
>
> [1]
> http://www.postgresql.org/docs/current/static/auth-methods.html#AUTH-PAM
> [2] http://www.postgresql.org/docs/current/static/role-membership.html
>
>
> Because cannot restrict from what ip address client can connet in such way.
You can restrict only whole group, not just individual member of such
group, or I misunderstand something.


>
>

> --
>Euler Taveira   Timbira - http://www.timbira.com.br/
>PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
>

Re: [HACKERS] [PATCH v3] GSSAPI encryption support

[Stephen Frost]

* Craig Ringer (cr...@2ndquadrant.com) wrote:
> On 16 October 2015 at 01:07, Robbie Harwood  wrote:
> > Looking at
> > http://www.postgresql.org/docs/9.4/static/protocol-message-formats.html
> > suggests that SSPI follows a separate codepath from the GSS code;
> > certainly it's a different auth request, which means it shouldn't
> > trigger the encryption path.
> 
> It's a different auth request, but the handling in be-auth.c is
> co-mingled to handle the cases:

be-auth.c?  You mean src/backend/libpq/auth.c?

> * We're compiled with Kerberos, handling SSPI from Windows
> * We're compiled with Kerberos, handling a GSSAPI request

Eh, *kerberos* was removed with 9.4.0.  I'm guessing you mean GSSAPI
above.

> * We're compiled with SSPI, and we're using Windows SSPI to handle a
> GSSAPI auth request
> * We're compiled with SSPI, and we're using it to handle a SSP client request
> 
> SSPI is a wrapper. For Windows Domain members it carries
> GSSAPI/Kerberos data with the spnego extension. (For local loopback it
> does NTLM, but you don't need to care about that).

I have to admit that I'm not following what you're getting at here.

src/backend/libpq/auth.c and src/interfaces/libpq/fe-auth.c have pretty
clearly independent paths for SSPI and GSSAPI and which is used
(server-side) is based on what the auth method in pg_hba.conf is.  In a
properly set up environment, the client could be using either, but it's
not like we try to figure out (or care) which the client is using from
the server's perspective.

Now, if you set up GSSAPI on the client side and tell it to require
encryption and you're using SSPI on the server side (or the other way
around) it's not likely to work without changes to the SSPI code paths,
which we should be looking at doing.  I've not looked at the patch in
much depth yet, but hopefully there's a straight-forward way to say
"we're using SSPI on (client/server) and that doesn't support
encryption yet, so don't try to require it" and throw an error if
someone does.  I don't think we necessairly have to have encryption with
SSPI working initially (and that's assuming SSPI can actually do
encryption the way GSSAPI can, I don't know that it can).

> > There is no reason that using GSSAPI from, e.g., MIT Kerberos for
> > Windows, should not work here.
> 
> Except that *nobody* does it. The EDB installer builds Pg with SSPI,
> and that's what the overwhelming majority of Windows users use.

Apparently *somebody* uses MIT KfW; I doubt MIT would have put out
KfW 4.0 if no one wanted it.  I agree that it's clearly a small number
of environments (I don't know of any currently) and it's entirely
possible that none of them use PG with GSSAPI on Windows for
authentication.

> Personally I don't care if this patch doesn't add support for GSSAPI
> encryption for sspi connections, only that it doesn't break them.

Agreed and we should certainly test it, but I'm not seeing what you're
getting at as the concern.

> >  Even more broadly than that, GSSAPI is a
> > RFC-standardized protocol with rigorously tested interop etc. etc.,
> > though whether anyone outside of MIT cares about MIT Kerberos for
> > Windows I have no idea.
> 
> It's maintained enough that AFAIK it works, and Pg supports compiling
> with it. No idea if anyone tests it with Pg anymore.

The EDB installers were changed to use SSPI instead of MIT KfW (or
perhaps just dropped building with GSSAPI, Dave would probably remember
better than I do) because KfW wasn't being maintained or updated any
more and the latest version had known security issues.  That was quite a
while ago and it looks like MIT has decided to revive KfW (note the
drought between about 2007 and 2012 or so though..) with KfW 4.0.

I doubt anyone's tried building or running PG with GSSAPI under Windows
though.  I agree it'd be good to test and see and perhaps even the EDB
installers could be changed to add support for it back in (not sure if
Dave really wants to though as it was a bit of a pain..).  However,
presumably MIT has updated KfW and contains to maintain it because
someone is using it.

Really, though, assuming that GSSAPI as provided by KfW works per the
spec-defined API, I don't see why it wouldn't work under Windows just
the same as it does under *nix, it's not like we have any
Windows-specific code in backend/libpq/auth.c or
interfaces/libpq/fe-auth.c for GSSAPI except a MingW-specific symbol
definition.

> > I think the important takeaway here is that I haven't actually changed
> > how *authentication* works; these changes only affect GSSAPI
> > post-authentication add encryption functions as defined by that
> > specification.  So if the authentication was working before, and the
> > GSSAPI implementation is following RFC, I would hope that this would
> > work still.
> 
> Hope so.
> 
> I'll put this on my "hope to test it at some stage" list, but I don't
> have a prebuilt environment for it and have a lot to get ready for the
> next CF, so 

Re: [HACKERS] Allow ssl_renegotiation_limit in PG 9.5

[Andres Freund]

On 2015-10-16 16:41:16 +0300, Shay Rojansky wrote:
> > If not, the only solution I can see is for PostgreSQL to not protest if it
> > sees the
> > parameter in the startup packet.
> >
> 
> Yeah, that's the ideal solution here as far as I'm concerned.

Well, it seems that's where we're ending up then. Could you prepare a
patch?

Regards,

Andres


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

Re: [HACKERS] Error creating gin index on jsonb columns

[Tom Lane]

Glenn Zhu  writes:
> We are currently running 9.4.4.  9.4.5 notes have two references to gin
> index but does not seem to address the issue.

> We are however able to create same index on some other databases.  So it
> maybe data related or size of table related?

I'd guess that it's triggered by a specific data item or set of data
items.  Doubt it has anything to do with table size per se.  The quoted
value is 0x7800, which makes me think that something is
misinterpreting a plain C string as a varlena value (with a length word),
or something along that line.

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] pam auth - add rhost item

[Alvaro Herrera]

Robert Haas wrote:

> I think some more interesting questions are:
> - Did he implement this correctly?
> - Would it break anything?
> - Are there lots of other knobs we should expose too instead of just one?
> - What would it take to turn this into a committable patch?
> - Would the cost of exposing this and perhaps some other knobs cost
> too much in performance for the number of people it would make happy?
> - If so, should the behavior be GUC-controlled or is there
> justification for arguing we should drop the whole patch?

I agree with this set of questions -- the idea behind the patch seemed
quite reasonable to me.

> I feel like we've got somebody new showing up to our community with an
> idea that is not obviously stupid.  If we want such people to stick
> around, we should try to give their ideas a fair shake.

+1 to this, too.

-- 
Á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] Parallel Seq Scan

[Amit Kapila]

On Fri, Oct 16, 2015 at 5:31 PM, Robert Haas  wrote:
>
> On Fri, Oct 16, 2015 at 7:42 AM, Amit Kapila 
wrote:
> > I think due to above changes it will report sync location on each page
> > scan, don't we want to report it once at end of scan?
>
> I think reporting for each page is correct.  Isn't that what the
> non-parallel case does?
>

Yes, sorry I got confused.


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

Re: [HACKERS] Allow ssl_renegotiation_limit in PG 9.5

[Shay Rojansky]

>
> As far as I remember, that was introduced because of renegotiation bugs
> with Mono:
> http://lists.pgfoundry.org/pipermail/npgsql-devel/2010-February/001074.html
> http://fxjr.blogspot.co.at/2010/03/ssl-renegotiation-patch.html
>
> Of course, with renegotiation disabled, nobody knows whether there would
> also have been renegotiation
> problems since Npgsql switched from Mono SSL to .NET SSL ...
>

You may be right (this was before my time on Npgsql). But since PostgreSQL
is dropping negotiation on its side it doesn't really make sense to dive
into this and find out if it's still buggy or not...

Maybe Npgsql could switch to sending the statement after the connection has
> been
> established and resending it after each RESET ALL?
> You could add documentation that people should not use RESET ALL with
> Npgsql unless they
> are ready to disable renegotiation afterwards.
>

That's certainly possible, although it seems problematic to prohibit RESET
ALL because of an issue like this. It's a useful feature that users may
want to use as they manipulate parameters, that's why PostgreSQL has it in
the first place...

I also prefer not to rely on documentation warnings which people may miss,
especially in this case where the consequences of accidentally turning on
renegotiation with a buggy stack would be extremely difficult to track and
debug...

If not, the only solution I can see is for PostgreSQL to not protest if it
> sees the
> parameter in the startup packet.
>

Yeah, that's the ideal solution here as far as I'm concerned.

Re: [HACKERS] Error creating gin index on jsonb columns

[Glenn Zhu]

Currently, after hitting the error, the indexes were still created but
marked with status of "invalid"

Looks like we shall see inserts to fail with the index on the column,
regardless of the index status ("valid" or "invalid"), if we start to
receive the "bad" values?  Maybe I shall drop all these indexes.

If it's a bug, what information would be needed to trigger a bug fix?  Is
there a formal channel?

Thanks.
-glenn


On Fri, Oct 16, 2015 at 6:43 AM, Tom Lane  wrote:

> Glenn Zhu  writes:
> > We are currently running 9.4.4.  9.4.5 notes have two references to gin
> > index but does not seem to address the issue.
>
> > We are however able to create same index on some other databases.  So it
> > maybe data related or size of table related?
>
> I'd guess that it's triggered by a specific data item or set of data
> items.  Doubt it has anything to do with table size per se.  The quoted
> value is 0x7800, which makes me think that something is
> misinterpreting a plain C string as a varlena value (with a length word),
> or something along that line.
>
> regards, tom lane
>



-- 
Glenn Zhu
SaaS Operations | ❖ Medallia, Inc.

Re: [HACKERS] TODO list updates

[Bruce Momjian]

On Fri, Oct 16, 2015 at 02:50:04PM +0530, Amit Kapila wrote:
> On Fri, Oct 16, 2015 at 8:34 AM, Bruce Momjian  wrote:
> 
> I have spend the past few days updating our TODO list, removing
> completed and now-unnecessary items:
> 
>         https://wiki.postgresql.org/wiki/Todo
> 
> 
> 
> Thanks.  It can help encourage many new entrants to community.

I reduced the number of items from 466 to 410.

Probably the most controvertial change was to move on-disk bitmap
indexes to the "not wanted" section, though I kept the links in case we
change our minds.  I just can't see how they would be a win with GIN and
in-memory bitmaps.  (I don't think BRIN indexes help for on-disk bitmap
use-cases, do they?)

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +


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

[HACKERS] Error creating gin index on jsonb columns

[Glenn Zhu]

We are getting an error on the following statement:

CREATE INDEX CONCURRENTLY customer_jsonb_fields_idx ON customer USING gin
(jsonb_fields jsonb_path_ops);

ERROR:  invalid memory alloc request size 2013265920

Anyone know what is causing it?  It does not seem to be data corruption as
when we deploy to multiple databases we got exactly the same error.

I did try to set maintenance_work_mam to 4GB and still the same error
occurred.

Thank you.
-glenn
-- 
Glenn Zhu
SaaS Operations | ❖ Medallia, Inc.

Re: [HACKERS] Error creating gin index on jsonb columns

[Tom Lane]

Glenn Zhu  writes:
> We are getting an error on the following statement:
> CREATE INDEX CONCURRENTLY customer_jsonb_fields_idx ON customer USING gin
> (jsonb_fields jsonb_path_ops);

> ERROR:  invalid memory alloc request size 2013265920

> Anyone know what is causing it?

Sounds like a bug from here.  What PG version is this exactly?  If it's
not the latest minor releases, try updating.  If it's still there, see if
you can extract a test case that you can share.

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] pam auth - add rhost item

[Robert Haas]

On Fri, Oct 16, 2015 at 8:47 AM, Euler Taveira  wrote:
> On 15-10-2015 05:41, kolo hhmow wrote:
>>
>> I have already explained this in my previous post. Did you read this?
>
>>
> Yes, I do.
>
>> So why postgresql give users an abbility to use a pam modules, when in
>> other side there is advice to not use them?
>> Anyway.
>
>>
> Where is such advise? I can't see it in docs [1].
>
>> I do not see any complication with this approach. Just use one
>> configuration entry in pg_hba.conf, and rest entries in some database
>> backend of pam module, which is most convenient with lot of entries than
>> editing pg_hba.conf.
>>
> Why don't you use a group role? I need just one entry in pg_hba.conf.

I feel like this discussion has taken an unhelpful turn.  Surely you
can see that this is not necessarily an exact substitute for what kolo
hhmow wants to do.  Yeah, he could decide to do something else
instead, but are you really confused about why he would want to do
this in PAM, or is this just a case of arguing that what we have is
good enough so let's not change anything or take suggestions?  He's
not saying there's no workaround; he's just saying he'd like this
better.

I think some more interesting questions are:
- Did he implement this correctly?
- Would it break anything?
- Are there lots of other knobs we should expose too instead of just one?
- What would it take to turn this into a committable patch?
- Would the cost of exposing this and perhaps some other knobs cost
too much in performance for the number of people it would make happy?
- If so, should the behavior be GUC-controlled or is there
justification for arguing we should drop the whole patch?

I feel like we've got somebody new showing up to our community with an
idea that is not obviously stupid.  If we want such people to stick
around, we should try to give their ideas a fair shake.

-- 
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] Error creating gin index on jsonb columns

[Glenn Zhu]

We can't test a non-concurrent index build in production - but your word is
just as good.

We only see this on some of production databases.  We did not see it in QA
testing.  But we will try to get a test case in QA.

Is this categorized as a bug specific to GIN indexes or a PostgreSQL bug in
general?

-glenn

On Fri, Oct 16, 2015 at 7:02 AM, Tom Lane  wrote:

> Glenn Zhu  writes:
> > Currently, after hitting the error, the indexes were still created but
> > marked with status of "invalid"
>
> That's just what CREATE INDEX CONCURRENTLY would do with any error.
> (It might be worth checking whether a non-CONCURRENTLY build hits the
> same error, though I'm betting it will.)
>
> > If it's a bug, what information would be needed to trigger a bug fix?
>
> A reproducible test case would move things along quite a bit; without that
> we're just guessing.
>
> > Is there a formal channel?
>
> Well, you could move the thread to pgsql-bugs but you might as well
> keep it where it is.
>
> regards, tom lane
>



-- 
Glenn Zhu
SaaS Operations | ❖ Medallia, Inc.

[HACKERS] buildfarm failures on crake and sittella

[Robert Haas]

Andrew,

The FileTextArrayFDW-build failure on crake, and the RedisFDW-build
failure on sittella, are expected results of my commit
5043193b78919a1bd144563aadc2f7f726549913.  If those FDWs do not push
quals down, they just need to be updated to pass an additional NIL
argument to make_foreignscan().  If they do, they need to pass a list
of the pushed-down quals in that new argument, as described in the
above-mentioned commit.

Can you either update those FDWs or disable those builds for now so
the BF is happy?

Thanks,

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


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

Re: [HACKERS] TODO list updates

[Bruce Momjian]

On Fri, Oct 16, 2015 at 12:00:11PM -0300, Alvaro Herrera wrote:
> Bruce Momjian wrote:
> > I have spend the past few days updating our TODO list, removing
> > completed and now-unnecessary items:
> > 
> > https://wiki.postgresql.org/wiki/Todo
> 
> Thanks.  We have "TodoDone" pages for items that were done in specific
> releases, but only for 8.4, 9.0 and 9.1.  I guess it was too much work
> to create those.  (See https://wiki.postgresql.org/wiki/Category:Todo ).

Oh, I had forgotten about that.  In some cases I knew the release that
completed the item, but in most cases the item had a different approach
which was not taken, or was superceeded by a better solution.

> I added a link to the latest posted version of bitmap indexes by Abhijit
> Menon-Sen, for completeness.

Thanks.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +


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

Re: [HACKERS] pg_dump LOCK TABLE ONLY question

[Robert Haas]

On Thu, Oct 15, 2015 at 9:13 PM, Jim Nasby  wrote:
> OTOH, now that the catalog is MVCC capable, do we even still need to lock
> the objects for a schema-only dump?

Yes.  The MVCC snapshots used for catalog reads are stable only for
the duration of one particular catalog read.  We're not using the
transaction snapshot.

-- 
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] TODO list updates

[Bruce Momjian]

On Fri, Oct 16, 2015 at 12:02:03PM -0300, Alvaro Herrera wrote:
> Bruce Momjian wrote:
> 
> > Are you suggesting I remove those links?  It is kind of odd to have
> > links to patches for features we don't want, or just keep it?
> 
> No, quite the contrary -- I think the links allow some other person
> research the issue including the history of patches and discussion, and
> decide for themselves whether the feature is right to be rejected.

I think on-disk bitmap indexes would only beat GIN indexes in a
read-only database on low-cardinality columns.  For example, if you had
a purchase_log table and wanted to know all the "blue" and "large" items
sold at a specific store, I can see on-disk bitmap indexes doing well
there.  If you added the product number, or needed read/write, I think
GIN would win.  I just don't think we have enough deployments who need
what on-disk bitmap are best at.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +


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

Re: [HACKERS] pam auth - add rhost item

[Robert Haas]

On Fri, Oct 16, 2015 at 10:50 AM, Euler Taveira  wrote:
>> I feel like we've got somebody new showing up to our community with an
>> idea that is not obviously stupid.  If we want such people to stick
>> around, we should try to give their ideas a fair shake.
>>
> I share the same feeling. I wasn't trying to throw a cold water on it.

OK, I felt like that's what you were doing.  Sorry if I
misinterpreted, and thanks for clarifying.

kolo hhmow: I suggest adding your patch to
https://commitfest.postgresql.org/action/commitfest_view/open so it
doesn't get forgotten about.  Hopefully someone who knows more about
this area will volunteer to review.

-- 
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] [RFC] overflow checks optimized away

[Michael Paquier]

On Fri, Oct 9, 2015 at 10:52 PM, Bruce Momjian  wrote:
>
> Any news on this item from 2013, worked on again 2014?
>
> ---
>
> On Wed, Aug  6, 2014 at 12:55:59PM -0400, Bruce Momjian wrote:
>> On Fri, Nov 29, 2013 at 02:04:10AM +, Greg Stark wrote:
>> > Attached is what I have so far. I have to say I'm starting to come
>> > around to Tom's point of view. This is a lot of hassle for not much
>> > gain. i've noticed a number of other overflow checks that the llvm
>> > optimizer is not picking up on so even after this patch it's not clear
>> > that all the signed overflow checks that depend on -fwrapv will be
>> > gone.
>> >
>> > This patch still isn't quite finished though.
>> >
>> > a) It triggers a spurious gcc warning about overflows on constant
>> > expressions. These value of these expressions aren't actually being
>> > used as they're used in the other branch of the overflow test. I think
>> > I see how to work around it for gcc using __builtin_choose_expr but it
>> > might be pretty grotty.
>> >
>> > b) I'm concerned these checks depend on INT_MIN/MAX and SHRT_MIN/MAX
>> > which may not be exactly the right length. I'm kind of confused why
>> > c.h assumes long is 32 bits and short is 16 bits though so I don't
>> > think I'm making it any worse. It may be better for us to just define
>> > our own limits since we know exactly how large we expect these data
>> > types to be.
>> >
>> > c) I want to add regression tests that will ensure that the overflow
>> > checks are all working. So far I haven't been able to catch any being
>> > optimized away even with -fno-wrapv and -fstrict-overflow. I think I
>> > just didn't build with the right options though and it should be
>> > possible.
>> >
>> > The goal here imho is to allow building with -fno-wrapv
>> > -fstrict-overflow safely. Whether we actually do or not is another
>> > question but it means we would be able to use new compilers like clang
>> > without worrying about finding the equivalent of -fwrapv for them.
>>
>> Where are we on this?

Well, I have played a bit with this patch and rebased it as attached.
One major change is the use of the variables PG_INT* that have been
added in 62e2a8d. Some places were not updated with those new checks,
in majority a couple of routines in int.c (I haven't finished
monitoring the whole code though). Also, I haven't played yet with my
compilers to optimize away some of the checks and break them, but I'll
give it a try with clang and gcc. For now, I guess that this patch is
a good thing to begin with though, I have checked that code compiles
and regression tests pass.
Regards,
-- 
Michael
diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index 29f058c..9b4b890 100644
--- a/src/backend/executor/execQual.c
+++ b/src/backend/executor/execQual.c
@@ -3185,7 +3185,8 @@ ExecEvalArray(ArrayExprState *astate, ExprContext *econtext,
 /* Get sub-array details from first member */
 elem_ndims = this_ndims;
 ndims = elem_ndims + 1;
-if (ndims <= 0 || ndims > MAXDIM)
+
+if (PG_INT32_ADD_OVERFLOWS(elem_ndims,1) || ndims > MAXDIM)
 	ereport(ERROR,
 			(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
 		  errmsg("number of array dimensions (%d) exceeds " \
diff --git a/src/backend/utils/adt/array_userfuncs.c b/src/backend/utils/adt/array_userfuncs.c
index c14ea23..a6b0d1b 100644
--- a/src/backend/utils/adt/array_userfuncs.c
+++ b/src/backend/utils/adt/array_userfuncs.c
@@ -113,8 +113,9 @@ array_append(PG_FUNCTION_ARGS)
 		ub = dimv[0] + lb[0] - 1;
 		indx = ub + 1;
 
-		/* overflow? */
-		if (indx < ub)
+		/* check for overflow in upper bound (indx+1) */
+		if (PG_INT32_ADD_OVERFLOWS(dimv[0],lb[0]) ||
+			PG_INT32_ADD_OVERFLOWS(dimv[0]+lb[0], 1))
 			ereport(ERROR,
 	(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
 	 errmsg("integer out of range")));
@@ -168,7 +169,7 @@ array_prepend(PG_FUNCTION_ARGS)
 		lb0 = lb[0];
 
 		/* overflow? */
-		if (indx > lb[0])
+		if (PG_INT32_ADD_OVERFLOWS(lb[0], -1))
 			ereport(ERROR,
 	(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
 	 errmsg("integer out of range")));
diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index 4e927d8..cc4c570 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -2763,12 +2763,12 @@ width_bucket_float8(PG_FUNCTION_ARGS)
 			result = 0;
 		else if (operand >= bound2)
 		{
-			result = count + 1;
 			/* check for overflow */
-			if (result < count)
+			if (PG_INT32_ADD_OVERFLOWS(count, 1))
 ereport(ERROR,
 		(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
 		 errmsg("integer out of range")));
+			result = count + 1;
 		}
 		else
 			result = ((float8) count * (operand - bound1) / (bound2 - bound1)) + 1;
@@ -2779,12 +2779,12 @@ width_bucket_float8(PG_FUNCTION_ARGS)
 			result = 0;
 		else if (operand <= bound2)
 		{
-			result = count + 1;
 			/* 

Re: [HACKERS] pam auth - add rhost item

[Euler Taveira]

On 16-10-2015 10:37, Robert Haas wrote:

- Did he implement this correctly?

>

- Would it break anything?

>
I did not review the patch.


- Are there lots of other knobs we should expose too instead of just one?

>
We are providing PAM_USER and PAM_CONV. The complete list of options are 
[1]. Maybe PAM_RUSER? BTW, we could use pg_ident.conf to map user foo 
(app) to user bar (PAM).



- What would it take to turn this into a committable patch?

>
Review?


- Would the cost of exposing this and perhaps some other knobs cost
too much in performance for the number of people it would make happy?

>
No.


- If so, should the behavior be GUC-controlled or is there
justification for arguing we should drop the whole patch?

The patch always set PAM_RHOST, ie. it means I can't disable it (at the 
postgres side). Is it a problem? Of course the PAM module can provide a 
way to ignore it but it is not our business.



I feel like we've got somebody new showing up to our community with an
idea that is not obviously stupid.  If we want such people to stick
around, we should try to give their ideas a fair shake.


I share the same feeling. I wasn't trying to throw a cold water on it.


[1] http://pubs.opengroup.org/onlinepubs/8329799/pam_set_item.htm


--
   Euler Taveira   Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento


--
Sent 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 v3] GSSAPI encryption support

[Craig Ringer]

On 16 October 2015 at 21:34, Stephen Frost  wrote:
> * Craig Ringer (cr...@2ndquadrant.com) wrote:
>> On 16 October 2015 at 01:07, Robbie Harwood  wrote:
>> > Looking at
>> > http://www.postgresql.org/docs/9.4/static/protocol-message-formats.html
>> > suggests that SSPI follows a separate codepath from the GSS code;
>> > certainly it's a different auth request, which means it shouldn't
>> > trigger the encryption path.
>>
>> It's a different auth request, but the handling in be-auth.c is
>> co-mingled to handle the cases:
>
> be-auth.c?  You mean src/backend/libpq/auth.c?

Ahem. Yes.

Also, I was actually thinking of the libpq front-end code, as it turns
out. The backend's handling of each method is much better separated.

>> * We're compiled with Kerberos, handling SSPI from Windows
>> * We're compiled with Kerberos, handling a GSSAPI request
>
> Eh, *kerberos* was removed with 9.4.0.  I'm guessing you mean GSSAPI
> above.

Yes, being sloppy.

> src/backend/libpq/auth.c and src/interfaces/libpq/fe-auth.c have pretty
> clearly independent paths for SSPI and GSSAPI

fe-auth.c doesn't. See pg_fe_sendauth(...)'s case AUTH_REQ_GSS: and
AUTH_REQ_SSPI:

Not what I call clearly independent. I've had to deal with that tangle
of joy a few times...

> and which is used
> (server-side) is based on what the auth method in pg_hba.conf is.

Yep, agreed there. I was thinking that the backend overlapped more,
like the frontend does.

>  In a
> properly set up environment, the client could be using either, but it's
> not like we try to figure out (or care) which the client is using from
> the server's perspective.

Exactly. I want to make sure we still don't have to care.

> Now, if you set up GSSAPI on the client side and tell it to require
> encryption and you're using SSPI on the server side (or the other way
> around) it's not likely to work without changes to the SSPI code paths,
> which we should be looking at doing.

So long as setting up gssapi auth on the backend without requiring
encryption still works with sspi clients, like it did before, I'm
happy. My concern is avoiding a regression in what works, not
requiring that the new functionality be supported everywhere.

>> > There is no reason that using GSSAPI from, e.g., MIT Kerberos for
>> > Windows, should not work here.
>>
>> Except that *nobody* does it. The EDB installer builds Pg with SSPI,
>> and that's what the overwhelming majority of Windows users use.
>
> Apparently *somebody* uses MIT KfW

Yeah, I meant nobody uses it with Pg on Windows.

> I doubt anyone's tried building or running PG with GSSAPI under Windows
> though.  I agree it'd be good to test and see and perhaps even the EDB
> installers could be changed to add support for it back in (not sure if
> Dave really wants to though as it was a bit of a pain..).

It's a lot of a pain, and not just because of maintenance. Kerberos on
Windows needs access to various innards that you don't need when just
using SSPI to delegate SSO to the OS. It's less secure, fiddly, and
annoying. At least it was last time I had to deal with it, when I was
working around some SSPI bugs in psqlODBC before landing up patching
them in the driver instead.

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


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

Re: [HACKERS] TODO list updates

[Alvaro Herrera]

Bruce Momjian wrote:
> I have spend the past few days updating our TODO list, removing
> completed and now-unnecessary items:
> 
>   https://wiki.postgresql.org/wiki/Todo

Thanks.  We have "TodoDone" pages for items that were done in specific
releases, but only for 8.4, 9.0 and 9.1.  I guess it was too much work
to create those.  (See https://wiki.postgresql.org/wiki/Category:Todo ).

I added a link to the latest posted version of bitmap indexes by Abhijit
Menon-Sen, for completeness.

-- 
Á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] TODO list updates

[Alvaro Herrera]

Bruce Momjian wrote:

> Are you suggesting I remove those links?  It is kind of odd to have
> links to patches for features we don't want, or just keep it?

No, quite the contrary -- I think the links allow some other person
research the issue including the history of patches and discussion, and
decide for themselves whether the feature is right to be rejected.

-- 
Á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] Error creating gin index on jsonb columns

[Tom Lane]

Glenn Zhu  writes:
> Is this categorized as a bug specific to GIN indexes or a PostgreSQL bug in
> general?

My gut says it's GIN-specific, but that's really only an educated guess;
we have too little info.

What I would recommend is that you get the data onto a non-production
machine where you can play around a bit more.  One thing you could do
then is run a build with debug symbols, attach to the backend process
with gdb, and set a breakpoint at "errfinish".  Then provoke the error,
and capture a backtrace from the call to errfinish.  That would greatly
narrow things down, though it might not be enough to isolate the bug
immediately.  (If I had a test case in hand, that's exactly the first
step I would take with it, but maybe you can do it for me.)

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] TODO list updates

[Alvaro Herrera]

Bruce Momjian wrote:

> Probably the most controvertial change was to move on-disk bitmap
> indexes to the "not wanted" section, though I kept the links in case we
> change our minds.  I just can't see how they would be a win with GIN and
> in-memory bitmaps.

Yeah, I recall we discussed bitmap indexes a lot and we found there
wasn't a lot of room to use them because GIN is just too good, it seems.
Also, the patches that were developed had a number of issues.  Anyone
wanting to develop bitmap indexes would probably be better off starting
from scratch.

> (I don't think BRIN indexes help for on-disk bitmap use-cases, do
> they?)

No, they don't.  I expect BRIN to be very bad in a limited domain (which
is where bitmap indexes are supposed to shine), except under specific
conditions.

-- 
Á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] Error creating gin index on jsonb columns

[Glenn Zhu]

We will follow your instructions and get back to you.

Thank you Tom.  Much appreciated!
-glenn

On Fri, Oct 16, 2015 at 7:16 AM, Tom Lane  wrote:

> Glenn Zhu  writes:
> > Is this categorized as a bug specific to GIN indexes or a PostgreSQL bug
> in
> > general?
>
> My gut says it's GIN-specific, but that's really only an educated guess;
> we have too little info.
>
> What I would recommend is that you get the data onto a non-production
> machine where you can play around a bit more.  One thing you could do
> then is run a build with debug symbols, attach to the backend process
> with gdb, and set a breakpoint at "errfinish".  Then provoke the error,
> and capture a backtrace from the call to errfinish.  That would greatly
> narrow things down, though it might not be enough to isolate the bug
> immediately.  (If I had a test case in hand, that's exactly the first
> step I would take with it, but maybe you can do it for me.)
>
> regards, tom lane
>



-- 
Glenn Zhu
SaaS Operations | ❖ Medallia, Inc.

Re: [HACKERS] TODO list updates

[Bruce Momjian]

On Fri, Oct 16, 2015 at 11:43:10AM -0300, Alvaro Herrera wrote:
> Bruce Momjian wrote:
> 
> > Probably the most controvertial change was to move on-disk bitmap
> > indexes to the "not wanted" section, though I kept the links in case we
> > change our minds.  I just can't see how they would be a win with GIN and
> > in-memory bitmaps.
> 
> Yeah, I recall we discussed bitmap indexes a lot and we found there
> wasn't a lot of room to use them because GIN is just too good, it seems.
> Also, the patches that were developed had a number of issues.  Anyone
> wanting to develop bitmap indexes would probably be better off starting
> from scratch.

Yes, that was my conclusion too.  We have played with the on-disk bitmap
idea for a long time, but GIN has gotten very good in that time.

Are you suggesting I remove those links?  It is kind of odd to have
links to patches for features we don't want, or just keep it?

> > (I don't think BRIN indexes help for on-disk bitmap use-cases, do
> > they?)
> 
> No, they don't.  I expect BRIN to be very bad in a limited domain (which
> is where bitmap indexes are supposed to shine), except under specific
> conditions.

Yes, that was my conclusion too.  Thanks.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +


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

Re: [HACKERS] Improve the concurency of vacuum full table and select statement on the same relation

[Robert Haas]

On Thu, Oct 15, 2015 at 8:28 PM, Jim Nasby  wrote:
> It's just how the authors of pg_repack decided to handle it. It seems pretty
> reasonable, since you probably don't want an errant DDL statement to cause
> the rollback of hours or days of pg_repack work.
>
> Ultimately, I don't think you'll find many people interested in working on
> this, because the whole goal is to never need VACUUM FULL or pg_repack. If
> you're clustering just for the sake of clustering, that has it's own set of
> difficulties that should be addressed.

I think the topic of online table reorganization is a pretty important
one, actually.  That is a need that we have had for a long time,
creates serious operational problems for users, and it's also a need
that is not going away.  I think the chances of eliminating that need
completely, even if we rearchitected or heap storage, are nil.

I think the bigger issue is that it's a very hard problem to solve.
pg_repack is one approach, but I've heard more than one person say
that, as C-3PO said about the asteroid, it may not be entirely stable.

-- 
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] pam auth - add rhost item

[kolo hhmow]

Ok.
Thak you all!
:)

On Fri, Oct 16, 2015 at 5:20 PM, Robert Haas  wrote:

> On Fri, Oct 16, 2015 at 10:50 AM, Euler Taveira 
> wrote:
> >> I feel like we've got somebody new showing up to our community with an
> >> idea that is not obviously stupid.  If we want such people to stick
> >> around, we should try to give their ideas a fair shake.
> >>
> > I share the same feeling. I wasn't trying to throw a cold water on it.
>
> OK, I felt like that's what you were doing.  Sorry if I
> misinterpreted, and thanks for clarifying.
>
> kolo hhmow: I suggest adding your patch to
> https://commitfest.postgresql.org/action/commitfest_view/open so it
> doesn't get forgotten about.  Hopefully someone who knows more about
> this area will volunteer to review.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: [HACKERS] a raft of parallelism-related bug fixes

[Robert Haas]

On Mon, Oct 12, 2015 at 1:04 PM, Robert Haas  wrote:
> Attached are 14 patches.  Patches #1-#4 are
> essential for testing purposes but are not proposed for commit,
> although some of the code they contain may eventually become part of
> other patches which are proposed for commit.  Patches #5-#12 are
> largely boring patches fixing fairly uninteresting mistakes; I propose
> to commit these on an expedited basis.  Patches #13-14 are also
> proposed for commit but seem to me to be more in need of review.

Hearing no objections, I've now gone and committed #5-#12.

> 0013-Modify-tqueue-infrastructure-to-support-transient-re.patch
> attempts to address a deficiency in the tqueue.c/tqueue.h machinery I
> recently introduced: backends can have ephemeral record types for
> which they use backend-local typmods that may not be the same between
> the leader and the worker.  This patch has the worker send metadata
> about the tuple descriptor for each such type, and the leader
> registers the same tuple descriptor and then remaps the typmods from
> the worker's typmod space to its own.  This seems to work, but I'm a
> little concerned that there may be cases it doesn't cover.  Also,
> there's room to question the overall approach.  The only other
> alternative that springs readily to mind is to try to arrange things
> during the planning phase so that we never try to pass records between
> parallel backends in this way, but that seems like it would be hard to
> code (and thus likely to have bugs) and also pretty limiting.

I am still hoping someone will step up to review this.

> 0014-Fix-problems-with-ParamListInfo-serialization-mechan.patch, which
> I just posted on the Parallel Seq Scan thread as a standalone patch,
> fixes pretty much what the name of the file suggests.  This actually
> fixes two problems, one of which Noah spotted and commented on over on
> that thread.  By pure coincidence, the last 'make check' regression
> failure I was still troubleshooting needed a fix for that issue plus a
> fix to plpgsql_param_fetch.  However, as I mentioned on the other
> thread, I'm not quite sure which way to go with the change to
> plpgsql_param_fetch so scrutiny of that point, in particular, would be
> appreciated.  See also
> http://www.postgresql.org/message-id/CA+TgmobN=wadvautwsh-xqvcdovkerasuxw2k3r6vmpwig7...@mail.gmail.com

Noah's been helping with this issue on the other thread.  I'll revise
this patch along the lines discussed there and resubmit.

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


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

Re: [HACKERS] PoC: Partial sort

[Alexander Korotkov]

On Sun, Jun 7, 2015 at 11:01 PM, Peter Geoghegan  wrote:

> On Sun, Jun 7, 2015 at 8:10 AM, Andreas Karlsson 
> wrote:
> > Are you planning to work on this patch for 9.6?
>
> FWIW I hope so. It's a nice patch.
>

I'm trying to to whisk dust. Rebased version of patch is attached. This
patch isn't passing regression tests because of plan changes. I'm not yet
sure about those changes: why they happens and are they really regression?
Since I'm not very familiar with planning of INSERT ON CONFLICT and RLS,
any help is appreciated.

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


partial-sort-basic-3.patch
Description: Binary data


regression.diffs
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] Proposal: SET ROLE hook

[Joe Conway]

In many environments there is a policy requiring users to login using
unprivileged accounts, and then escalate their privileges if and when
they require it. In PostgreSQL this could be done by granting the
superuser role to an unprivileged user with noinherit, and then making
the superuser role nologin. Something like:

8<-
psql -U postgres
create user joe noinherit;
grant postgres to joe;
alter user postgres nologin;
\q
psql -U joe
-- do stuff *not requiring* escalated privs
set role postgres;
-- do stuff *requiring* escalated privs
reset role;
8<-

One of the problems with this is we would ideally like to know whenever
joe escalates himself to postgres. Right now that is not really possible
without doing untenable things such as logging every statement.

In order to address this issue, I am proposing a new SET ROLE hook. The
attached patch (role-esc-hook.diff) is I believe all we need. Then
extension authors could implement logging of privilege escalation.

A proof of concept extension patch is also attached. That one is not
meant to be applied, just illustrates one potential use of the hook. I
just smashed it on top of passwordcheck for the sake of convenience.

With both patches applied, the following scenario:
8<
psql -U joe postgres
psql (9.6devel)
Type "help" for help.

postgres=> set role postgres;
SET
postgres=# select rolname, rolpassword from pg_authid;
 rolname  | rolpassword
--+-
 joe  |
 postgres |
(2 rows)

postgres=# set log_statement = none;
SET
postgres=# reset role;
RESET
8<

Generates the following in the log:

8<
LOG:  Role joe transitioning to Superuser Role postgres
STATEMENT:  set role postgres;
LOG:  statement: select rolname, rolpassword from pg_authid;
LOG:  statement: set log_statement = none;
LOG:  Superuser Role postgres transitioning to Role joe
STATEMENT:  reset role;
8<

Note that we cannot prevent joe from executing
  set log_statement = none;
but we at least get the evidence in the log and can ask joe why he felt
the need to do that. We could also set up alerts based on the logged
events, etc.

This particular hook will not capture role changes due to SECURITY
DEFINER functions, but I think that is not only ok but preferred.
Escalation during a SECURITY DEFINER function is a preplanned sanctioned
event, unlike an ad hoc unconstrained role change to superuser. And
given the demo patch, we could see any SECURITY DEFINER function created
by the superuser when it gets created in the log (which again is subject
to auditing, alerts, etc.)

Ultimately I would also like to see a general hook available which would
fire for all changes to GUC settings, but I think this one is still very
useful as it is highly targeted.

Comments?

Thanks,

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 2d0a44e..76b3957 100644
*** a/src/backend/commands/variable.c
--- b/src/backend/commands/variable.c
***
*** 31,36 
--- 31,38 
  #include "utils/timestamp.h"
  #include "mb/pg_wchar.h"
  
+ SetRole_hook_type SetRole_hook = NULL;
+ 
  /*
   * DATESTYLE
   */
*** assign_role(const char *newval, void *ex
*** 904,909 
--- 906,919 
  {
  	role_auth_extra *myextra = (role_auth_extra *) extra;
  
+ 	/*
+ 	 * Any defined hooks must be able to execute in a failed
+ 	 * transaction to restore a prior value of the ROLE GUC variable.
+ 	 */
+ 	if (SetRole_hook)
+ 		(*SetRole_hook) (GetUserId(),
+ 		 myextra->roleid, myextra->is_superuser);
+ 
  	SetCurrentRoleId(myextra->roleid, myextra->is_superuser);
  }
  
diff --git a/src/include/commands/variable.h b/src/include/commands/variable.h
index 9cf2edd..be243df 100644
*** a/src/include/commands/variable.h
--- b/src/include/commands/variable.h
***
*** 12,17 
--- 12,20 
  
  #include "utils/guc.h"
  
+ /* Hook for plugins to get control in check_role() */
+ typedef void (*SetRole_hook_type) (Oid, Oid, bool);
+ extern PGDLLIMPORT SetRole_hook_type SetRole_hook;
  
  extern bool check_datestyle(char **newval, void **extra, GucSource source);
  extern void assign_datestyle(const char *newval, void *extra);
diff --git a/contrib/passwordcheck/passwordcheck.c b/contrib/passwordcheck/passwordcheck.c
index 78c44b2..867a703 100644
*** a/contrib/passwordcheck/passwordcheck.c
--- b/contrib/passwordcheck/passwordcheck.c
***
*** 21,28 
--- 21,31 
  #endif
  
  #include "commands/user.h"
+ #include "commands/variable.h"
  #include "fmgr.h"
+ #include "miscadmin.h"
  #include "libpq/md5.h"
+ #include "utils/memutils.h"
  
  PG_MODULE_MAGIC;
  
*** PG_MODULE_MAGIC;
*** 31,36 
--- 34,41 
  
  extern void _PG_init(void);
  
+ char	   *save_log_statement = NULL;
+ 
  

Re: [HACKERS] Obsolete use of volatile in walsender.c, walreceiver.c, walreceiverfuncs.c?

[Michael Paquier]

On Sat, Oct 17, 2015 at 3:21 AM, Robert Haas wrote:
> OK, committed his, and yours.
>
> I back-patched his spin.h comment fix to 9.5 since that's a factual
> error, but the rest of this seems like optimization so I committed it
> only to master.

That sounds right. Thanks!
-- 
Michael


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Fri, Oct 16, 2015 at 6:12 PM, Kouhei Kaigai  wrote:
> I think, it is right approach to pretend EPQ doesn't exist if scanrelid==0,
> because what replaced by these ForeignScan/CustomScan node are local join
> node like NestLoop. They don't have its own EPQ slot, but constructs joined-
> tuple based on the underlying scan-tuple originally stored within EPQ slots.

I think you've got that backwards.  The fact that they don't have
their own EPQ slot is the problem we need to solve.  When an EPQ
recheck happens, we rescan every relation in the query.  Each relation
needs to return 0 or 1 tuples.  If it returns a tuple, the tuple it
returns must be either the same tuple it previously returned, or an
updated version of that tuple.  But "the tuple it previously returned"
does not necessarily mean the tuple it returned most recently.  It
means the tuple that it returned which, when passed through the rest
of the plan, contributed to generate the result tuple that is being
rechecked.

Now, if you don't have an EPQ slot, how are you going to do this?
When the EPQ machinery engages, you need to somehow get the tuple you
previously returned stored someplace.  And the first time thereafter
that you get called by ExecProcNode, you need to return that tuple,
provided that it still passes the quals.  The second time you get
called, and any subsequent times, you need to return an empty slot.
The EPQ slot is well-suited to this task.  It's got a TupleTableSlot
to store the tuple you need to return, and it's got a flag indicating
whether you've already returned that tuple.  So you're good.

But with Etsuro Fujita's patch, and I think what you have proposed has
been similar, how are you going to do it?  The proposal is to call the
recheck method and hope for the best, but what is the recheck method
going to do?  Where is it going to get the previously-returned tuple?
How will it know if it has already returned it during the lifetime of
this EPQ check?  Offhand, it looks to me like, at least in some
circumstances, you're probably going to return whatever tuple you
returned most recently (which has a good chance of being the right
one, but not necessarily) over and over again.  That's not going to
fly.

The bottom line is that a foreign scan that is a pushed-down join is
still a *scan*, and every already-existing scan type has an EPQ slot
*for a reason*.  They *need* it in order to deliver the correct
behavior.  And foreign scans and custom scans need it to, and for the
same reason.

>> Again, the root of the problem here is that the EPQ machinery provides
>> 1 slot per RTI, and it uses scanrelid to figure out which EPQ slot is
>> applicable for a given scan node.  Here, we have scanrelid == 0, so it
>> gets confused.  But it's not the case that a pushed-down join has NO
>> scanrelid.  It actually has, in effect, *multiple* scanrelids.  So we
>> should pick any one of those, say the lowest-numbered one, and use
>> that to decide which EPQ slot to use.  The attached patch shows
>> roughly what I have in mind, although this is just crap code to
>> demonstrate the basic idea and doesn't pretend to adjust everything
>> that needs fixing.
>>
> One tricky point of this idea is ExecStoreTuple() in ExecScanFetch(),
> because the EPQ slot picked up by get_proxy_scanrelid() contains
> a tuple of base relation then it tries to put this tuple on the
> TupleTableSlot initialized to save the joined-tuple.
> Of course, recheckMtd is called soon, so callback will be able to
> handle the request correctly. However, it is a bit unnatural to store
> a tuple on incompatible TupleTableSlot.

I think that the TupleTableSlot is incompatible because the dummy
patch I posted only addresses half of the problem.  I didn't do
anything about the code that stores stuff into an EPQ slot.  If that
were also fixed, then the tuple which the patch retrieves from the
slot would not be incompatible.

-- 
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] Foreign join pushdown vs EvalPlanQual

[Tom Lane]

Kouhei Kaigai  writes:
>> On Fri, Oct 16, 2015 at 5:00 AM, Etsuro Fujita
>>  wrote:
>> I don't see how this can be right.  You're basically just pretending
>> EPQ doesn't exist in the remote join case, which isn't going to work
>> at all.  Those bits of code that look at es_epqTuple, es_epqTupleSet,
>> and es_epqScanDone are not optional.  You can't just skip over those
>> as if they don't matter.

> I think, it is right approach to pretend EPQ doesn't exist if scanrelid==0,
> because what replaced by these ForeignScan/CustomScan node are local join
> node like NestLoop.

That's just nonsense.  The reason that nestloop doesn't need its own EPQ
slot is that what it will be joining is tuples provided by scan nodes,
and it was the scan nodes that took care of fetching correct,
updated-if-need-be tuples for the EPQ check.  You can't just discard that
responsibility when you're implementing a join remotely ... at least not
if you want to preserve semantics similar to what happens with local
tables.

Or maybe I misunderstood what you meant, but it's certainly not OK to say
that EPQ is a no-op for a pushed-down join.  Rather, it has to perform all
the same checks that would have happened for any of its constitutent
tables.

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] pageinspect patch, for showing tuple data

[Michael Paquier]

On Sat, Oct 17, 2015 at 5:15 AM, Nikolay Shaplov
 wrote:
> So what's next?

Wait and see a bit.

> We need something else to discuss?
> We need somebody else's opinion to rule this out?

The spec of the patch looks clear to me.

> Or it's ready to commit, and just not marked this way?

No, I don't think we have reached this state yet.

> I am going to make report based on this patch in Vienna. It would be
> nice to have it committed until then :)

This is registered in this November's CF and the current one is not
completely wrapped up, so I haven't rushed into looking at it.
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] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> Kouhei Kaigai  writes:
> >> On Fri, Oct 16, 2015 at 5:00 AM, Etsuro Fujita
> >>  wrote:
> >> I don't see how this can be right.  You're basically just pretending
> >> EPQ doesn't exist in the remote join case, which isn't going to work
> >> at all.  Those bits of code that look at es_epqTuple, es_epqTupleSet,
> >> and es_epqScanDone are not optional.  You can't just skip over those
> >> as if they don't matter.
> 
> > I think, it is right approach to pretend EPQ doesn't exist if scanrelid==0,
> > because what replaced by these ForeignScan/CustomScan node are local join
> > node like NestLoop.
> 
> That's just nonsense.  The reason that nestloop doesn't need its own EPQ
> slot is that what it will be joining is tuples provided by scan nodes,
> and it was the scan nodes that took care of fetching correct,
> updated-if-need-be tuples for the EPQ check.  You can't just discard that
> responsibility when you're implementing a join remotely ... at least not
> if you want to preserve semantics similar to what happens with local
> tables.
>
NestLoop itself does not need its own EPQ slot, no doubt. However,
entire sub-tree of NestLoop takes at least two underlying EPQ slots
of the base relations to be joined.

My opinion is, simply, ForeignScan/CustomScan with scanrelid==0 takes
over the responsibility of EPQ recheck of entire join sub-tree that is
replaced by the ForeignScan/CustomScan node.
If ForeignScan run a remote join on foreign tables: A and B, it shall
apply both of scan-quals and join-clause towards the tuples kept in
the EPQ slots, in some fashion depending on FDW implementation.

Nobody concerned about what check shall be applied here. EPQ recheck
shall be applied as if entire join sub-tree exists here.
Major difference between I and Fujita-san is how to recheck it.
I think FDW knows the best way to do (even if we can provide utility
routines for majority cases), however, Fujita-san says a particular
implementation is the best for all the people. I cannot agree with
his opinion.

> Or maybe I misunderstood what you meant, but it's certainly not OK to say
> that EPQ is a no-op for a pushed-down join.  Rather, it has to perform all
> the same checks that would have happened for any of its constitutent
> tables.
>
I've never said that EPQ should be no-op for a pushed-down join.
Responsibility of the entire join sub-tree is not discarded, and
not changed, even if it is replaced by a remote-join.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 


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

Re: [HACKERS] Parallel Seq Scan

[Noah Misch]

On Thu, Oct 15, 2015 at 04:30:01PM +0530, Amit Kapila wrote:
> On Mon, Oct 12, 2015 at 9:16 PM, Robert Haas  wrote:
> > plpgsql_param_fetch() assumes that it can detect whether it's being
> > called from copyParamList() by checking whether params !=
> > estate->paramLI.  I don't know why this works, but I do know that this
> > test fails to detect the case where it's being called from
> > SerializeParamList(), which causes failures in exec_eval_datum() as
> > predicted.  Calls from SerializeParamList() need the same treatment as
> > calls from copyParamList() because it, too, will try to evaluate every
> > parameter in the list.
> 
> From what I understood by looking at code in this area, I think the check
> params != estate->paramLI and code under it is required for parameters
> that are setup by setup_unshared_param_list().  Now unshared params
> are only created for Cursors and expressions that are passing a R/W
> object pointer; for cursors we explicitly prohibit the parallel
> plan generation
> and I am not sure if it makes sense to generate parallel plans for
> expressions
> involving R/W object pointer, if we don't generate parallel plan where
> expressions involve such parameters, then SerializeParamList() should not
> be affected by the check mentioned by you.

The trouble comes from the opposite direction.  A setup_unshared_param_list()
list is fine under today's code, but a shared param list needs more help.  To
say it another way, parallel queries that use the shared estate->paramLI need,
among other help, the logic now guarded by "params != estate->paramLI".


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Fri, Oct 16, 2015 at 7:48 PM, Kouhei Kaigai  wrote:
> My opinion is, simply, ForeignScan/CustomScan with scanrelid==0 takes
> over the responsibility of EPQ recheck of entire join sub-tree that is
> replaced by the ForeignScan/CustomScan node.
> If ForeignScan run a remote join on foreign tables: A and B, it shall
> apply both of scan-quals and join-clause towards the tuples kept in
> the EPQ slots, in some fashion depending on FDW implementation.

And my opinion, as I said before, is that's completely wrong.  The
ForeignScan which represents a pushed-down join is a *scan*.  In
general, scans have one EPQ slot, and that is the right number.  This
pushed-down join scan, though, is in a state of confusion.  The code
that populates the EPQ slots thinks it's got multiple slots, one per
underlying relation.  Meanwhile, the code that reads data back out of
those slots thinks it doesn't have any slots at all.  Both of those
pieces of code are wrong.  This foreign scan, like any other scan,
should use ONE slot.

Both you and Etsuro Fujita are proposing to fix this problem by
somehow making it the FDW's problem to reconstruct the tuple
previously produced by the join from whole-row images of the baserels.
But that's not looking back far enough: why are we asking for
whole-row images of the baserels when what we really want is a
whole-row image of the output of the join?  The output of the join is
what we need to re-return.

-- 
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] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> On Fri, Oct 16, 2015 at 5:00 AM, Etsuro Fujita
>  wrote:
> > As for #2, I updated the patch, which uses a local join execution plan for
> > an EvalPlanQual rechech, according to the comment from Robert [1]. Attached
> > is an updated version of the patch.  This is a WIP patch, but it would be
> > appreciated if I could get feedback earlier.
> 
> I don't see how this can be right.  You're basically just pretending
> EPQ doesn't exist in the remote join case, which isn't going to work
> at all.  Those bits of code that look at es_epqTuple, es_epqTupleSet,
> and es_epqScanDone are not optional.  You can't just skip over those
> as if they don't matter.
>
I think, it is right approach to pretend EPQ doesn't exist if scanrelid==0,
because what replaced by these ForeignScan/CustomScan node are local join
node like NestLoop. They don't have its own EPQ slot, but constructs joined-
tuple based on the underlying scan-tuple originally stored within EPQ slots.

> Again, the root of the problem here is that the EPQ machinery provides
> 1 slot per RTI, and it uses scanrelid to figure out which EPQ slot is
> applicable for a given scan node.  Here, we have scanrelid == 0, so it
> gets confused.  But it's not the case that a pushed-down join has NO
> scanrelid.  It actually has, in effect, *multiple* scanrelids.  So we
> should pick any one of those, say the lowest-numbered one, and use
> that to decide which EPQ slot to use.  The attached patch shows
> roughly what I have in mind, although this is just crap code to
> demonstrate the basic idea and doesn't pretend to adjust everything
> that needs fixing.
>
One tricky point of this idea is ExecStoreTuple() in ExecScanFetch(),
because the EPQ slot picked up by get_proxy_scanrelid() contains
a tuple of base relation then it tries to put this tuple on the
TupleTableSlot initialized to save the joined-tuple.
Of course, recheckMtd is called soon, so callback will be able to
handle the request correctly. However, it is a bit unnatural to store
a tuple on incompatible TupleTableSlot.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 


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

[HACKERS] WIP: lookbehind constraints for our regexp engine

[Tom Lane]

I've occasionally heard complaints that our regex engine only has
lookahead constraints not lookbehind constraints, while Perl's for example
does have those.  While I was fooling around with the other issues in that
code, I learned enough to realize that it would not be that hard to
implement such things, and the attached proof-of-concept patch does so
(using Perl-compatible syntax, in case you're wondering).

However, I don't feel like this is done to the point of being committable,
because its performance leaves something to be desired in a lot of cases.
The difficulty is that because the engine can only match in a forward
direction, in the worst case you have to check a lookbehind constraint by
trying to match starting from the string start to see if a match exists
ending at the current-location where you're testing the constraint.  That
makes it, worst case, O(N^2) to search a string of length N.  Now, you can
improve on that greatly if you can determine that possible matches don't
extend all the way back to the string start.  In the attached patch I use
the "cold start pointer" returned by shortest() to decide that characters
before the coldstart point no longer have to be rechecked.  That helps
some, but it's not good enough because there are too many cases where the
engine doesn't move the coldstart point forward very aggressively.

It might be that that behavior itself can be improved on, which would
be nice because there are also non-lookbehind-related scenarios where
smarter coldstart detection would help performance.  But I have no very
good ideas about how to do it.

Another idea I've been toying with is to add logic that tries to determine
the maximum possible match length for a regex.  If we can determine that
for the lookbehind sub-regex, then we'd know we have to back up at most
that far before applying the match check.  This seems promising because
a lot of other engines don't even support variable-length lookbehind
patterns at all, and so we'd be assured of good performance in all the
cases that competitors can handle at all.

Anyway, I'm not planning to do much more work on this right now, but
I thought I'd throw it out there just to let people know that this seems
within reach.  I'm curious to know how many people care, and how much.

regards, tom lane

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 2946122..4d482ec 100644
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
*** SELECT foo FROM regexp_split_to_table('t
*** 4477,4489 
 where no substring matching re begins
 (AREs only) 
 

   
  
  
 
! Lookahead constraints cannot contain back references
! (see ),
  and all parentheses within them are considered non-capturing.
 
 
--- 4477,4503 
 where no substring matching re begins
 (AREs only) 
 
+ 
+
+ (?=re) 
+ positive lookbehind matches at any point
+where a substring matching re ends
+(AREs only) 
+
+ 
+
+ (?!re) 
+ negative lookbehind matches at any point
+where no substring matching re ends
+(AREs only) 
+

   
  
  
 
! Lookahead and lookbehind constraints cannot contain back
! references (see ),
  and all parentheses within them are considered non-capturing.
 
 
*** SELECT regexp_matches('abc01234xyz', '(?
*** 5355,5361 
  the lack of special treatment for a trailing newline,
  the addition of complemented bracket expressions to the things
  affected by newline-sensitive matching,
! the restrictions on parentheses and back references in lookahead
  constraints, and the longest/shortest-match (rather than first-match)
  matching semantics.
 
--- 5369,5375 
  the lack of special treatment for a trailing newline,
  the addition of complemented bracket expressions to the things
  affected by newline-sensitive matching,
! the restrictions on parentheses and back references in lookahead/lookbehind
  constraints, and the longest/shortest-match (rather than first-match)
  matching semantics.
 
diff --git a/src/backend/regex/README b/src/backend/regex/README
index 5c24d3d..6c9f483 100644
*** a/src/backend/regex/README
--- b/src/backend/regex/README
*** The possible arc types are:
*** 332,341 
  as "$0->to_state" or "$1->to_state" for end-of-string and end-of-line
  constraints respectively.
  
! LACON constraints, which represent "(?=re)" and "(?!re)" constraints,
! i.e. the input starting at this point must match (or not match) a
! given sub-RE, but the matching input is not consumed.  These are
! dumped as ":subtree_number:->to_state".
  
  If you see anything else (especially any question marks) in the display of
  an arc, it's dumpnfa() trying to tell you that there's something 

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Tom Lane]

Robert Haas  writes:
> Both you and Etsuro Fujita are proposing to fix this problem by
> somehow making it the FDW's problem to reconstruct the tuple
> previously produced by the join from whole-row images of the baserels.
> But that's not looking back far enough: why are we asking for
> whole-row images of the baserels when what we really want is a
> whole-row image of the output of the join?  The output of the join is
> what we need to re-return.

There are multiple components to the requirement though:

1. Recheck the rows that were in the baserels and possibly fetch updated
versions of them.  (Once we're in EPQ, we want the most recent row
versions, not what the query snapshot can see.)

2. Apply relevant restriction clauses and see if the updated row versions
still pass the clauses.

3. If so, form a join row and return that.  Else return NULL.

One way or another, the FDW has to do all of the above, or as much of it
as it can't fob off on the remote server, once it's decided to bypass
local implementation of the join.  Just recomputing the original join
row is *not* good enough.

I think what Kaigai-san and Etsuro-san are after is trying to find a way
to reuse some of the existing EPQ machinery to help with that.  This may
not be practical, or it may end up being messier than a standalone
implementation; but it's not silly on its face to want to reuse some of
that code.

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] [PATCH v3] GSSAPI encryption support

[Stephen Frost]

* Craig Ringer (cr...@2ndquadrant.com) wrote:
> On 16 October 2015 at 21:34, Stephen Frost  wrote:
> >> It's a different auth request, but the handling in be-auth.c is
> >> co-mingled to handle the cases:
> >
> > be-auth.c?  You mean src/backend/libpq/auth.c?
> 
> Ahem. Yes.

No worries. :)

> Also, I was actually thinking of the libpq front-end code, as it turns
> out. The backend's handling of each method is much better separated.

Ok.

> > src/backend/libpq/auth.c and src/interfaces/libpq/fe-auth.c have pretty
> > clearly independent paths for SSPI and GSSAPI
> 
> fe-auth.c doesn't. See pg_fe_sendauth(...)'s case AUTH_REQ_GSS: and
> AUTH_REQ_SSPI:
> 
> Not what I call clearly independent. I've had to deal with that tangle
> of joy a few times...

Hmm, yea, the actual choice of how to respond to the auth method the
server wants us to use is kind of tricky, but once we've settled on one
or the other, the rest of the code is pretty independent.

Still, I agree with your point that we need to be careful to not break
what we have there by, say, trying to move to encrypted GSSAPI with an
SSPI server.

I'm not sure that I want to really change that fe-auth.c code but I
really think there may be a simpler way to have the same behavior.  That
mishmash of #ifdef's and swith/case statements strikes me as possibly
being overly cute.  Clear blocks of "we got this message, this is what
we do when we're compiled with X, or Y, or X+Y" would probably be a lot
easier to follow.

> >  In a
> > properly set up environment, the client could be using either, but it's
> > not like we try to figure out (or care) which the client is using from
> > the server's perspective.
> 
> Exactly. I want to make sure we still don't have to care.

Agreed.

> > Now, if you set up GSSAPI on the client side and tell it to require
> > encryption and you're using SSPI on the server side (or the other way
> > around) it's not likely to work without changes to the SSPI code paths,
> > which we should be looking at doing.
> 
> So long as setting up gssapi auth on the backend without requiring
> encryption still works with sspi clients, like it did before, I'm
> happy. My concern is avoiding a regression in what works, not
> requiring that the new functionality be supported everywhere.

Great, we're definitely agreed on that.

> >> > There is no reason that using GSSAPI from, e.g., MIT Kerberos for
> >> > Windows, should not work here.
> >>
> >> Except that *nobody* does it. The EDB installer builds Pg with SSPI,
> >> and that's what the overwhelming majority of Windows users use.
> >
> > Apparently *somebody* uses MIT KfW
> 
> Yeah, I meant nobody uses it with Pg on Windows.

It'd be nice if it did, in case any of those users *do* end up wanting
to use PG.

> > I doubt anyone's tried building or running PG with GSSAPI under Windows
> > though.  I agree it'd be good to test and see and perhaps even the EDB
> > installers could be changed to add support for it back in (not sure if
> > Dave really wants to though as it was a bit of a pain..).
> 
> It's a lot of a pain, and not just because of maintenance. Kerberos on
> Windows needs access to various innards that you don't need when just
> using SSPI to delegate SSO to the OS. It's less secure, fiddly, and
> annoying. At least it was last time I had to deal with it, when I was
> working around some SSPI bugs in psqlODBC before landing up patching
> them in the driver instead.

Yeah, I remember working with KfW back-in-the-day.  I never played with
the new 4.0 version, so perhaps it's better, but I'm not particularly
anxious to get into that mess again..

As for this patch, the reason I've not been as involved (beyond being
ridiculously busy) is that Michael's environment, which at least appears
perfectly reasonable (and works with PG unpatched) isn't working.  If we
can get that working (and I've not looked at what's happening, so I have
no idea how easy or hard that would be), then I'd be a lot more excited
to spend time doing review of the patch.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Parallel Seq Scan

[Amit Kapila]

On Sat, Oct 17, 2015 at 6:15 AM, Noah Misch  wrote:
>
> On Thu, Oct 15, 2015 at 04:30:01PM +0530, Amit Kapila wrote:
> > On Mon, Oct 12, 2015 at 9:16 PM, Robert Haas 
wrote:
> > > plpgsql_param_fetch() assumes that it can detect whether it's being
> > > called from copyParamList() by checking whether params !=
> > > estate->paramLI.  I don't know why this works, but I do know that this
> > > test fails to detect the case where it's being called from
> > > SerializeParamList(), which causes failures in exec_eval_datum() as
> > > predicted.  Calls from SerializeParamList() need the same treatment as
> > > calls from copyParamList() because it, too, will try to evaluate every
> > > parameter in the list.
> >
> > From what I understood by looking at code in this area, I think the
check
> > params != estate->paramLI and code under it is required for parameters
> > that are setup by setup_unshared_param_list().  Now unshared params
> > are only created for Cursors and expressions that are passing a R/W
> > object pointer; for cursors we explicitly prohibit the parallel
> > plan generation
> > and I am not sure if it makes sense to generate parallel plans for
> > expressions
> > involving R/W object pointer, if we don't generate parallel plan where
> > expressions involve such parameters, then SerializeParamList() should
not
> > be affected by the check mentioned by you.
>
> The trouble comes from the opposite direction.  A
setup_unshared_param_list()
> list is fine under today's code, but a shared param list needs more
help.  To
> say it another way, parallel queries that use the shared estate->paramLI
need,
> among other help, the logic now guarded by "params != estate->paramLI".
>

Why would a parallel query need such a logic, that logic is needed mainly
for cursor with params and we don't want a parallelize such cases?


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

Re: [HACKERS] plpython is broken for recursive use

[Tom Lane]

I wrote:
> This seems like a very Rube-Goldbergian way of setting up a local
> namespace for the user-defined code.  I think perhaps what we should do
> is:

> 1. Compile the user-supplied code directly into a code object, without
> wrapping it in a "def".  (Hence, PLy_procedure_munge_source goes away
> entirely, which would be nice.)  Forget about generating a code object
> containing a call, too.

After further study, it appears this approach won't work because it
breaks "yield" --- AFAICT, Python only allows "yield" inside a "def".

At this point I think what we need is to find a way of passing the
function parameters honestly, that is, as actual parameters in the
manufactured call.  I've not looked into how that might be done.

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] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Fri, Oct 16, 2015 at 9:51 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> Both you and Etsuro Fujita are proposing to fix this problem by
>> somehow making it the FDW's problem to reconstruct the tuple
>> previously produced by the join from whole-row images of the baserels.
>> But that's not looking back far enough: why are we asking for
>> whole-row images of the baserels when what we really want is a
>> whole-row image of the output of the join?  The output of the join is
>> what we need to re-return.
>
> There are multiple components to the requirement though:
>
> 1. Recheck the rows that were in the baserels and possibly fetch updated
> versions of them.  (Once we're in EPQ, we want the most recent row
> versions, not what the query snapshot can see.)

Check.  But postgres_fdw, and probably quite a few other FDWs, use
early row locking.  So ROW_MARK_COPY is in use and we need not worry
about refetching rows.

> 2. Apply relevant restriction clauses and see if the updated row versions
> still pass the clauses.

Check.

> 3. If so, form a join row and return that.  Else return NULL.

Not check.

Suppose we've got two foreign tables ft1 and ft2, using postgres_fdw.
There is a local table t.  The user does something like UPDATE t SET
... FROM ft1, ft2 WHERE t = ft1.a AND ft1.b = ft2.b AND   The
query planner generates something like:

Update
-> Join
  -> Scan on t
  -> Foreign Scan on 

If an EPQ recheck occurs, the only thing that matters is that the
Foreign Scan return the right output row (or possibly now rows, if the
row it would have formed no longer matches the quals).  It doesn't
matter how it does this.  Let's say the columns actually needed by the
query from the ft1-ft2 join are ft1.a, ft1.b, ft2.a, and ft2.b.
Currently, the output of the foreign scan is something like: ft1.a,
ft1.b, ft2.a, ft.b, ft1.*, ft2.*.  The EPQ recheck has access to ft1.*
and ft2.*, but it's not straightforward for postgres_fdw to regenerate
the join tuple from that.  Maybe the pushed-down was a left join,
maybe it was a right join, maybe it was a full join.  So some of the
columns could have gone to NULL.  To figure it out, you need to build
a secondary plan tree that mimics the structure of the join you pushed
down, which is kinda hairy.

Think how much easier your life would be if you hadn't bothered
fetching ft1.* and ft2.*, which aren't so handy in this case, and had
instead made the output of the foreign scan ft1.a, ft1.b, ft2.a,
ft2.b, ROW(ft1.a, ft1.b, ft2.a, ft2.b) -- and that the output of that
ROW() operation was stored in an EPQ slot.  Now, you don't need the
secondary plan tree any more.  You've got all the data you need right
in your hand.  The values inside the ROW() constructor were evaluated
after accounting for the goes-to-NULL effects of any pushed-down
joins.

This example is of the early row locking case, but I think the story
is about the same if the FDW wants to do late row locking instead.  If
there's an EPQ recheck, it could issue individual row re-fetches
against every base table and then re-do all the joins that it pushed
down locally.  But it would be faster and cleaner, I think, to send
one query to the remote side that re-fetches all the rows at once, and
whose target list is exactly what we need, rather than whole row
targetlists for each baserel that then have to be rejiggered on our
side.

> I think what Kaigai-san and Etsuro-san are after is trying to find a way
> to reuse some of the existing EPQ machinery to help with that.  This may
> not be practical, or it may end up being messier than a standalone
> implementation; but it's not silly on its face to want to reuse some of
> that code.

Yeah, I think we're all in agreement that reusing as much of the EPQ
machinery as is sensible is something we should do.  We are not in
agreement on which parts of it need to be changed or extended.

-- 
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] Dangling Client Backend Process

[Amit Kapila]

On Fri, Oct 16, 2015 at 3:34 PM, Rajeev rastogi 
wrote:
>
>
> Yes it will be really helpful to know the earlier reason for "not making
> backend exit on postmaster death".
> Please let me know if there is any thread, which I can refer to find the
> same.
>
> IMHO there could be below major issues, if we don't kill client backend
> process on postmaster death:
> 1. Postmaster cannot be re-started again as pointed by Kyotaro and Andres
> Also.
> 2. If from existing client session, we try to do some operation which has
> dependency with backend process, then that operation will either fail or
> may lead to undefined behavior sometime.
> 3. Also unintentionally all operation done by application will not be
> checkpointed and also will not be flushed by bgworker.
> 4. Replicating of WAL to standby will be stopped and if synchronous
> standby is configured then command from master will be hanged.
>
>
What exactly we want to do as part of this proposal?  As far as I
can see, we have below two options on postmaster death:

a. Exit all the active backends in whichever state they are.
b. Exit backend/s after they finish there current work and are
waiting for new command.

I think what we are discussing here is option-b.


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

Re: [HACKERS] Proposal: SET ROLE hook

[Andres Freund]

On 2015-10-16 09:20:26 -0700, Joe Conway wrote:
> One of the problems with this is we would ideally like to know whenever
> joe escalates himself to postgres. Right now that is not really possible
> without doing untenable things such as logging every statement.

Alternatively you can just have a elevate_user() function that does the
logging and escalating? That seems like the same amount of code and it'd
work with released versions of postgres?

Sure, that has some disandvantages over your approach, but for the
presented use case with humans needing to escalate I don't see any.

Greetings,

Andres Freund


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

Re: [HACKERS] Proposal: SET ROLE hook

[Joe Conway]

On 10/16/2015 09:28 AM, Andres Freund wrote:
> Alternatively you can just have a elevate_user() function that does the
> logging and escalating? That seems like the same amount of code and it'd
> work with released versions of postgres?
> 
> Sure, that has some disadvantages over your approach, but for the
> presented use case with humans needing to escalate I don't see any.

Hmmm, do you mean essentially skip the "GRANT postgres to joe" and use a
SECURITY DEFINER C function that does the set role to postgres under the
covers with "GRANT EXECUTE on FUNCTION elevate_user() to joe"? Being
able to use something like that on existing versions would be very nice,
but it feels kind of grotty. Or maybe you mean something else?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature