Re: [HACKERS] Supporting TAP tests with MSVC and Windows

[Michael Paquier]

On Sat, Jul 25, 2015 at 10:54 PM, Michael Paquier
 wrote:
> An updated patch is attached.

Attached is v9, that fixes conflicts with 01f6bb4 and recent commits
that added TAP tests in pg_basebackup series.
-- 
Michael


0001-TAP-tests-for-MSVC.patch
Description: binary/octet-stream

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

[HACKERS] Don'st start streaming after creating a slot in pg_receivexlog

[Andres Freund]

Hi,

Heikki complained about pg_receivexlog --create-slot starting to stream
in his talk in St. Petersburg. Saying that that makes it a hard to
script feature - and I have to agree. Since that option is new to 9.5 we
can should change that behaviour now if we decide to.

Michael, what do you think?

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] dblink: add polymorphic functions.

[Heikki Linnakangas]

On 07/18/2015 01:32 AM, Corey Huinker wrote:

So this patch would result in less C code while still adding 3 new
functions. Can anyone think of why that wouldn't be the best way to go?


Let's pursue the "CAST(srf() AS row_rtype)" syntax that Joe suggested 
upthread 
(http://www.postgresql.org/message-id/559a9643.9070...@joeconway.com). 
For some reason, the discussion went on around the details of the 
submitted patch after that, even though everyone seemed to prefer the 
CAST() syntax.


- Heikki



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

Re: [HACKERS] Don'st start streaming after creating a slot in pg_receivexlog

[Heikki Linnakangas]

On 07/29/2015 10:37 AM, Andres Freund wrote:

Heikki complained about pg_receivexlog --create-slot starting to stream
in his talk in St. Petersburg. Saying that that makes it a hard to
script feature - and I have to agree. Since that option is new to 9.5 we
can should change that behaviour now if we decide to.


To be clear, I think "pg_receivexlog --create-slot" should only create 
the slot, and exit.


- Heikki



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

Re: [HACKERS] Don'st start streaming after creating a slot in pg_receivexlog

[Simon Riggs]

On 29 July 2015 at 08:37, Andres Freund  wrote:


> Heikki complained about pg_receivexlog --create-slot starting to stream
> in his talk in St. Petersburg. Saying that that makes it a hard to
> script feature - and I have to agree. Since that option is new to 9.5 we
> can should change that behaviour now if we decide to.
>
> Michael, what do you think?
>

--drop-slot seems pointless, since you can just do that with psql

If we make --create-slot do nothing but add the slot, then that seems
pointless also

Would we need to add those options to all commands, when it can be done
with psql?

I'd suggest just removing --create-slot and --drop-slot altogether

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] pg_basebackup and replication slots

[Simon Riggs]

On 22 July 2015 at 05:43, Michael Paquier  wrote:


> Now, do we plan to do something about the creation of a slot. I
> imagine that it would be useful if we could have --create-slot to
> create a slot when beginning a base backup and if-not-exists to
> control the error flow. There is already CreateReplicationSlot for
> this purpose in streamutils.c so most of the work is already done.
> Also, when a base backup fails for a reason or another, we should try
> to drop the slot in disconnect_and_exit() if it has been created by
> pg_basebackup. if if-not-exists is true and the slot already existed
> when beginning, we had better not dropping it perhaps...


What is the purpose of creating a temporary slot?

My understand was that slots are supposed to be persistent so we should
create them before use.

If we create a slot when one is needed and then drop automatically on
session disconnect (as Heikki suggest), what benefit does using a slot give
us?

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Don'st start streaming after creating a slot in pg_receivexlog

[Michael Paquier]

On Wed, Jul 29, 2015 at 4:51 PM, Heikki Linnakangas  wrote:
> On 07/29/2015 10:37 AM, Andres Freund wrote:
>>
>> Heikki complained about pg_receivexlog --create-slot starting to stream
>> in his talk in St. Petersburg. Saying that that makes it a hard to
>> script feature - and I have to agree. Since that option is new to 9.5 we
>> can should change that behaviour now if we decide to.
>
> To be clear, I think "pg_receivexlog --create-slot" should only create the
> slot, and exit.

Even if I would like to make pg_recvlogical and pg_receivexlog behave
as similarly as possible by having an additional switch --start in
pg_receivexlog to control if it starts to stream or not, this ship has
already sailed for backward-compatibility's sake... Then let's just
create the slot and exit().
-- 
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] Typo in comment in ATPrepChangePersistence

[Heikki Linnakangas]

On 07/29/2015 05:26 AM, Amit Langote wrote:

Attached fixes a typo:

- * no permanent tables cannot reference unlogged ones.
+ * permanent tables cannot reference unlogged ones.


Thanks, fixed.

- Heikki


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

Re: [HACKERS] Don'st start streaming after creating a slot in pg_receivexlog

[Andres Freund]

On 2015-07-29 08:54:40 +0100, Simon Riggs wrote:
> --drop-slot seems pointless, since you can just do that with psql
> 
> If we make --create-slot do nothing but add the slot, then that seems
> pointless also

> Would we need to add those options to all commands, when it can be done
> with psql?

They work over the replication protocol, which is handy, because it can
be done with the same user/permissions as the normal pg_receivexlog.


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

Re: [HACKERS] more RLS oversights

[Dean Rasheed]

On 29 July 2015 at 02:36, Joe Conway  wrote:
> On 07/03/2015 10:03 AM, Noah Misch wrote:
>> (6) AlterPolicy() calls InvokeObjectPostAlterHook(PolicyRelationId, ...), but
>> CreatePolicy() and DropPolicy() lack their respective hook invocations.
>
> Patch attached. Actually AlterPolicy() was also missing its hook -- the
> existing InvokeObjectPostAlterHook() was only in rename_policy().
>
> I'm not 100% sure about the hook placement -- would appreciate if
> someone could confirm I got it correct.
>

The CreatePolicy() and AlterPolicy() changes look OK to me, but the
RemovePolicyById() change looks to be unnecessary ---
RemovePolicyById() is called only from doDeletion(), which in turned
is called only from deleteOneObject(), which already invokes the drop
hooks. So ISTM that RemovePolicyById() doesn't need to do anything,
right?

Regards,
Dean


-- 
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] Don'st start streaming after creating a slot in pg_receivexlog

[Heikki Linnakangas]

On 07/29/2015 10:58 AM, Michael Paquier wrote:

On Wed, Jul 29, 2015 at 4:51 PM, Heikki Linnakangas  wrote:

On 07/29/2015 10:37 AM, Andres Freund wrote:


Heikki complained about pg_receivexlog --create-slot starting to stream
in his talk in St. Petersburg. Saying that that makes it a hard to
script feature - and I have to agree. Since that option is new to 9.5 we
can should change that behaviour now if we decide to.


To be clear, I think "pg_receivexlog --create-slot" should only create the
slot, and exit.


Even if I would like to make pg_recvlogical and pg_receivexlog behave
as similarly as possible by having an additional switch --start in
pg_receivexlog to control if it starts to stream or not, this ship has
already sailed for backward-compatibility's sake... Then let's just
create the slot and exit().


Hmm. pg_receivelogical is basically a debugging tool. I don't think 
anyone will have it integrated into production scripts etc. So maybe we 
could just change it.


I'm not sure I understand the proposal though. If you don't specify 
--create-slot, nor --start, what would the program do? Nothing?


- Heikki



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

Re: [HACKERS] pg_basebackup and replication slots

[Andres Freund]

On 2015-07-29 08:57:38 +0100, Simon Riggs wrote:
> On 22 July 2015 at 05:43, Michael Paquier  wrote:
> 
> 
> > Now, do we plan to do something about the creation of a slot. I
> > imagine that it would be useful if we could have --create-slot to
> > create a slot when beginning a base backup and if-not-exists to
> > control the error flow. There is already CreateReplicationSlot for
> > this purpose in streamutils.c so most of the work is already done.
> > Also, when a base backup fails for a reason or another, we should try
> > to drop the slot in disconnect_and_exit() if it has been created by
> > pg_basebackup. if if-not-exists is true and the slot already existed
> > when beginning, we had better not dropping it perhaps...
> 
> 
> What is the purpose of creating a temporary slot?

> If we create a slot when one is needed and then drop automatically on
> session disconnect (as Heikki suggest), what benefit does using a slot give
> us?

The goal is to have a reliable pg_basebackup that doesn't fail due to
missing WAL (which can easily happen even with -X) . That seems like a
sane desire.  The point of using a temporary slot is to not have a
leftover slot afterwards, reserving resources. Especially important if
the basebackup actually failed...


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

Re: [HACKERS] pg_basebackup and replication slots

[Simon Riggs]

On 29 July 2015 at 09:09, Andres Freund  wrote:

> On 2015-07-29 08:57:38 +0100, Simon Riggs wrote:
> > On 22 July 2015 at 05:43, Michael Paquier 
> wrote:
> >
> >
> > > Now, do we plan to do something about the creation of a slot. I
> > > imagine that it would be useful if we could have --create-slot to
> > > create a slot when beginning a base backup and if-not-exists to
> > > control the error flow. There is already CreateReplicationSlot for
> > > this purpose in streamutils.c so most of the work is already done.
> > > Also, when a base backup fails for a reason or another, we should try
> > > to drop the slot in disconnect_and_exit() if it has been created by
> > > pg_basebackup. if if-not-exists is true and the slot already existed
> > > when beginning, we had better not dropping it perhaps...
> >
> >
> > What is the purpose of creating a temporary slot?
>
> > If we create a slot when one is needed and then drop automatically on
> > session disconnect (as Heikki suggest), what benefit does using a slot
> give
> > us?
>
> The goal is to have a reliable pg_basebackup that doesn't fail due to
> missing WAL (which can easily happen even with -X) . That seems like a
> sane desire.


Agreed, that is sane. Slots are good.


> The point of using a temporary slot is to not have a
> leftover slot afterwards, reserving resources. Especially important if
> the basebackup actually failed...
>

Creating a slot and then deleting it if the session disconnects does not
successfully provide the functionality desired above.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Don'st start streaming after creating a slot in pg_receivexlog

[Simon Riggs]

On 29 July 2015 at 09:01, Andres Freund  wrote:

> On 2015-07-29 08:54:40 +0100, Simon Riggs wrote:
> > --drop-slot seems pointless, since you can just do that with psql
> >
> > If we make --create-slot do nothing but add the slot, then that seems
> > pointless also
>
> > Would we need to add those options to all commands, when it can be done
> > with psql?
>
> They work over the replication protocol, which is handy, because it can
> be done with the same user/permissions as the normal pg_receivexlog.
>

It's useful to separate permissions for creating/dropping a slot from using
it. A one-time act configures (or re-configures) how you wish the cluster
to look, another more regular task is to connect to the slot and use it.
But if you want to have a single user with privileges for both, you can
create that. I don't see it as something that we need to support in the
replication protocol, since that would require us to extend it every time
we think of something else.

Creating a temporary slot goes against the whole concept of slots, so using
the same id in the same script isn't actually needed, except maybe to
simplify testing.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Don'st start streaming after creating a slot in pg_receivexlog

[Shulgin, Oleksandr]

On Wed, Jul 29, 2015 at 10:02 AM, Heikki Linnakangas 
wrote:

> On 07/29/2015 10:58 AM, Michael Paquier wrote:
>
>> On Wed, Jul 29, 2015 at 4:51 PM, Heikki Linnakangas 
>> wrote:
>>
>>> On 07/29/2015 10:37 AM, Andres Freund wrote:
>>>

 Heikki complained about pg_receivexlog --create-slot starting to stream
 in his talk in St. Petersburg. Saying that that makes it a hard to
 script feature - and I have to agree. Since that option is new to 9.5 we
 can should change that behaviour now if we decide to.

>>>
>>> To be clear, I think "pg_receivexlog --create-slot" should only create
>>> the
>>> slot, and exit.
>>>
>>
>> Even if I would like to make pg_recvlogical and pg_receivexlog behave
>> as similarly as possible by having an additional switch --start in
>> pg_receivexlog to control if it starts to stream or not, this ship has
>> already sailed for backward-compatibility's sake... Then let's just
>> create the slot and exit().
>>
>
> Hmm. pg_receivelogical is basically a debugging tool. I don't think anyone
> will have it integrated into production scripts etc. So maybe we could just
> change it.
>
> I'm not sure I understand the proposal though. If you don't specify
> --create-slot, nor --start, what would the program do? Nothing?
>

Maybe we should make --start an optional flag for --create-slot in both
pg_recvlogical and pg_receivexlog?

So that if you didn't use --create-slot or --drop-slot, then it is assumed
that you want to start streaming.  And if you're using --create-slot, you
can still start streaming right away, though not by default.

At the moment I find it odd that pg_recvlogical -S myslot doesn't start
streaming and require you to use --start explicitly.

-- 
*Oleksandr Shulgin*
*Database Engineer*

Mobile: +49 160 84-90-639
Email: oleksandr.shul...@zalando.de

Re: [HACKERS] more RLS oversights

[Dean Rasheed]

On 29 July 2015 at 05:02, Joe Conway  wrote:
> On 07/03/2015 10:03 AM, Noah Misch wrote:
>> (7) Using an aggregate function in a policy predicate elicits an inapposite
>> error message due to use of EXPR_KIND_WHERE for parse analysis.  Need a new
>> ParseExprKind.  Test case:
>
> Patch attached. Comments?
>

I don't think there is any point in adding the new function
transformPolicyClause(), which is identical to transformWhereClause().
You can just use transformWhereClause() with EXPR_KIND_POLICY. It's
already used for lots of other expression kinds.

There are a couple of places in test_rls_hooks.c that also need updating.

I think that check_agglevels_and_constraints() and
transformWindowFuncCall() could be made to emit more targeted error
messages for EXPR_KIND_POLICY, for example "aggregate functions are
not allowed in policy USING and WITH CHECK expressions".

Regards,
Dean


-- 
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] Don'st start streaming after creating a slot in pg_receivexlog

[Andres Freund]

On 2015-07-29 09:23:43 +0100, Simon Riggs wrote:
> Creating a temporary slot goes against the whole concept of slots, so using
> the same id in the same script isn't actually needed, except maybe to
> simplify testing.

The concept of a slot is to reserve resources. I don't see why it's
wrong to reserve resources temporarily.


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

Re: [HACKERS] pg_basebackup and replication slots

[Andres Freund]

On 2015-07-29 09:17:04 +0100, Simon Riggs wrote:
> On 29 July 2015 at 09:09, Andres Freund  wrote:
> > The point of using a temporary slot is to not have a
> > leftover slot afterwards, reserving resources. Especially important if
> > the basebackup actually failed...
> >
> 
> Creating a slot and then deleting it if the session disconnects does not
> successfully provide the functionality desired above.

Uh? If you create the slot, start streaming, and then start the
basebackup, it does. It does *not* guarantee that the base backup can be
converted into a replica, but it's sufficient to guarantee it can
brought out of recovery.


-- 
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] Generalized JSON output functions

[Shulgin, Oleksandr]

>
> On July 13 I wrote:
>
>  Yes, but I think the plugin is the right place to do it. What is more,
>> this won't actually prevent you completely from producing non-ECMAScript
>> compliant JSON, since json or jsonb values containing offending numerics
>> won't be caught, AIUI. But a fairly simple to write function that reparsed
>> and fixed the JSON inside the decoder would work.
>>
>
> The OP admitted that this was a serious flaw in his approach. In fact,
> given that a json value can contain an offending numeric value, any
> approach which doesn't involve reparsing is pretty much bound to fail.
>

I agree that was a critical omission in my thinking.

Now, back to the whitespace issue: I could submit a patch to unify the
whitespace w/o all the hairy callbacks.  Did we have the consensus here: no
spaces whatsoever unless some *_pretty function is used?

--
Alex

Re: [HACKERS] deparsing utility commands

[Shulgin, Oleksandr]

On Tue, May 12, 2015 at 12:25 AM, Alvaro Herrera 
wrote:

> David Steele wrote:
>
> > I have reviewed and tested this patch and everything looks good to me.
> > It also looks like you added better coverage for schema DDL, which is a
> > welcome addition.
>
> Thanks -- I have pushed this now.
>

Hi,

I've tried compiling the 0003-ddl_deparse-extension part from
http://www.postgresql.org/message-id/20150409161419.gc4...@alvh.no-ip.org
on current master and that has failed because the 0002 part hasn't been
actually pushed (I've asked Alvaro off the list about this, that's how I
know the reason ;-).

I was able to update the 0002 part so it applies cleanly (updated version
attached), and then the contrib module compiles after one minor change and
seems to work.

I've started to look into what it would take to move 0002's code to the
extension itself, and I've got a question about use of printTypmod() in
format_type_detailed():

if (typemod >= 0)
*typemodstr = printTypmod(NULL, typemod, typeform->typmodout);
else
*typemodstr = pstrdup("");

Given that printTypmod() does psprintf("%s%s") one way or the other,
shouldn't we pass an empty string here instead of NULL as typname argument?

My hope is to get this test module extended quite a bit, not only to
> cover existing commands, but also so that it causes future changes to
> cause failure unless command collection is considered.  (In a previous
> version of this patch, there was a test mode that ran everything in the
> serial_schedule of regular regression tests.  That was IMV a good way to
> ensure that new commands were also tested to run under command
> collection.  That hasn't been enabled in the new test module, and I
> think it's necessary.)
>


> If anyone wants to contribute to the test module so that more is
> covered, that would be much appreciated.
>

I'm planning to have a look at this part also.

--
Alex
From 5381f5efafd1c2fd29b7c842e5ef1edd552d545e Mon Sep 17 00:00:00 2001
From: Oleksandr Shulgin 
Date: Wed, 29 Jul 2015 11:09:56 +0200
Subject: [PATCH] ddl_deparse core support

---
 src/backend/commands/seclabel.c |   5 +
 src/backend/commands/sequence.c |  34 +++
 src/backend/commands/tablecmds.c|   3 +-
 src/backend/utils/adt/format_type.c | 127 +
 src/backend/utils/adt/regproc.c | 101 +--
 src/backend/utils/adt/ruleutils.c   | 516 
 src/include/commands/sequence.h |   1 +
 src/include/utils/builtins.h|   4 +
 src/include/utils/ruleutils.h   |  21 +-
 9 files changed, 730 insertions(+), 82 deletions(-)

diff --git a/src/backend/commands/seclabel.c b/src/backend/commands/seclabel.c
index 1ef98ce..aa4de97 100644
--- a/src/backend/commands/seclabel.c
+++ b/src/backend/commands/seclabel.c
@@ -63,6 +63,11 @@ ExecSecLabelStmt(SecLabelStmt *stmt)
 	(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 	 errmsg("must specify provider when multiple security label providers have been loaded")));
 		provider = (LabelProvider *) linitial(label_provider_list);
+		/*
+		 * Set the provider in the statement so that DDL deparse can use
+		 * provider explicitly in generated statement.
+		 */
+		stmt->provider = (char *) provider->provider_name;
 	}
 	else
 	{
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 9c1037f..b0f8003 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1517,6 +1517,40 @@ process_owned_by(Relation seqrel, List *owned_by)
 		relation_close(tablerel, NoLock);
 }
 
+/*
+ * Return sequence parameters, detailed
+ */
+Form_pg_sequence
+get_sequence_values(Oid sequenceId)
+{
+	Buffer		buf;
+	SeqTable	elm;
+	Relation	seqrel;
+	HeapTupleData seqtuple;
+	Form_pg_sequence seq;
+	Form_pg_sequence retSeq;
+
+	retSeq = palloc(sizeof(FormData_pg_sequence));
+
+	/* open and AccessShareLock sequence */
+	init_sequence(sequenceId, &elm, &seqrel);
+
+	if (pg_class_aclcheck(sequenceId, GetUserId(),
+		  ACL_SELECT | ACL_UPDATE | ACL_USAGE) != ACLCHECK_OK)
+		ereport(ERROR,
+(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied for sequence %s",
+		RelationGetRelationName(seqrel;
+
+	seq = read_seq_tuple(elm, seqrel, &buf, &seqtuple);
+
+	memcpy(retSeq, seq, sizeof(FormData_pg_sequence));
+
+	UnlockReleaseBuffer(buf);
+	relation_close(seqrel, NoLock);
+
+	return retSeq;
+}
 
 /*
  * Return sequence parameters, for use by information schema
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d394713..5058f9f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -8169,7 +8169,8 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
 if (!list_member_oid(tab->changedConstraintOids,
 	 foundObject.objectId))
 {
-	char	   *defstring = pg_get_constraintdef_string(foundObject.objectId);
+	char	   *defstring = pg_get_constraintdef_string(foundObject.objectId,
+		true);
 
 		

Re: [HACKERS] REVOKE [ADMIN OPTION FOR] ROLE

[Oleksii Kliukin]

On Tue, Jul 28, 2015 at 10:51 AM, Egor Rogov  wrote:
>
> Well, I looked into a draft of SQL:2003. It basically says that "cascade"
> for  must behave the same way as for  privilege statement>. That is, from standard's point of view we have a code
> issue.
>
> Still I doubt about usefulness of this behavior. Do we really need it in
> PostgreSQL?

I think it's useful, as long as there are use-cases where instead of
granting privileges on the specific classes of database objects (i.e.
SELECT on all tables in a given schema) a role is granted instead
which 'accumulates' those privileges. This is the case in our
environment, and, while we are not using ADMIN OPTION, I can imagine
it  might be used in order to 'delegate' assignment of privileges from
the central authority to responsible sub-authorities in different
departments. Then, if you need to revoke those (i.e. because the
structure of departments had changed), it's enough to REVOKE ..FROM..
CASCADE instead of getting through each individual assignment case.

Kind regards,
--
Oleksii


-- 
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] LWLock deadlock and gdb advice

[Andres Freund]

Hi,

Finally getting to this.

On 2015-07-15 18:44:03 +0300, Heikki Linnakangas wrote:
> Previously, LWLockAcquireWithVar set the variable associated with the lock
> atomically with acquiring it. Before the lwlock-scalability changes, that
> was straightforward because you held the spinlock anyway, but it's a lot
> harder/expensive now. So I changed the way acquiring a lock with a variable
> works. There is now a separate flag, LW_FLAG_VAR_SET, which indicates that
> the current lock holder has updated the variable. The LWLockAcquireWithVar
> function is gone - you now just use LWLockAcquire(), which always clears the
> LW_FLAG_VAR_SET flag, and you can call LWLockUpdateVar() after that if you
> want to set the variable immediately.
>
> This passes make check, but I haven't done any testing beyond that. Does
> this look sane to you?

The prime thing I dislike about the patch is how long it now holds the
spinlock in WaitForVar. I don't understand why that's necessary? There's
no need to hold a spinlock until after the
   mustwait = (pg_atomic_read_u32(&lock->state) & LW_VAL_EXCLUSIVE) != 0;
unless I miss something?

In an earlier email you say:
> After the spinlock is released above, but before the LWLockQueueSelf() call,
> it's possible that another backend comes in, acquires the lock, changes the
> variable's value, and releases the lock again. In 9.4, the spinlock was not
> released until the process was queued.

But that's not a problem. The updater in that case will have queued a
wakeup for all waiters, including WaitForVar()?

I'll try to reproduce the problem now. But I do wonder if it's possibly
just the missing spinlock during the update.

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] pg_basebackup and replication slots

[Simon Riggs]

On 29 July 2015 at 11:43, Andres Freund  wrote:

> On 2015-07-29 09:17:04 +0100, Simon Riggs wrote:
> > On 29 July 2015 at 09:09, Andres Freund  wrote:
> > > The point of using a temporary slot is to not have a
> > > leftover slot afterwards, reserving resources. Especially important if
> > > the basebackup actually failed...
> > >
> >
> > Creating a slot and then deleting it if the session disconnects does not
> > successfully provide the functionality desired above.
>
> Uh? If you create the slot, start streaming, and then start the
> basebackup, it does. It does *not* guarantee that the base backup can be
> converted into a replica, but it's sufficient to guarantee it can
> brought out of recovery.
>

Perhaps we are misunderstanding the word "it" here. "it can be brought out
of recovery"?

You appear to be saying that a backup that disconnects before completion is
useful in some way. How so?

If the slot is cleaned up on disconnect, as suggested, then you end up with
half a backup and WAL is cleaned up. The only possible use for slots is to
reserve resources (as you say); the resources will clearly not be reserved
if we drop the slot on disconnect. What use is that?

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] MultiXact member wraparound protections are now enabled

[Simon Riggs]

On 28 July 2015 at 14:20, Robert Haas  wrote:

> On Sat, Jul 25, 2015 at 4:11 AM, Simon Riggs 
> wrote:
> > On 22 July 2015 at 21:45, Robert Haas  wrote:
> >> But it seemed to me that this could be rather confusing.  I thought it
> >> would be better to be explicit about whether the protections are
> >> enabled in all cases.  That way, (1) if you see the message saying
> >> they are enabled, they are enabled; (2) if you see the message saying
> >> they are disabled, they are disabled; and (3) if you see neither
> >> message, your version does not have those protections.
> >
> > (3) would imply that we can't ever remove the message, in case people
> think
> > they are unprotected.
> >
> > If we display (1) and then we find a further bug, where does that leave
> us?
> > Do we put a second "really, really fixed" message?
> >
> > AIUI this refers to a bug fix, its not like we've invented some
> anti-virus
> > mode to actively prevent or even scan for further error. I'm not sure
> why we
> > need a message to say a bug fix has been applied; that is what the
> release
> > notes are for.
> >
> > If something is disabled, we should say so, but otherwise silence means
> > safety and success.
>
> Well, I think that we can eventually downgrade or remove the message
> once (1) we've actually fixed all of the known multixact bugs and (2)
> a couple of years have gone by and most people are in the clear.  But
> right now, we've still got significant bugs unfixed.
>
> https://wiki.postgresql.org/wiki/MultiXact_Bugs
>
> Therefore, in my opinion, anything that might make it harder to debug
> problems with the MultiXact system is premature at this point.  The
> detective work that it took to figure out the chain of events that led
> to the problem fixed in 068cfadf9e2190bdd50a30d19efc7c9f0b825b5e was
> difficult; I wanted to make sure that future debugging would be
> easier, not harder.  I still think that's the right decision, but I
> recognize that not everyone agrees.


I do now, thanks for explaining.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Don'st start streaming after creating a slot in pg_receivexlog

[Michael Paquier]

On Wed, Jul 29, 2015 at 5:02 PM, Heikki Linnakangas wrote:
> Hmm. pg_receivelogical is basically a debugging tool. I don't think anyone
> will have it integrated into production scripts etc. So maybe we could just
> change it.

This sounds good to me as well.

> I'm not sure I understand the proposal though.

In short, I would propose the following:
- Have --create-slot only create a slot, then exit for both
pg_recvlogical and pg_receivexlog.
- Have --drop-slot drop a slot, then exit.
- Remove the --start switch in pg_recvlogical, and let the utility
start streaming by default.
By doing so both utilities will behave similarly with the same set of options.

> If you don't specify
> --create-slot, nor --start, what would the program do? Nothing?

Complain if neither --create-slot nor --drop-slot nor --start are specified:
$ pg_recvlogical -f - --slot toto -d postgres
pg_recvlogical: at least one action needs to be specified
Try "pg_recvlogical --help" for more information.
$ echo $?
1
-- 
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] LWLock deadlock and gdb advice

[Heikki Linnakangas]

On 07/29/2015 02:39 PM, Andres Freund wrote:

On 2015-07-15 18:44:03 +0300, Heikki Linnakangas wrote:

Previously, LWLockAcquireWithVar set the variable associated with the lock
atomically with acquiring it. Before the lwlock-scalability changes, that
was straightforward because you held the spinlock anyway, but it's a lot
harder/expensive now. So I changed the way acquiring a lock with a variable
works. There is now a separate flag, LW_FLAG_VAR_SET, which indicates that
the current lock holder has updated the variable. The LWLockAcquireWithVar
function is gone - you now just use LWLockAcquire(), which always clears the
LW_FLAG_VAR_SET flag, and you can call LWLockUpdateVar() after that if you
want to set the variable immediately.

This passes make check, but I haven't done any testing beyond that. Does
this look sane to you?


The prime thing I dislike about the patch is how long it now holds the
spinlock in WaitForVar. I don't understand why that's necessary? There's
no need to hold a spinlock until after the
mustwait = (pg_atomic_read_u32(&lock->state) & LW_VAL_EXCLUSIVE) != 0;
unless I miss something?

In an earlier email you say:

After the spinlock is released above, but before the LWLockQueueSelf() call,
it's possible that another backend comes in, acquires the lock, changes the
variable's value, and releases the lock again. In 9.4, the spinlock was not
released until the process was queued.


But that's not a problem. The updater in that case will have queued a
wakeup for all waiters, including WaitForVar()?


If you release the spinlock before LWLockQueueSelf(), then no. It's 
possible that the backend you wanted to wait for updates the variable's 
value before you've queued up. Or even releases the lock, and it gets 
re-acquired by another backend, before you've queued up.


Or are you thinking of just moving the SpinLockRelease to after 
LWLockQueueSelf(), i.e. to the other side of the "mustwait = ..." line? 
That'd probably be ok.


- Heikki



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

Re: [HACKERS] pg_basebackup and replication slots

[Andres Freund]

On 2015-07-29 12:47:01 +0100, Simon Riggs wrote:
> On 29 July 2015 at 11:43, Andres Freund  wrote:
> 
> > On 2015-07-29 09:17:04 +0100, Simon Riggs wrote:
> > > On 29 July 2015 at 09:09, Andres Freund  wrote:
> > > > The point of using a temporary slot is to not have a
> > > > leftover slot afterwards, reserving resources. Especially important if
> > > > the basebackup actually failed...
> > > >
> > >
> > > Creating a slot and then deleting it if the session disconnects does not
> > > successfully provide the functionality desired above.
> >
> > Uh? If you create the slot, start streaming, and then start the
> > basebackup, it does. It does *not* guarantee that the base backup can be
> > converted into a replica, but it's sufficient to guarantee it can
> > brought out of recovery.
> >
> 
> Perhaps we are misunderstanding the word "it" here. "it can be brought out
> of recovery"?

The finished base backup.

> You appear to be saying that a backup that disconnects before completion is
> useful in some way. How so?

I'm not trying to say that at all.

As far as I understand this subthread the goal is to have a
pg_basebackup that internally creates a slot, so it can guarantee that
all the required WAL is present till streamed out by -X
stream/fetch. The problem with just creating a slot is that it'd "leak"
if pg_basebackup is killed, or the connection breaks.  The idea with the
ephemeral slot is that it'd automatically kept alive until the base
backup is complete, but would also automatically be dropped if the base
backup fails.

Makes sense?

We pretty much have all the required infrastructure for that in slot.c,
it's just not exposed to the replication protocol.

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] Support for N synchronous standby servers - take 2

[Sawada Masahiko]

On Tue, Jul 21, 2015 at 3:50 PM, Michael Paquier
 wrote:
> On Mon, Jul 20, 2015 at 9:59 PM, Beena Emerson  
> wrote:
>> Simon Riggs wrote:
>>
>>> The choice between formats is not
>>> solely predicated on whether we have multi-line support.
>>
>>> I still think writing down some actual use cases would help bring the
>>> discussion to a conclusion. Inventing a general facility is hard without
>>> some clear goals about what we need to support.
>>
>> We need to at least support the following:
>> - Grouping: Specify of standbys along with the minimum number of commits
>> required from the group.
>> - Group Type: Groups can either be priority or quorum group.
>
> As far as I understood at the lowest level a group is just an alias
> for a list of nodes, quorum or priority are properties that can be
> applied to a group of nodes when this group is used in the expression
> to define what means synchronous commit.
>
>> - Group names: to simplify status reporting
>> - Nesting: At least 2 levels of nesting
>
> If I am following correctly, at the first level there is the
> definition of the top level objects, like groups and sync expression.
>

The grouping and using same application_name different server is similar.
How does the same application_name different server work?

>> Using JSON, sync rep parameter to replicate in 2 different clusters could be
>> written as:
>>
>>{"remotes":
>> {"quorum": 2,
>>  "servers": [{"london":
>> {"priority": 2,
>>  "servers": ["lndn1", "lndn2", "lndn3"]
>> }}
>> ,
>>   {"nyc":
>> {"priority": 1,
>>  "servers": ["ny1", "ny2"]
>> }}
>>   ]
>> }
>> }
>> The same parameter in the new language (as suggested above) could be written
>> as:
>>  'remotes: 2(london: 1[lndn1, lndn2, lndn3], nyc: 1[ny1, ny2])'
>
> OK, there is a typo. That's actually 2(london: 2[lndn1, lndn2, lndn3],
> nyc: 1[ny1, ny2]) in your grammar. Honestly, if we want group aliases,
> I think that JSON makes the most sense. One of the advantage of a
> group is that you can use it in several places in the blob and set
> different properties into it, hence we should be able to define a
> group out of the sync expression.
> Hence I would think that something like that makes more sense:
> {
> "sync_standby_names":
> {
> "quorum":2,
> "nodes":
> [
> {"priority":1,"group":"cluster1"},
> {"quorum":2,"nodes":["node1","node2","node3"]}
> ]
> },
> "groups":
> {
> "cluster1":["node11","node12","node13"],
> "cluster2":["node21","node22","node23"]
> }
> }
>
>> Also, I was thinking the name of the main group could be optional.
>> Internally, it can be given the name 'default group' or 'main group' for
>> status reporting.
>>
>> The above could also be written as:
>>  '2(london: 2[lndn1, lndn2, lndn3], nyc: 1[ny1, ny2])'
>>
>> backward compatible:
>> In JSON, while validating we may have to check if it starts with '{' to go
>
> Something worth noticing, application_name can begin with "{".
>
>> for JSON parsing else proceed with the current method.
>
>> A,B,C => 1[A,B,C]. This can be added in the new parser code.
>
> This makes sense. We could do the same for JSON-based format as well
> by reusing the in-memory structure used to deparse the blob when the
> former grammar is used as well.

If I validate s_s_name JSON syntax, I will definitely use JSONB,
rather than JSON.
Because JSONB has some useful operation functions for adding node,
deleting node to s_s_name today.
But the down side of using JSONB for s_s_name is that it could switch
in key name order place.(and remove duplicate key)
For example in the syntax Michael suggested,


* JSON (just casting JSON)
  json

 { +
 "sync_standby_names": +
 { +
 "quorum":2,   +
 "nodes":  +
 [ +
 {"priority":1,"group":"cluster1"},+
 {"quorum":2,"nodes":["node1","node2","node3"]}+
 ] +
 },+
 "groups": +
 { +
 "cluster1":["node11","node12","node13"],  +
  

Re: [HACKERS] LWLock deadlock and gdb advice

[Andres Freund]

On 2015-07-29 14:55:54 +0300, Heikki Linnakangas wrote:
> On 07/29/2015 02:39 PM, Andres Freund wrote:
> >In an earlier email you say:
> >>After the spinlock is released above, but before the LWLockQueueSelf() call,
> >>it's possible that another backend comes in, acquires the lock, changes the
> >>variable's value, and releases the lock again. In 9.4, the spinlock was not
> >>released until the process was queued.
> >
> >But that's not a problem. The updater in that case will have queued a
> >wakeup for all waiters, including WaitForVar()?
> 
> If you release the spinlock before LWLockQueueSelf(), then no. It's possible
> that the backend you wanted to wait for updates the variable's value before
> you've queued up. Or even releases the lock, and it gets re-acquired by
> another backend, before you've queued up.

For normal locks the equivalent problem is solved by re-checking wether
a conflicting lock is still held after enqueuing. Why don't we do the
same here? Doing it that way has the big advantage that we can just
remove the spinlocks entirely on platforms with atomic 64bit
reads/writes.

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] LWLock deadlock and gdb advice

[Heikki Linnakangas]

On 07/29/2015 03:08 PM, Andres Freund wrote:

On 2015-07-29 14:55:54 +0300, Heikki Linnakangas wrote:

On 07/29/2015 02:39 PM, Andres Freund wrote:

In an earlier email you say:

After the spinlock is released above, but before the LWLockQueueSelf() call,
it's possible that another backend comes in, acquires the lock, changes the
variable's value, and releases the lock again. In 9.4, the spinlock was not
released until the process was queued.


But that's not a problem. The updater in that case will have queued a
wakeup for all waiters, including WaitForVar()?


If you release the spinlock before LWLockQueueSelf(), then no. It's possible
that the backend you wanted to wait for updates the variable's value before
you've queued up. Or even releases the lock, and it gets re-acquired by
another backend, before you've queued up.


For normal locks the equivalent problem is solved by re-checking wether
a conflicting lock is still held after enqueuing. Why don't we do the
same here? Doing it that way has the big advantage that we can just
remove the spinlocks entirely on platforms with atomic 64bit
reads/writes.


Ah, ok, that should work, as long as you also re-check the variable's 
value after queueing. Want to write the patch, or should I?


- Heikki



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

Re: [HACKERS] LWLock deadlock and gdb advice

[Andres Freund]

On 2015-07-29 15:14:23 +0300, Heikki Linnakangas wrote:
> Ah, ok, that should work, as long as you also re-check the variable's value
> after queueing. Want to write the patch, or should I?

I'll try. Shouldn't be too hard.

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] Remaining 'needs review' patchs in July commitfest

[Pavel Golub]

Hello, Heikki.

You wrote:

HL> 21 patches remain in Needs Review state, in the July commitfest. Some of
HL> them have a reviewer signed up. I have highlighted some of them below 
HL> that worry me the most. What are we going to do about these? For each of
HL> them, I'd like the authors to have some idea on what they need to do to
HL> get the patch into committable state (or if the whole approach is going
HL> to be rejected), but I don't know what that advise should be.

>> COPY RAW

HL> No consensus on whether to add this to the server's COPY command, or as
HL> a new psql backslash option.

I did quick review for this, however I need some more time for tests. May be I 
will
do it next week. There is a consensus, only detailed review needed.


HL> -- 

HL> - Heikki





-- 
With best wishes,
 Pavel  mailto:pa...@gf.microolap.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] Support for N synchronous standby servers - take 2

[Michael Paquier]

On Wed, Jul 29, 2015 at 9:03 PM, Sawada Masahiko  wrote:
> On Tue, Jul 21, 2015 at 3:50 PM, Michael Paquier
>  wrote:
>> On Mon, Jul 20, 2015 at 9:59 PM, Beena Emerson  
>> wrote:
>>> Simon Riggs wrote:
>>>
 The choice between formats is not
 solely predicated on whether we have multi-line support.
>>>
 I still think writing down some actual use cases would help bring the
 discussion to a conclusion. Inventing a general facility is hard without
 some clear goals about what we need to support.
>>>
>>> We need to at least support the following:
>>> - Grouping: Specify of standbys along with the minimum number of commits
>>> required from the group.
>>> - Group Type: Groups can either be priority or quorum group.
>>
>> As far as I understood at the lowest level a group is just an alias
>> for a list of nodes, quorum or priority are properties that can be
>> applied to a group of nodes when this group is used in the expression
>> to define what means synchronous commit.
>>
>>> - Group names: to simplify status reporting
>>> - Nesting: At least 2 levels of nesting
>>
>> If I am following correctly, at the first level there is the
>> definition of the top level objects, like groups and sync expression.
>>
>
> The grouping and using same application_name different server is similar.
> How does the same application_name different server work?

In the same of a priority group both nodes get the same priority,
imagine for example that we need to wait for 2 nodes with lower
priority: node1 with priority 1, node2 with priority 2 and again node2
with priority 2, we would wait for the first one, and then one of the
second. In quorum group, any of them could be qualified for selection.

>>> Using JSON, sync rep parameter to replicate in 2 different clusters could be
>>> written as:
>>>
>>>{"remotes":
>>> {"quorum": 2,
>>>  "servers": [{"london":
>>> {"priority": 2,
>>>  "servers": ["lndn1", "lndn2", "lndn3"]
>>> }}
>>> ,
>>>   {"nyc":
>>> {"priority": 1,
>>>  "servers": ["ny1", "ny2"]
>>> }}
>>>   ]
>>> }
>>> }
>>> The same parameter in the new language (as suggested above) could be written
>>> as:
>>>  'remotes: 2(london: 1[lndn1, lndn2, lndn3], nyc: 1[ny1, ny2])'
>>
>> OK, there is a typo. That's actually 2(london: 2[lndn1, lndn2, lndn3],
>> nyc: 1[ny1, ny2]) in your grammar. Honestly, if we want group aliases,
>> I think that JSON makes the most sense. One of the advantage of a
>> group is that you can use it in several places in the blob and set
>> different properties into it, hence we should be able to define a
>> group out of the sync expression.
>> Hence I would think that something like that makes more sense:
>> {
>> "sync_standby_names":
>> {
>> "quorum":2,
>> "nodes":
>> [
>> {"priority":1,"group":"cluster1"},
>> {"quorum":2,"nodes":["node1","node2","node3"]}
>> ]
>> },
>> "groups":
>> {
>> "cluster1":["node11","node12","node13"],
>> "cluster2":["node21","node22","node23"]
>> }
>> }
>>
>>> Also, I was thinking the name of the main group could be optional.
>>> Internally, it can be given the name 'default group' or 'main group' for
>>> status reporting.
>>>
>>> The above could also be written as:
>>>  '2(london: 2[lndn1, lndn2, lndn3], nyc: 1[ny1, ny2])'
>>>
>>> backward compatible:
>>> In JSON, while validating we may have to check if it starts with '{' to go
>>
>> Something worth noticing, application_name can begin with "{".
>>
>>> for JSON parsing else proceed with the current method.
>>
>>> A,B,C => 1[A,B,C]. This can be added in the new parser code.
>>
>> This makes sense. We could do the same for JSON-based format as well
>> by reusing the in-memory structure used to deparse the blob when the
>> former grammar is used as well.
>
> If I validate s_s_name JSON syntax, I will definitely use JSONB,
> rather than JSON.
> Because JSONB has some useful operation functions for adding node,
> deleting node to s_s_name today.
> But the down side of using JSONB for s_s_name is that it could switch
> in key name order place.(and remove duplicate key)
> For example in the syntax Michael suggested,
> [...]
> "group" and "sync_standby_names" has been switched place. I'm not sure
> it's good for the users.

I think that's perfectly fine.
-- 
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] Freeze avoidance of very large table.

[Masahiko Sawada]

On Thu, Jul 16, 2015 at 8:51 PM, Sawada Masahiko  wrote:
> On Wed, Jul 15, 2015 at 3:07 AM, Sawada Masahiko  
> wrote:
>> On Wed, Jul 15, 2015 at 12:55 AM, Simon Riggs  wrote:
>>> On 10 July 2015 at 15:11, Sawada Masahiko  wrote:


 Oops, I had forgotten to add new file heapfuncs.c.
 Latest patch is attached.
>>>
>>>
>>> I think we've established the approach is desirable and defined the way
>>> forwards for this, so this is looking good.
>>
>> If we want to move stuff like pg_stattuple, pg_freespacemap into core,
>> we could move them into heapfuncs.c.
>>
>>> Some of my requests haven't been actioned yet, so I personally would not
>>> commit this yet. I am happy to continue as reviewer/committer unless others
>>> wish to take over.
>>> The main missing item is pg_upgrade support, which won't happen by end of
>>> CF1, so I am marking this as Returned With Feedback. Hopefully we can review
>>> this again before CF2.
>>
>> I appreciate your reviewing.
>> Yeah, the pg_upgrade support and regression test for VFM patch is
>> almost done now, I will submit the patch in this week after testing it
>> .
>
> Attached patch is latest v9 patch.
>
> I added:
> - regression test for visibility map (visibilitymap.sql and
> visibilitymap.out files)
> - pg_upgrade support (rewriting vm file to vfm file)
> - regression test for pg_upgrade
>

Previous patch has some fail to apply, so attached the rebased patch.
Catalog version is not decided yet, so we will need to rewrite
VISIBILITY_MAP_FROZEN_BIT_CAT_VER in pg_upgrade.h
Please review it.

Regards,

--
Masahiko Sawada
diff --git a/contrib/pgstattuple/pgstatapprox.c b/contrib/pgstattuple/pgstatapprox.c
index 22c5f7a..b1b6a06 100644
--- a/contrib/pgstattuple/pgstatapprox.c
+++ b/contrib/pgstattuple/pgstatapprox.c
@@ -87,7 +87,7 @@ statapprox_heap(Relation rel, output_type *stat)
 		 * If the page has only visible tuples, then we can find out the free
 		 * space from the FSM and move on.
 		 */
-		if (visibilitymap_test(rel, blkno, &vmbuffer))
+		if (visibilitymap_test(rel, blkno, &vmbuffer, VISIBILITYMAP_ALL_VISIBLE))
 		{
 			freespace = GetRecordedFreeSpace(rel, blkno);
 			stat->tuple_len += BLCKSZ - freespace;
diff --git a/src/backend/access/heap/Makefile b/src/backend/access/heap/Makefile
index b83d496..806ce27 100644
--- a/src/backend/access/heap/Makefile
+++ b/src/backend/access/heap/Makefile
@@ -12,6 +12,7 @@ subdir = src/backend/access/heap
 top_builddir = ../../../..
 include $(top_builddir)/src/Makefile.global
 
-OBJS = heapam.o hio.o pruneheap.o rewriteheap.o syncscan.o tuptoaster.o visibilitymap.o
+OBJS = heapam.o hio.o pruneheap.o rewriteheap.o syncscan.o tuptoaster.o visibilitymap.o \
+	heapfuncs.o
 
 include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 050efdc..2dbabc8 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -2176,8 +2176,9 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
 	CheckForSerializableConflictIn(relation, NULL, InvalidBuffer);
 
 	/*
-	 * Find buffer to insert this tuple into.  If the page is all visible,
-	 * this will also pin the requisite visibility map page.
+	 * Find buffer to insert this tuple into.  If the page is all visible
+	 * or all frozen, this will also pin the requisite visibility map and
+	 * frozen map page.
 	 */
 	buffer = RelationGetBufferForTuple(relation, heaptup->t_len,
 	   InvalidBuffer, options, bistate,
@@ -2192,7 +2193,11 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
 	if (PageIsAllVisible(BufferGetPage(buffer)))
 	{
 		all_visible_cleared = true;
+
+		/* all-frozen information is also cleared at the same time */
 		PageClearAllVisible(BufferGetPage(buffer));
+		PageClearAllFrozen(BufferGetPage(buffer));
+
 		visibilitymap_clear(relation,
 			ItemPointerGetBlockNumber(&(heaptup->t_self)),
 			vmbuffer);
@@ -2493,7 +2498,11 @@ heap_multi_insert(Relation relation, HeapTuple *tuples, int ntuples,
 		if (PageIsAllVisible(page))
 		{
 			all_visible_cleared = true;
+
+			/* all-frozen information is also cleared at the same time */
 			PageClearAllVisible(page);
+			PageClearAllFrozen(page);
+
 			visibilitymap_clear(relation,
 BufferGetBlockNumber(buffer),
 vmbuffer);
@@ -2776,9 +2785,9 @@ heap_delete(Relation relation, ItemPointer tid,
 
 	/*
 	 * If we didn't pin the visibility map page and the page has become all
-	 * visible while we were busy locking the buffer, we'll have to unlock and
-	 * re-lock, to avoid holding the buffer lock across an I/O.  That's a bit
-	 * unfortunate, but hopefully shouldn't happen often.
+	 * visible or all frozen while we were busy locking the buffer, we'll
+	 * have to unlock and re-lock, to avoid holding the buffer lock across an
+	 * I/O.  That's a bit unfortunate, but hopefully shouldn't happen often.
 	 */
 	if (vmbuffer == InvalidBuffer && PageIsAllVisible(page))
 	{
@@ -2970,10 +29

Re: [HACKERS] pg_basebackup and replication slots

[Simon Riggs]

On 29 July 2015 at 13:00, Andres Freund  wrote:


> As far as I understand this subthread the goal is to have a
> pg_basebackup that internally creates a slot, so it can guarantee that
> all the required WAL is present till streamed out by -X
> stream/fetch. The problem with just creating a slot is that it'd "leak"
> if pg_basebackup is killed, or the connection breaks.  The idea with the
> ephemeral slot is that it'd automatically kept alive until the base
> backup is complete, but would also automatically be dropped if the base
> backup fails.
>
> Makes sense?
>

So this would be needed when creating a standalone backup that would not be
persistently connected to the master, yet we want to bring it up as a
live/writable server in a single command, and we want to make it easy to
script in case our script is killed?

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Don'st start streaming after creating a slot in pg_receivexlog

[Simon Riggs]

On 29 July 2015 at 12:51, Michael Paquier  wrote:


> In short, I would propose the following:
> - Have --create-slot only create a slot, then exit for both
> pg_recvlogical and pg_receivexlog.
> - Have --drop-slot drop a slot, then exit.
>

It makes more sense to create one new utility to issue replication commands
than to enhance multiple utility commands to have bizarre looking
additional features and modes.

pg_reputil --create-slot
pg_reputil --drop-slot
etc

though I prefer the idea of using psql and the already existing slot
functions than doing all of this extra code that needs maintaining.

- Remove the --start switch in pg_recvlogical, and let the utility
> start streaming by default.
> By doing so both utilities will behave similarly with the same set of
> options.
>

+1

That way all the utilities have just one thing they do.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] pg_basebackup and replication slots

[Andres Freund]

On 2015-07-29 13:45:22 +0100, Simon Riggs wrote:
> So this would be needed when creating a standalone backup that would not be
> persistently connected to the master, yet we want to bring it up as a
> live/writable server in a single command

I'm not understanding what you mean with 'single command'
here. Currently there's no way to do this safely without configuring a
high enough wal_keep_segments.

You can (since yesterday) manually create a slot, run pg_basebackup, and
drop the slot. But that's not safe if your script is interrupted
somehow. Since many base backups are run in a non-interactive fashion
asking for intervention to clean up in that case imo is not an
acceptable answer.

> and we want to make it easy to script in case our script is killed?

Or the connection dies/is killed, or the server is restarted, or ...


-- 
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] A little RLS oversight?

[Stephen Frost]

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Mon, Jul 27, 2015 at 4:58 PM, Stephen Frost  wrote:
> >> I would expect that if the current user has permission to bypass RLS,
> >> and they have set row_security to OFF, then it should be off for all
> >> tables that they have access to, regardless of how they access those
> >> tables (directly or through a view). If it really is intentional that
> >> RLS remains active when querying through a view not owned by the
> >> table's owner, then the other calls to check_enable_rls() should
> >> probably be left as they were, since the table might have been updated
> >> through such a view and that code can't really tell at that point.
> >
> > Joe and I were discussing this earlier and it was certainly intentional
> > that RLS still be enabled if you're querying through a view as the RLS
> > rights of the view owner are used, not your own.  Note that we don't
> > allow a user to assume the BYPASSRLS right of the view owner though,
> > also intentionally.
> 
> That seems inconsistent.  If querying through a view doesn't adopt the
> BYPASSRLS right (or lack thereof) of the view owner, and I agree that
> it shouldn't, then it also shouldn't disregard the fact that the
> person issuing the query has that right.

That's not how other role attributes currently work though, specifically
thinking of superuser.  Even when running a query as a superuser you can
get a permission denied if you're querying a view where the view owner
hasn't got access to the relation under the view.

=# create role r1;
CREATE ROLE
=*# create role r2;
CREATE ROLE
=*# create table t1 (c1 int);
CREATE TABLE
=*# alter table t1 owner to r1;
ALTER TABLE
=*# create view v1 as select * from t1;
CREATE VIEW
=*# alter view v1 owner to r2;
ALTER VIEW
=*# select * from v1;
ERROR:  permission denied for relation t1

> In other words, we've made a decision, when going through views, to
> test ACLs based on who owns the view.  We do that in all cases, not
> only sometimes.  Now, when querying a view, whose BYPASSRLS privilege
> do we consult?  It should either be the person issuing the query in
> all cases, or the view owner in all cases, not some hybrid of the two.

For superuser (the only similar precedent that we have, I believe), we
go based on the view owner, but that isn't quite the same as BYPASSRLS.

The reason this doesn't hold is that you have to use a combination of
BYPASSRLS and row_security=off to actually bypass RLS, unlike the
superuser role attribute which is just always "on" if you've got it.  If
having BYPASSRLS simply always meant "don't do any RLS" then we could
use the superuser precedent to use what the view owner has, but at least
for my part, I'm a lot happier with BYPASSRLS and row_security than with
superuser and would rather we continue in that direction, where the user
has the choice of if they want their role attribute to be in effect or
not.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] LWLock deadlock and gdb advice

[Andres Freund]

On 2015-07-29 14:22:23 +0200, Andres Freund wrote:
> On 2015-07-29 15:14:23 +0300, Heikki Linnakangas wrote:
> > Ah, ok, that should work, as long as you also re-check the variable's value
> > after queueing. Want to write the patch, or should I?
> 
> I'll try. Shouldn't be too hard.

What do you think about something roughly like the attached?

- Andres
>From 2879408d8da7714ab6af6238dfe1c8a535800af3 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Wed, 29 Jul 2015 15:06:54 +0200
Subject: [PATCH] Other way to fix the issues around the broken LWLock variable
 support.

---
 src/backend/storage/lmgr/lwlock.c | 105 +-
 1 file changed, 69 insertions(+), 36 deletions(-)

diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index e5566d1..c64f20d 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -1066,7 +1066,15 @@ LWLockAcquireCommon(LWLock *lock, LWLockMode mode, uint64 *valptr, uint64 val)
 
 	/* If there's a variable associated with this lock, initialize it */
 	if (valptr)
+	{
+#ifdef LWLOCK_STATS
+		lwstats->spin_delay_count += SpinLockAcquire(&lock->mutex);
+#else
+		SpinLockAcquire(&lock->mutex);
+#endif
 		*valptr = val;
+		SpinLockRelease(&lock->mutex);
+	}
 
 	TRACE_POSTGRESQL_LWLOCK_ACQUIRE(T_NAME(lock), T_ID(lock), mode);
 
@@ -1259,6 +1267,60 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode)
 }
 
 /*
+ * Does the lwlock in its current state need to wait for the variable value to
+ * change?
+ *
+ * If we don't need to wait, and it's because the value of the variable has
+ * changed, store the current value in newval.
+ *
+ * *result is set to true if the lock was free, and false otherwise.
+ */
+static bool
+LWLockConflictsWithVar(LWLock *lock,
+	   uint64 *valptr, uint64 oldval, uint64 *newval,
+	   bool *result)
+{
+	bool		mustwait;
+	uint64		value;
+
+	mustwait = (pg_atomic_read_u32(&lock->state) & LW_VAL_EXCLUSIVE) != 0;
+
+	if (!mustwait)
+	{
+		*result = true;
+		return false;
+	}
+
+	/*
+	 * Perform comparison using spinlock as we can't rely on atomic 64 bit
+	 * reads/stores.  On platforms with a way to do atomic 64 bit reads/writes
+	 * the spinlock can be optimized away.
+	 */
+#ifdef LWLOCK_STATS
+	lwstats->spin_delay_count += SpinLockAcquire(&lock->mutex);
+#else
+	SpinLockAcquire(&lock->mutex);
+#endif
+
+	*result = false;
+
+	value = *valptr;
+	if (value != oldval)
+	{
+		mustwait = false;
+		*newval = value;
+	}
+	else
+	{
+		mustwait = true;
+	}
+
+	SpinLockRelease(&lock->mutex);
+
+	return mustwait;
+}
+
+/*
  * LWLockWaitForVar - Wait until lock is free, or a variable is updated.
  *
  * If the lock is held and *valptr equals oldval, waits until the lock is
@@ -1313,39 +1375,9 @@ LWLockWaitForVar(LWLock *lock, uint64 *valptr, uint64 oldval, uint64 *newval)
 	for (;;)
 	{
 		bool		mustwait;
-		uint64		value;
-
-		mustwait = (pg_atomic_read_u32(&lock->state) & LW_VAL_EXCLUSIVE) != 0;
-
-		if (mustwait)
-		{
-			/*
-			 * Perform comparison using spinlock as we can't rely on atomic 64
-			 * bit reads/stores.
-			 */
-#ifdef LWLOCK_STATS
-			lwstats->spin_delay_count += SpinLockAcquire(&lock->mutex);
-#else
-			SpinLockAcquire(&lock->mutex);
-#endif
 
-			/*
-			 * XXX: We can significantly optimize this on platforms with 64bit
-			 * atomics.
-			 */
-			value = *valptr;
-			if (value != oldval)
-			{
-result = false;
-mustwait = false;
-*newval = value;
-			}
-			else
-mustwait = true;
-			SpinLockRelease(&lock->mutex);
-		}
-		else
-			mustwait = false;
+		mustwait = LWLockConflictsWithVar(lock, valptr, oldval, newval,
+		&result);
 
 		if (!mustwait)
 			break;/* the lock was free or value didn't match */
@@ -1365,12 +1397,13 @@ LWLockWaitForVar(LWLock *lock, uint64 *valptr, uint64 oldval, uint64 *newval)
 		pg_atomic_fetch_or_u32(&lock->state, LW_FLAG_RELEASE_OK);
 
 		/*
-		 * We're now guaranteed to be woken up if necessary. Recheck the
-		 * lock's state.
+		 * We're now guaranteed to be woken up if necessary. Recheck the lock
+		 * and variables state.
 		 */
-		mustwait = (pg_atomic_read_u32(&lock->state) & LW_VAL_EXCLUSIVE) != 0;
+		mustwait = LWLockConflictsWithVar(lock, valptr, oldval, newval,
+		&result);
 
-		/* Ok, lock is free after we queued ourselves. Undo queueing. */
+		/* Ok, no conflict after we queued ourselves. Undo queueing. */
 		if (!mustwait)
 		{
 			LOG_LWDEBUG("LWLockWaitForVar", lock, "free, undoing queue");
-- 
2.4.0.rc2.1.g3d6bc9a


-- 
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] Don'st start streaming after creating a slot in pg_receivexlog

[Andres Freund]

On 2015-07-29 13:53:31 +0100, Simon Riggs wrote:
> It makes more sense to create one new utility to issue replication commands
> than to enhance multiple utility commands to have bizarre looking
> additional features and modes.
> 
> pg_reputil --create-slot
> pg_reputil --drop-slot
> etc

Logical slots behave differently than physical slots and need different
options. So to me there's a fair point in having support for
manipulating logical slot in a tool around logical slots.

Additionally that support was there in 9.4 and I know at least of three
scripts that call pg_recvlogical to create logical slots. Admittedly one
of them is of my own creation.

> though I prefer the idea of using psql and the already existing slot
> functions than doing all of this extra code that needs maintaining.

It's not that uncommon to have replicas only access the primary for
replication type connections. So it seems completely sensible to use the
replication protocol to manage slots. And that you can't really do with
psql.

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] Don'st start streaming after creating a slot in pg_receivexlog

[Michael Paquier]

On Wed, Jul 29, 2015 at 8:51 PM, Michael Paquier
 wrote:
> On Wed, Jul 29, 2015 at 5:02 PM, Heikki Linnakangas wrote:
>> Hmm. pg_receivelogical is basically a debugging tool. I don't think anyone
>> will have it integrated into production scripts etc. So maybe we could just
>> change it.
>
> This sounds good to me as well.
>
>> I'm not sure I understand the proposal though.
>
> In short, I would propose the following:
> - Have --create-slot only create a slot, then exit for both
> pg_recvlogical and pg_receivexlog.
> - Have --drop-slot drop a slot, then exit.
> - Remove the --start switch in pg_recvlogical, and let the utility
> start streaming by default.
> By doing so both utilities will behave similarly with the same set of options.
>
>> If you don't specify
>> --create-slot, nor --start, what would the program do? Nothing?
>
> Complain if neither --create-slot nor --drop-slot nor --start are specified:
> $ pg_recvlogical -f - --slot toto -d postgres
> pg_recvlogical: at least one action needs to be specified
> Try "pg_recvlogical --help" for more information.
> $ echo $?
> 1

Here is a patch implementing those things. IMO if-not-exists does not
make much sense anymore. Documentation has been shuffled a bit as
well.
-- 
Michael


20150729_pgrecv_slots.patch
Description: binary/octet-stream

-- 
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] Don'st start streaming after creating a slot in pg_receivexlog

[Andres Freund]

On 2015-07-29 22:17:27 +0900, Michael Paquier wrote:
> Here is a patch implementing those things. IMO if-not-exists does not
> make much sense anymore

What? It's rather useful to be able to discern between 'slot was already
there' and 'oops, some error occured'. -1

To me the pg_recvlogical changes are pretty pointless.

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] Don'st start streaming after creating a slot in pg_receivexlog

[Michael Paquier]

On Wed, Jul 29, 2015 at 10:15 PM, Andres Freund wrote:
> It's not that uncommon to have replicas only access the primary for
> replication type connections. So it seems completely sensible to use the
> replication protocol to manage slots. And that you can't really do with
> psql.

Actually, you can. CREATE_REPLICATION_SLOT is one of the commands that
work with psql, but that's not really practical compared to what
pg_recvlogical and pg_receivexlog can do.
$ psql -d "replication=1"
=# CREATE_REPLICATION_SLOT hoge PHYSICAL;
 slot_name | consistent_point | snapshot_name | output_plugin
---+--+---+---
 hoge  | 0/0  | null  | null
(1 row)
-- 
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] Don'st start streaming after creating a slot in pg_receivexlog

[Andres Freund]

On 2015-07-29 22:21:04 +0900, Michael Paquier wrote:
> On Wed, Jul 29, 2015 at 10:15 PM, Andres Freund wrote:
> > It's not that uncommon to have replicas only access the primary for
> > replication type connections. So it seems completely sensible to use the
> > replication protocol to manage slots. And that you can't really do with
> > psql.
>
> Actually, you can. CREATE_REPLICATION_SLOT is one of the commands that
> work with psql

I do know that, I wrote the bit in the docs explaining that that's
possible...

But using replication connections that way in psql makes requires it to
be invoked in a different way and makes it it impossible to specify the
database name in a normal way etc. You can also run into commands that
give you 'unexpected result status' errors and such. I don't think
that's a good default interface.


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

[Kouhei Kaigai]

Hi Amit,

Could you tell me the code intention around ExecInitFunnel()?

ExecInitFunnel() calls InitFunnel() that opens the relation to be
scanned by the underlying PartialSeqScan and setup ss_ScanTupleSlot
of its scanstate.
According to the comment of InitFunnel(), it open the relation and
takes appropriate lock on it. However, an equivalent initialization
is also done on InitPartialScanRelation().

Why does it acquire the relation lock twice?

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] upgrade failure from 9.5 to head

[Andrew Dunstan]

My cross-version upgrade testing tool just threw up this failure, 
upgrading from 9.5 to head:


   CREATE ROLE "dummy_seclabel_user1";
   CREATE ROLE
   ALTER ROLE "dummy_seclabel_user1" WITH NOSUPERUSER INHERIT
   CREATEROLE NOCREATEDB LOGIN NOREPLICATION NOBYPASSRLS;
   ALTER ROLE
   SECURITY LABEL FOR "dummy" ON ROLE "dummy_seclabel_user1" IS
   'classified';
   psql:pg_upgrade_dump_globals.sql:25: ERROR:  security label provider
   "dummy" is not loaded


This error might not be terribly new - it's been masked by another 
earlier problem that I have now catered for in the tool.


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] upgrade failure from 9.5 to head

[Tom Lane]

Andrew Dunstan  writes:
> My cross-version upgrade testing tool just threw up this failure, 
> upgrading from 9.5 to head:

> CREATE ROLE "dummy_seclabel_user1";
> CREATE ROLE
> ALTER ROLE "dummy_seclabel_user1" WITH NOSUPERUSER INHERIT
> CREATEROLE NOCREATEDB LOGIN NOREPLICATION NOBYPASSRLS;
> ALTER ROLE
> SECURITY LABEL FOR "dummy" ON ROLE "dummy_seclabel_user1" IS
> 'classified';
> psql:pg_upgrade_dump_globals.sql:25: ERROR:  security label provider
> "dummy" is not loaded

Apparently you're trying to pg_upgrade an installation that contains
leftover databases from src/test/modules/ testing?  Not sure we have
any intention of supporting that.  Even if it made sense in some cases,
the README for dummy_seclabel is pretty definitive that that one is
not meant for production.

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] upgrade failure from 9.5 to head

[Andres Freund]

Hi,

On 2015-07-29 10:16:10 -0400, Andrew Dunstan wrote:
> 
> My cross-version upgrade testing tool just threw up this failure, upgrading
> from 9.5 to head:
> 
>CREATE ROLE "dummy_seclabel_user1";
>CREATE ROLE
>ALTER ROLE "dummy_seclabel_user1" WITH NOSUPERUSER INHERIT
>CREATEROLE NOCREATEDB LOGIN NOREPLICATION NOBYPASSRLS;
>ALTER ROLE
>SECURITY LABEL FOR "dummy" ON ROLE "dummy_seclabel_user1" IS
>'classified';
>psql:pg_upgrade_dump_globals.sql:25: ERROR:  security label provider
>"dummy" is not loaded

Ick! So the dummy_seclabel test more or less only works by accident if I
see that correctly. The .so is only loaded because the CREATE EXTENSION
in the test triggers a CREATE FUNCTION dummy_seclabel_dummy() ... LANG
C.

That's pretty damn ugly.


-- 
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] upgrade failure from 9.5 to head

[Tom Lane]

Andres Freund  writes:
> On 2015-07-29 10:16:10 -0400, Andrew Dunstan wrote:
>> psql:pg_upgrade_dump_globals.sql:25: ERROR:  security label provider
>> "dummy" is not loaded

> Ick! So the dummy_seclabel test more or less only works by accident if I
> see that correctly. The .so is only loaded because the CREATE EXTENSION
> in the test triggers a CREATE FUNCTION dummy_seclabel_dummy() ... LANG
> C.

Well, there's a larger issue, which is that (a) Andrew's new installation
very likely doesn't have dummy_seclabel.so built/installed at all, and
(b) even if he did, there's nothing that would cause it to get loaded
during pg_upgrade's DDL restore run.

Now as far as dummy_seclabel is concerned, the easy answer is "we don't
care".  But on reflection, doesn't this mean that the entire
implementation of SECURITY LABEL is broken?  At least to the extent that
it can't work during pg_upgrade unless the user takes manual action to
configure the relevant providers' .so libraries into the new installation
*before* he runs pg_upgrade.  That doesn't say "production ready" to 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] dblink: add polymorphic functions.

[Corey Huinker]

On Wed, Jul 29, 2015 at 3:48 AM, Heikki Linnakangas  wrote:

> On 07/18/2015 01:32 AM, Corey Huinker wrote:
>
>> So this patch would result in less C code while still adding 3 new
>> functions. Can anyone think of why that wouldn't be the best way to go?
>>
>
> Let's pursue the "CAST(srf() AS row_rtype)" syntax that Joe suggested
> upthread (
> http://www.postgresql.org/message-id/559a9643.9070...@joeconway.com). For
> some reason, the discussion went on around the details of the submitted
> patch after that, even though everyone seemed to prefer the CAST() syntax.
>
> - Heikki
>
>
I'm all for adding that syntax, but it wouldn't be useful for my purposes
unless row_rtype could be a variable, and my understanding is that it can't.

Re: [HACKERS] upgrade failure from 9.5 to head

[Andres Freund]

On 2015-07-29 10:38:19 -0400, Tom Lane wrote:
> Well, there's a larger issue, which is that (a) Andrew's new installation
> very likely doesn't have dummy_seclabel.so built/installed at all

Hm. That issue doesn't particularly concern me. Having all .so's
available in the installation seems like a pretty basic
requirement. Security labels are by far not the only things that'll fail
without an extension's .so present, no?

> (b) even if he did, there's nothing that would cause it to get loaded
> during pg_upgrade's DDL restore run.

Well, generally it's assumed that all security labels are loaded via
shared_preload_libraries. I'm not super happy about that decision, but
given the desire to be able to have labels on shared objects I can see
the reasoning.

> Now as far as dummy_seclabel is concerned, the easy answer is "we don't
> care".  But on reflection, doesn't this mean that the entire
> implementation of SECURITY LABEL is broken?  At least to the extent that
> it can't work during pg_upgrade unless the user takes manual action to
> configure the relevant providers' .so libraries into the new installation
> *before* he runs pg_upgrade.  That doesn't say "production ready" to me.

Hm, I don't think that particular issue is that bad. We decided labels
are only going to work if they're in shared_preload_libararies, and they
really only do if that's the case.

I think if we think we should do something here we should add a check
that label providers are loaded in s_p_l.


-- 
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] dblink: add polymorphic functions.

[Tom Lane]

Corey Huinker  writes:
> On Wed, Jul 29, 2015 at 3:48 AM, Heikki Linnakangas  wrote:
>> Let's pursue the "CAST(srf() AS row_rtype)" syntax that Joe suggested
>> upthread (
>> http://www.postgresql.org/message-id/559a9643.9070...@joeconway.com). For
>> some reason, the discussion went on around the details of the submitted
>> patch after that, even though everyone seemed to prefer the CAST() syntax.

> I'm all for adding that syntax, but it wouldn't be useful for my purposes
> unless row_rtype could be a variable, and my understanding is that it can't.

Not sure why inserting a variable name is so much better than inserting a
type name?

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] Remaining 'needs review' patchs in July commitfest

[Fabien COELHO]

Hello Heikki,

About two patches I submitted:



pgbench - allow backslash-continuations in custom scripts


Everyone wants the feature, using multi-line SELECTs in pgbench scripts, 
but we don't seem to be reaching a consensus on how it should work. I 
think we'll need to integrate the lexer, but it would be nice to still 
support multi-statements as well, with some syntax.


I was willing to do a small job of that one, but it drifted to solving the 
meta problem of pgbench lexing for doing it "the right way".  Kyotaro 
HORIGUCHI has taken up the challenge and submitted a 3-part patch to 
modify psql lexer, then reuse it "as is" in pgbench.  I'm impressed:-)


I think that the approach is a little overkill for the simple targetted 
feature, and I'm not sure that it allows for multi-statements, but I have 
not checked.  Anyway I may try to review it, although not in the short 
term.  I'm not too optimistic because if passed, it means that the psql 
lexer would be used in 2 contexts, so changes in any of them would require 
ensuring that it does not break anything in the other one, and I'm not 
sure that it is such constraint is desirable.  Duplicating the lexer is 
not a realistic option either, I do not think that pgbench is worth it.


Anyway, in the mean time, the patch may be switched to "returned with 
feedback" or "rejected".





checkpoint continuous flushing


This does a big memory allocation at checkpoint, which Tom vehemently 
objects to. I don't much like it either, although I would be OK with a 
more moderately-sized allocation.


AFAICS Tom has not expressed any view on the patch in its current form 
(there is no message from Tom on the thread).



ISTM that Tom complained about a year ago about OOM risks on a 2007 
version of the sorting part by Takahiro ITAGAKI which was dynamically 
allocating and freeing about 24 bytes per buffers on each checkpoint.


The current v5 version uses 4 bytes per buffer at the first run and reuse 
the memory, it is allocated once and thus there is no risk of returning it 
and failing to get it back, so no significant OOM risk. Maybe the 
allocation should be moved earlier when starting the checkpointer process, 
though.


A second objection a year ago from Tom was about proof of performance 
gains. I've spent quite some time to collect a lot of data to measure 
benefits under different loads, representing X00 hours of pgbench runs, as 
can be seen in the thread. ISTM that the performance (tps & latency) 
figures, for instance:


http://www.postgresql.org/message-id/raw/alpine.DEB.2.10.1506170803210.9794@sto

are overwhelmingly in favor of the patch.


If the memory requirement of 4 bytes per buffer is still too much, it is 
eventually possible to reduce it with a guc to specify the allowed amount 
of memory and some shuffling in the code to do things by chunk.


My opinion is that 4 bytes per buffer is reasonable enough given the 
measured benefits. Also, there is more benefits if the whole checkpoint is 
sorted instead of just part of it.


I'm really willing to improve the write stall issues which freezes 
postgresql when checkpointing on HDD, so if it has to be chunked because 
this is a blocker I'll make the effort, but I do not think that it is 
useful as such.




It's not clear on what criteria this should be accepted or rejected.


Given the overall performance gains and reduction in latency, where a lot 
of write stalls are avoided or at least greatly reduced, I would be sad 
for pg to get it rejected. That does not mean that it cannot be improved.



What workloads need to be tested?


If you tell me, and provide the matching dedicated host for testing, I can 
run tests...


--
Fabien.


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

Re: [HACKERS] security labels on databases are bad for dump & restore

[Robert Haas]

On Wed, Jul 29, 2015 at 12:39 AM, Noah Misch  wrote:
> On Tue, Jul 28, 2015 at 03:36:13PM -0400, Robert Haas wrote:
>> On Tue, Jul 28, 2015 at 3:33 PM, Andres Freund  wrote:
>> > Hm?  Let me try again: If the admin does a ALTER DATABASE ... SET guc =
>> > ... *before* restoring a backup and the backup does contain a setting
>> > for the same guc, but with a different value it'll overwrite the
>> > previous explicit action by the DBA without any warning.  If the backup
>> > does *not* contain that guc the previous action survives.  That's
>> > confusing, because you're more likely to be in the 'the backup does not
>> > contain the guc' situation when testing where it thus will work.
>>
>> True.  But I don't think modifying a database before restoring into it
>> is terribly supported.  Even pg_dump --clean, which is supposed to do
>> this sort of thing, doesn't seem to work terribly reliably.  We could
>> try to fix this by having a command like ALTER DATABASE ... RESET ALL
>> that we issue before restoring the settings, but I'm afraid that will
>> take us into all sorts of unreasonable scenarios that are better just
>> labeled as "don't do that".
>
> Andres's example is a harbinger of the semantic morass ahead.  Excepting
> database objects and the "public" schema object, pg_dump and pg_dumpall mutate
> only the objects they CREATE.  They consistently restore object properties
> (owner, ACLs, security label, etc.) if and only if issuing a CREATE statement
> for the object.  For example, restoring objects contained in a schema without
> restoring the schema itself changes none of those schema properties.  pg_dump
> and pg_dumpall have mostly followed that rule for databases, too, but they
> depart from it for comment and security label.  That was a mistake.  We can't
> in general mutate an existing database to match, because we can't mutate the
> encoding, datcollate or datctype.  Even discounting that problem, I value
> consistency with the rest of the dumpable object types.

What we've proven so far (if Craig's comments are to be believed) is
that the oft-recommended formula of pg_dumpall -g plus pg_dump of each
database doesn't completely work.  That's absolutely gotta be fixed.
I'm open to whatever ideas you or others may have about how to fix
that, but it doesn't seem to me that changing the behavior of pg_dump
only when -c is specified gets us any closer to that goal.

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

[Amit Kapila]

On Wed, Jul 29, 2015 at 7:32 PM, Kouhei Kaigai  wrote:
>
> Hi Amit,
>
> Could you tell me the code intention around ExecInitFunnel()?
>
> ExecInitFunnel() calls InitFunnel() that opens the relation to be
> scanned by the underlying PartialSeqScan and setup ss_ScanTupleSlot
> of its scanstate.

The main need is for relation descriptor which is then required to set
the scan tuple's slot.  Basically it is required for tuples flowing from
worker which will use the scan tuple slot of FunnelState.


> According to the comment of InitFunnel(), it open the relation and
> takes appropriate lock on it. However, an equivalent initialization
> is also done on InitPartialScanRelation().
>
> Why does it acquire the relation lock twice?
>

I think locking twice is not required, it is just that I have used the API
ExecOpenScanRelation() which is used during other node's initialisation
due to which it lock's twice.  I think in general it should be harmless.



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

Re: [HACKERS] Reduce ProcArrayLock contention

[Amit Kapila]

On Mon, Jul 27, 2015 at 8:47 PM, Robert Haas  wrote:
>
> On Sat, Jul 25, 2015 at 12:42 AM, Amit Kapila 
wrote:
> > I thought that internal API will automatically take care of it,
> > example for msvc it uses _InterlockedCompareExchange64
> > which if doesn't work on 32-bit systems or is not defined, then
> > we have to use 32-bit version, but I am not certain about
> > that fact.
>
> Instead of using pg_atomic_uint64, how about using pg_atomic_uint32
> and storing the pgprocno rather than the pointer directly?
>

Good Suggestion!

I think this can work the way you are suggesting and I am working on
same.  Here I have one question,  do you prefer to see the code for
this optimisation be done via some LWLock interface as Pavan is
suggesting?  I am not very sure if LWLock is a good interface for this
work, but surely I can encapsulate it into different functions rather than
doing everything in ProcArrayEndTransaction.


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

Re: [HACKERS] upgrade failure from 9.5 to head

[Stephen Frost]

* Andres Freund (and...@anarazel.de) wrote:
> On 2015-07-29 10:38:19 -0400, Tom Lane wrote:
> > Well, there's a larger issue, which is that (a) Andrew's new installation
> > very likely doesn't have dummy_seclabel.so built/installed at all
> 
> Hm. That issue doesn't particularly concern me. Having all .so's
> available in the installation seems like a pretty basic
> requirement. Security labels are by far not the only things that'll fail
> without an extension's .so present, no?

It's certainly an issue that postgis users are familiar with.

> > (b) even if he did, there's nothing that would cause it to get loaded
> > during pg_upgrade's DDL restore run.
> 
> Well, generally it's assumed that all security labels are loaded via
> shared_preload_libraries. I'm not super happy about that decision, but
> given the desire to be able to have labels on shared objects I can see
> the reasoning.

Yes.

> > Now as far as dummy_seclabel is concerned, the easy answer is "we don't
> > care".  But on reflection, doesn't this mean that the entire
> > implementation of SECURITY LABEL is broken?  At least to the extent that
> > it can't work during pg_upgrade unless the user takes manual action to
> > configure the relevant providers' .so libraries into the new installation
> > *before* he runs pg_upgrade.  That doesn't say "production ready" to me.
> 
> Hm, I don't think that particular issue is that bad. We decided labels
> are only going to work if they're in shared_preload_libararies, and they
> really only do if that's the case.
> 
> I think if we think we should do something here we should add a check
> that label providers are loaded in s_p_l.

That has caused issues with the buildfarm in the past..  I'd like to
have a way to do that though, for label providers and potentially other
things which should really only be loaded via s_p_l.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] upgrade failure from 9.5 to head

[Tom Lane]

Andres Freund  writes:
> On 2015-07-29 10:38:19 -0400, Tom Lane wrote:
>> Now as far as dummy_seclabel is concerned, the easy answer is "we don't
>> care".  But on reflection, doesn't this mean that the entire
>> implementation of SECURITY LABEL is broken?  At least to the extent that
>> it can't work during pg_upgrade unless the user takes manual action to
>> configure the relevant providers' .so libraries into the new installation
>> *before* he runs pg_upgrade.  That doesn't say "production ready" to me.

> Hm, I don't think that particular issue is that bad. We decided labels
> are only going to work if they're in shared_preload_libararies, and they
> really only do if that's the case.

In that case, where in the documentation of the pg_upgrade process does
it say "you must configure the new installation with all security label
providers installed in shared_preload_libraries after initdb'ing the
new installation and before running pg_upgrade"?  And how can you meet
that requirement if you are using a canned script that does both those
steps for you?  (Red Hat certainly ships such a script in their packaging,
and I rather imagine that the Debian-style packages do too.)

And even more to the point, why exactly should security providers get this
dispensation when we don't make people jump through hoops like that for
anything else?  AFAICS, with the way things are now, if you simply load
a dump script without bothering with setting up shared_preload_libraries,
then you have all the objects loaded and no security labels attached to
them.  Isn't that a security breach by definition?

I think it's fairly broken if pg_upgrade output, or pg_dump output in
general, can't be loaded without such requirements.  Perhaps we could
have the dump script issue a LOAD for the label providers that will be
referenced; or maybe better, fix "SECURITY LABEL FOR provider" so that
it autoloads the relevant provider, which would require either a mapping
table or some convention about the .so name for a provider.

IMO, the current situation is fine for toy providers like dummy_seclabel,
but if you want the feature to ever be regarded as more than a toy,
this issue needs work.

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] upgrade failure from 9.5 to head

[Tom Lane]

Stephen Frost  writes:
> * Andres Freund (and...@anarazel.de) wrote:
>> Hm. That issue doesn't particularly concern me. Having all .so's
>> available in the installation seems like a pretty basic
>> requirement. Security labels are by far not the only things that'll fail
>> without an extension's .so present, no?

> It's certainly an issue that postgis users are familiar with.

Really?  What aspect of postgis requires mucking with
shared_preload_libraries?

If you ask me, shared_preload_libraries was only ever meant as a
performance optimization.  If user-visible DDL behavior depends on a
library being preloaded that way, that feature is broken.  There
are some cases where we probably don't care enough to provide a
proper solution, but I'm not sure why we would think that security
labels fall in the don't-really-give-a-damn-if-it-works class.

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] upgrade failure from 9.5 to head

[Stephen Frost]

Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Andres Freund  writes:
> > On 2015-07-29 10:38:19 -0400, Tom Lane wrote:
> >> Now as far as dummy_seclabel is concerned, the easy answer is "we don't
> >> care".  But on reflection, doesn't this mean that the entire
> >> implementation of SECURITY LABEL is broken?  At least to the extent that
> >> it can't work during pg_upgrade unless the user takes manual action to
> >> configure the relevant providers' .so libraries into the new installation
> >> *before* he runs pg_upgrade.  That doesn't say "production ready" to me.
> 
> > Hm, I don't think that particular issue is that bad. We decided labels
> > are only going to work if they're in shared_preload_libararies, and they
> > really only do if that's the case.
> 
> In that case, where in the documentation of the pg_upgrade process does
> it say "you must configure the new installation with all security label
> providers installed in shared_preload_libraries after initdb'ing the
> new installation and before running pg_upgrade"?  And how can you meet
> that requirement if you are using a canned script that does both those
> steps for you?  (Red Hat certainly ships such a script in their packaging,
> and I rather imagine that the Debian-style packages do too.)

The Debian packages have a place where you can drop scripts to be run
during the process to address exactly this issue.  I specifically asked
for that to be added because of the issues with doing PostGIS upgrades.

> And even more to the point, why exactly should security providers get this
> dispensation when we don't make people jump through hoops like that for
> anything else?  AFAICS, with the way things are now, if you simply load
> a dump script without bothering with setting up shared_preload_libraries,
> then you have all the objects loaded and no security labels attached to
> them.  Isn't that a security breach by definition?

Upgrades are not part of normal operation and if you goof it up then,
yes, you're going to run into problems, but anyone who is going through
that process is going to care quite a bit about making sure they do it
correctly, including testing the process before going through it on a
real system.

> I think it's fairly broken if pg_upgrade output, or pg_dump output in
> general, can't be loaded without such requirements.  Perhaps we could
> have the dump script issue a LOAD for the label providers that will be
> referenced; or maybe better, fix "SECURITY LABEL FOR provider" so that
> it autoloads the relevant provider, which would require either a mapping
> table or some convention about the .so name for a provider.
> 
> IMO, the current situation is fine for toy providers like dummy_seclabel,
> but if you want the feature to ever be regarded as more than a toy,
> this issue needs work.

I'm all for improving on the current situation.  I agree that it's not a
terribly good state to be in.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] upgrade failure from 9.5 to head

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > * Andres Freund (and...@anarazel.de) wrote:
> >> Hm. That issue doesn't particularly concern me. Having all .so's
> >> available in the installation seems like a pretty basic
> >> requirement. Security labels are by far not the only things that'll fail
> >> without an extension's .so present, no?
> 
> > It's certainly an issue that postgis users are familiar with.
> 
> Really?  What aspect of postgis requires mucking with
> shared_preload_libraries?

Having to have the libraries in place is what I was getting at, which is
what Andres was also talking about, if I understood correctly.

Even without having to muck with shared_preload_libraries though, you
had better have those libraries in place if you want things to work.
Having to also deal with shared_preload_libraries for some cases doesn't
strike me as a huge issue.

> If you ask me, shared_preload_libraries was only ever meant as a
> performance optimization.  If user-visible DDL behavior depends on a
> library being preloaded that way, that feature is broken.  There
> are some cases where we probably don't care enough to provide a
> proper solution, but I'm not sure why we would think that security
> labels fall in the don't-really-give-a-damn-if-it-works class.

I agree that labels are important and that we really want the label
provider loaded from shared_preload_libraries.  I also understand that
shared_preload_libraries was originally intended as a performance
optimization and that the security labels system has ended up changing
that.  I don't believe that'll be the last thing which we want to be
loaded and running from the very start (if we end up with auditing as an
extension, or any hooks in the postmaster that we provide for extensions
to use, etc).

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] upgrade failure from 9.5 to head

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> Really?  What aspect of postgis requires mucking with
>> shared_preload_libraries?

> Having to have the libraries in place is what I was getting at, which is
> what Andres was also talking about, if I understood correctly.

Right, I agree with that: you need to have installed all the software
you're using, where "installed" means "the executable files are built
and placed where they need to be in the filesystem".

> Having to also deal with shared_preload_libraries for some cases doesn't
> strike me as a huge issue.

I think it is, especially if what we're offering as a workaround is "write
a custom script and make sure that your pg_upgrade wrapper script has an
option to call that halfway through".  Rube Goldberg would be proud.

It's possible that the problem here is not so much reliance on
shared_preload_libraries as it is that there's no provision in
pg_upgrade for dealing with the need to set it.  But one way or
the other, this is a usability fail.

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] more RLS oversights

[Joe Conway]

On 07/29/2015 01:01 AM, Dean Rasheed wrote:
> The CreatePolicy() and AlterPolicy() changes look OK to me, but the
> RemovePolicyById() change looks to be unnecessary ---
> RemovePolicyById() is called only from doDeletion(), which in turned
> is called only from deleteOneObject(), which already invokes the drop
> hooks. So ISTM that RemovePolicyById() doesn't need to do anything,
> right?

Seems correct. Will remove that change and commit it that way.

Joe



-- 
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] upgrade failure from 9.5 to head

[Alvaro Herrera]

Tom Lane wrote:
> Andres Freund  writes:

> > Ick! So the dummy_seclabel test more or less only works by accident if I
> > see that correctly. The .so is only loaded because the CREATE EXTENSION
> > in the test triggers a CREATE FUNCTION dummy_seclabel_dummy() ... LANG
> > C.

I set it up that way on purpose, because there doesn't seem to be any
other reasonable way.  (It wasn't like that in the original contrib
module).

> But on reflection, doesn't this mean that the entire implementation of
> SECURITY LABEL is broken?

Heck, see the *very first* hunk of this patch:
https://www.postgresql.org/message-id/CACACo5R4VwGddt00qtPmhUhtd%3Dokwu2ECM-j20B6%2BcmgtZF6mQ%40mail.gmail.com
This was found to be necessary during deparsing of SECURITY LABEL
command, and only of that command -- all other DDL is able to pass back
the OID of the corresponding object, so that the deparsing code can look
up the object in catalogs.  But for security label providers there is no
catalog, and there is no way to obtain the identify of the label
provider that I can see.  Thus the ugly hack in the patch: the parse
node has to be altered during execution to make sure the provider is
correctly set up for deparse to be able to obtain it.

That to me seemed pretty broken, but I found no better way.

I don't think this is dummy_seclabel's fault.

-- 
Á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] more RLS oversights

[Joe Conway]

On 07/29/2015 02:41 AM, Dean Rasheed wrote:
> I don't think there is any point in adding the new function
> transformPolicyClause(), which is identical to transformWhereClause().
> You can just use transformWhereClause() with EXPR_KIND_POLICY. It's
> already used for lots of other expression kinds.

Yeah, I was debating that. Although I do think the name
transformWhereClause() is unfortunate. Maybe it ought to be renamed
transformBooleanExpr() or something similar?

> There are a couple of places in test_rls_hooks.c that also need updating.

Right -- thanks

> I think that check_agglevels_and_constraints() and
> transformWindowFuncCall() could be made to emit more targeted error
> messages for EXPR_KIND_POLICY, for example "aggregate functions are
> not allowed in policy USING and WITH CHECK expressions".

ok

Thanks for the review.

Joe




-- 
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] upgrade failure from 9.5 to head

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > Having to also deal with shared_preload_libraries for some cases doesn't
> > strike me as a huge issue.
> 
> I think it is, especially if what we're offering as a workaround is "write
> a custom script and make sure that your pg_upgrade wrapper script has an
> option to call that halfway through".  Rube Goldberg would be proud.
> 
> It's possible that the problem here is not so much reliance on
> shared_preload_libraries as it is that there's no provision in
> pg_upgrade for dealing with the need to set it.  But one way or
> the other, this is a usability fail.

Why would it be pg_upgrade?  When using it, you initdb the new cluster
yourself and you can configure the postgresql.conf in there however you
like before running the actual pg_upgrade.  Sure, if you're using a
wrapper then you need to deal with that, but if you want pg_upgrade to
handle configuration options in postgresql.conf then you're going to
have to pass config info to the wrapper script anyway, which would then
pass it to pg_upgrade.

The wrapper script may even already deal with this if it copies the old
postgresql.conf into place (which I think the Debian one might do, with
a bit of processing for anything that needs to be addressed between the
major versions..  not looking at it right now though).

More generally, I completely agree that this is something which we can
improve upon.  It doesn't seem like a release blocker or something which
we need to fix in the back branches though.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] dblink: add polymorphic functions.

[Corey Huinker]

On Wed, Jul 29, 2015 at 10:48 AM, Tom Lane  wrote:

> Corey Huinker  writes:
> > On Wed, Jul 29, 2015 at 3:48 AM, Heikki Linnakangas 
> wrote:
> >> Let's pursue the "CAST(srf() AS row_rtype)" syntax that Joe suggested
> >> upthread (
> >> http://www.postgresql.org/message-id/559a9643.9070...@joeconway.com).
> For
> >> some reason, the discussion went on around the details of the submitted
> >> patch after that, even though everyone seemed to prefer the CAST()
> syntax.
>
> > I'm all for adding that syntax, but it wouldn't be useful for my purposes
> > unless row_rtype could be a variable, and my understanding is that it
> can't.
>
> Not sure why inserting a variable name is so much better than inserting a
> type name?
>
> regards, tom lane
>

Apologies in advance if I'm going over things you already know. Just trying
to package up the problem statement into something easily digestible.

In a polymorphic function, I don't know the return type. It's whatever type
was specified on the function call.

Say I've written a function with a function like
outer_polymorphic_function(p_rowtype anyelement,p1 ,p2,p3, ...) returns
setof anyelement

And inside that function is a series (probably a handful, but potentially
thousands) of async dblink calls, and their corresponding calls to
dblink_get_result(), which currently return setof record, each of which
needs to be casted to whatever anyelement happens to be given this
execution.

Currently, I have to look up p_rowtype in pg_attribute and pg_class, render
the column specs as valid SQL, and compose the query as a string

   fetch_values_query := 'select * from dblink_get_result($1) as t ( ' ||
'c1 type, c2 othertype, ... ' || ')';

and then execute that dynamically like so:

   return query execute fetch_values_query using l_connection_name;

It would be nice if I didn't have to resort to dynamic SQL do to this.

If the CAST() function is implemented, but does not allow to cast as a
variable, then I'm in the same boat:

   fetch_values_query := 'select * from CAST(dblink_get_result($1) as ' ||
pg_typeof(p_rowtype) || ')';

Admittedly, that's a bit cleaner, but I'm still executing that dynamic SQL
once per connection I made, and there could be a lot of them.

If there were dblink() functions that returned polymorphic results, it
would be a non issue:

   dblink_send_query('conn1','select * from
thing_i_know_is_shaped_like_my_rowtype')
   ...
   return query select * from dblink_get_result_any(p_rowtype,'conn1');


I'm all for the expanded capabilities of CAST(), but I have a specific need
for polymorphic dblink() functions.

Re: [HACKERS] upgrade failure from 9.5 to head

[Tom Lane]

Stephen Frost  writes:
> More generally, I completely agree that this is something which we can
> improve upon.  It doesn't seem like a release blocker or something which
> we need to fix in the back branches though.

No, it's not a release blocker; it's been like this since we invented
security labels, which seems to have been 9.1.  But security labels were
toys in 9.1, and this deficiency is one reason they still are toys.
We need to have a to-do item to get rid of it, rather than claiming
things are just fine as they are.

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] dblink: add polymorphic functions.

[Joe Conway]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 07/29/2015 08:56 AM, Corey Huinker wrote:
> On Wed, Jul 29, 2015 at 10:48 AM, Tom Lane  sure why inserting a variable name is so much better than inserting
> a type name?

> In a polymorphic function, I don't know the return type. It's
> whatever type was specified on the function call.
> 
> Say I've written a function with a function like 
> outer_polymorphic_function(p_rowtype anyelement,p1 ,p2,p3, ...) 
> returns setof anyelement

Remind me where you get p_rowtype at runtime again? At some point you
are still having to calculate it, no?

- -- 
Joe Conway
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJVuPv8AAoJEDfy90M199hlYCwP/i0/berqnjXFch6zFM5TDZ6h
+UxQxMmVg933U6Cdoc+huMz8Hiotix76BZVgHOa8xI7Vktx1y5D7o7auczg31v4o
BkzbJFX9ziK5TXZm5wEvtTPNJOOq25AqvHzmrwr4JnPvyAOCKQASTqhIi95mdflZ
tGI+fuI4Dlkj76JaIuZjIh1rKMdycHsRmVfULVx6luR5LwzhX/iyW66pMkNHPYui
7K3DP2hF4HR6xoq4Jvj+HX1MSLLRAjm6+emx6YKnkSkxCQvL5EzwupWWhr7khrYk
QV0fwbAG5JtQJlid/DxezUFgpi2qs2AoPy2ub7JyEZjY0ULt4ehOx9/pzk0ATMNo
e3jB1H9BHTCiy5tY3VZBCe0uQ3lqiNalyUPcwYviS3yciuNg78rIkCQKe2KritZw
t0BY0SMASjbgIINlOLkDCg3HaYi3JujdbwSR5dG41RxaMej3MMieh3Opyg9bfZhI
TxWLsec7FPW/T23wPGk1aQZ7IbLRlOz95fJlAua6g1d5m6GWE3lyRQAQP+QFNWfX
/dmAy0Hvyp3a1wRsFrsG1W+GoyNV1pwfjXp+QlDDhGf8G/Q4l5s7OzZRcLs0O67Z
3K7Jn2k8ps4ZxA5yqRZdl3aAuaOpf3iqffQEeWQqXx7UAM5Sd8qorOJXpNhw+JtX
9W1YQ3eNgGkDfcOa9JLm
=P2ff
-END PGP SIGNATURE-


-- 
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] dblink: add polymorphic functions.

[Tom Lane]

Corey Huinker  writes:
> On Wed, Jul 29, 2015 at 10:48 AM, Tom Lane  wrote:
>> Not sure why inserting a variable name is so much better than inserting a
>> type name?

> In a polymorphic function, I don't know the return type. It's whatever type
> was specified on the function call.

> Say I've written a function with a function like
> outer_polymorphic_function(p_rowtype anyelement,p1 ,p2,p3, ...) returns
> setof anyelement

> And inside that function is a series (probably a handful, but potentially
> thousands) of async dblink calls, and their corresponding calls to
> dblink_get_result(), which currently return setof record, each of which
> needs to be casted to whatever anyelement happens to be given this
> execution.

Yeah.  I would argue that the appropriate fix for that involves allowing
the "p_rowtype%TYPE" syntax to be used in the CAST.  I think right now
you can only use %TYPE in DECLARE sections, but that's a limitation that
causes problems in more situations than just this one.

Mind you, making that work might be less than trivial :-( ... but it would
have uses well beyond dblink().

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] LWLock deadlock and gdb advice

[Jeff Janes]

On Tue, Jul 28, 2015 at 9:06 AM, Jeff Janes  wrote:

> On Tue, Jul 28, 2015 at 7:06 AM, Andres Freund  wrote:
>
>> Hi,
>>
>> On 2015-07-19 11:49:14 -0700, Jeff Janes wrote:
>> > After applying this patch to commit fdf28853ae6a397497b79f, it has
>> survived
>> > testing long enough to convince that this fixes the problem.
>>
>> What was the actual workload breaking with the bug? I ran a small
>> variety and I couldn't reproduce it yet. I'm not saying there's no bug,
>> I just would like to be able to test my version of the fixes...
>>
>
> It was the torn-page fault-injection code here:
>
>
> https://drive.google.com/open?id=0Bzqrh1SO9FcEfkxFb05uQnJ2cWg0MEpmOXlhbFdyNEItNmpuek1zU2gySGF3Vk1oYXNNLUE
>
> It is not a minimal set, I don't know if all parts of this are necessary
> to rerproduce it.  The whole crash-recovery cycling might not even be
> important.
>


I've reproduced it again against commit b2ed8edeecd715c8a23ae462.

It took 5 hours on a 8 core "Intel(R) Xeon(R) CPU E5-2650".

I also reproduced it in 3 hours on the same machine with both JJ_torn_page
and JJ_xid set to zero (i.e. turned off, no induced crashes), so the
fault-injection patch should not be necessary to get the issue..


Cheers,

Jeff

Re: [HACKERS] LWLock deadlock and gdb advice

[Andres Freund]

On 2015-07-29 09:23:32 -0700, Jeff Janes wrote:
> On Tue, Jul 28, 2015 at 9:06 AM, Jeff Janes  wrote:
> I've reproduced it again against commit b2ed8edeecd715c8a23ae462.
> 
> It took 5 hours on a 8 core "Intel(R) Xeon(R) CPU E5-2650".
> 
> I also reproduced it in 3 hours on the same machine with both JJ_torn_page
> and JJ_xid set to zero (i.e. turned off, no induced crashes), so the
> fault-injection patch should not be necessary to get the issue..

Hm, that's a single socket or dual socket E5-2650 system? That CPU
itself has 8 cores, and can work in a dual socket system, that's why I
ask.


-- 
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] LWLock deadlock and gdb advice

[Jeff Janes]

On Wed, Jul 29, 2015 at 9:26 AM, Andres Freund  wrote:

> On 2015-07-29 09:23:32 -0700, Jeff Janes wrote:
> > On Tue, Jul 28, 2015 at 9:06 AM, Jeff Janes 
> wrote:
> > I've reproduced it again against commit b2ed8edeecd715c8a23ae462.
> >
> > It took 5 hours on a 8 core "Intel(R) Xeon(R) CPU E5-2650".
> >
> > I also reproduced it in 3 hours on the same machine with both
> JJ_torn_page
> > and JJ_xid set to zero (i.e. turned off, no induced crashes), so the
> > fault-injection patch should not be necessary to get the issue..
>
> Hm, that's a single socket or dual socket E5-2650 system? That CPU
> itself has 8 cores, and can work in a dual socket system, that's why I
> ask.
>

It is a virtual machine, and I think a property of the VM software is that
any given virtual machine can only run on one socket of the underlying
hardware.  The real machine is dual socket, though.

Cheers,

Jeff

Re: [HACKERS] Supporting TAP tests with MSVC and Windows

[Heikki Linnakangas]

On 07/29/2015 10:13 AM, Michael Paquier wrote:

On Sat, Jul 25, 2015 at 10:54 PM, Michael Paquier
 wrote:

An updated patch is attached.


Attached is v9, that fixes conflicts with 01f6bb4 and recent commits
that added TAP tests in pg_basebackup series.


Thanks, committed with some more tweaking. Peter just added a 
slurp_file() function to TestLib.pm, so I used that to replace the call 
to 'cat' instead of my previous snippet. I also fixed the issue in the 
pg_basebackup test, that LSN is not necessarily 8 characters long, 
slightly differently. Calling pg_xlogfile_name seemed a bit indirect.


I expanded the documentation on how to download and install IPC::Run. I 
instructed to add PERL5LIB to buildenv.pl, pointing it to the extracted 
IPC-Run-0.94/lib directory. Your phrasing of "putting it into PERL5LIB" 
was pretty vague, and in fact the PERL5LIB environment variable was 
overwritten in vcregress.pl, so just setting the PERL5LIB variable 
wouldn't have worked. I changed vcregress.pl to not do that.


I considered moving the instructions on downloading and installing 
IPC::Run in the Requirements section, instead of the Running the 
Regression Tests section. But there are additional instructions on 
downloading and installing more dependencies in the Building the 
Documentation section too. The other dependencies in the Requirements 
section affect the built binaries. Well, except for the Diff utility. 
We're not totally consistent, but I think it's OK as it is.



Also I added an equivalent of --enable-tap-tests for this MSVC stuff,
removed afterwards by Heikki. Do we want it or not?


On Unix, that option controls whether "make check-world" runs the TAP 
tests, but there is no equivalent of "check-world" on Windows. Maybe 
there should be, but as long as there isn't, the only thing that the 
option did was to stop "vcregress tapcheck" from working, if didn't 
enable it in the config file first. That seems silly, so I removed it. 
We'll need it if we add an equivalent of "make check-world" to 
vcregress, but let's cross that bridge when we get there.


- Heikki



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

Re: [HACKERS] more RLS oversights

[Dean Rasheed]

On 29 July 2015 at 16:52, Joe Conway  wrote:
> On 07/29/2015 02:41 AM, Dean Rasheed wrote:
>> I don't think there is any point in adding the new function
>> transformPolicyClause(), which is identical to transformWhereClause().
>> You can just use transformWhereClause() with EXPR_KIND_POLICY. It's
>> already used for lots of other expression kinds.
>
> Yeah, I was debating that. Although I do think the name
> transformWhereClause() is unfortunate. Maybe it ought to be renamed
> transformBooleanExpr() or something similar?
>

I expect it's probably being used by other people's code though, so
renaming it might cause pain for quite a few people.

Regards,
Dean


-- 
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] dblink: add polymorphic functions.

[Corey Huinker]

On Wed, Jul 29, 2015 at 12:14 PM, Joe Conway  wrote:

> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA1
>
> On 07/29/2015 08:56 AM, Corey Huinker wrote:
> > On Wed, Jul 29, 2015 at 10:48 AM, Tom Lane  > sure why inserting a variable name is so much better than inserting
> > a type name?
>
> > In a polymorphic function, I don't know the return type. It's
> > whatever type was specified on the function call.
> >
> > Say I've written a function with a function like
> > outer_polymorphic_function(p_rowtype anyelement,p1 ,p2,p3, ...)
> > returns setof anyelement
>
> Remind me where you get p_rowtype at runtime again? At some point you
> are still having to calculate it, no?
>
>
Say I've got a table my_partitioned_table (key1 integer, key2 integer,
metric1 integer, metric2 integer);

And I've got many partitions on that table.

My code lets you do something like this:

select key1, key2, sum(metric1) as a_sum_of_sums, sum(metric2) as
another_sum_of_sums
from execute_buncha_queries(null::my_partitioned_table,
'connection_string_thats_just_a_loopback',
array['select key1, key2, sum(metric1),
sum(metric2) from my_partition_p1 group by 1,2',
  'select key1, key2, sum(metric1),
sum(metric2) from my_partition_p2 group by 1,2',
  ...])
group by 1,2


All those queries happen to return a shape the same as
my_partitioned_table. The query takes the partially summed values, spread
out across a lot of processors, and does the lighter work of summing the
sums.

The function execute_buncha_queries fires off those string queries async,
enough to fill up X number of processors, fetches results as they happen,
and keeps feeding the processors queries until it runs out. But
execute_buncha_queries needs to send back results in the shape of whatever
value was passed in the first parameter.

I can't put a cast around execute_buncha_queries because the function won't
know how to cast the results coming back from dblink.





















select * from
execute_lotta_queries(null::my_table_or_type,'connection_string_to_remote_db',
array['query 1','query 2','query 3'])

Now, it's up to the user to make sure that all the query strings return a
query of shape "my_table_or_type", but that's a runtime problem.
And obviously, there are a lot of connection strings, but that's too much
detail for the problem at hand.

Re: [HACKERS] more RLS oversights

[Joe Conway]

On 07/29/2015 08:46 AM, Joe Conway wrote:
> On 07/29/2015 01:01 AM, Dean Rasheed wrote:
>> The CreatePolicy() and AlterPolicy() changes look OK to me, but the
>> RemovePolicyById() change looks to be unnecessary ---
>> RemovePolicyById() is called only from doDeletion(), which in turned
>> is called only from deleteOneObject(), which already invokes the drop
>> hooks. So ISTM that RemovePolicyById() doesn't need to do anything,
>> right?
> 
> Seems correct. Will remove that change and commit it that way.

Pushed to HEAD and 9.5.

Joe




-- 
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] Reduce ProcArrayLock contention

[Robert Haas]

On Wed, Jul 29, 2015 at 10:54 AM, Amit Kapila  wrote:
> On Mon, Jul 27, 2015 at 8:47 PM, Robert Haas  wrote:
>> On Sat, Jul 25, 2015 at 12:42 AM, Amit Kapila 
>> wrote:
>> > I thought that internal API will automatically take care of it,
>> > example for msvc it uses _InterlockedCompareExchange64
>> > which if doesn't work on 32-bit systems or is not defined, then
>> > we have to use 32-bit version, but I am not certain about
>> > that fact.
>>
>> Instead of using pg_atomic_uint64, how about using pg_atomic_uint32
>> and storing the pgprocno rather than the pointer directly?
>>
>
> Good Suggestion!
>
> I think this can work the way you are suggesting and I am working on
> same.  Here I have one question,  do you prefer to see the code for
> this optimisation be done via some LWLock interface as Pavan is
> suggesting?  I am not very sure if LWLock is a good interface for this
> work, but surely I can encapsulate it into different functions rather than
> doing everything in ProcArrayEndTransaction.

I would try to avoid changing lwlock.c.  It's pretty easy when so
doing to create mechanisms that work now but make further upgrades to
the general lwlock mechanism difficult.  I'd like to avoid 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] LWLock deadlock and gdb advice

[Heikki Linnakangas]

On 07/29/2015 04:10 PM, Andres Freund wrote:

On 2015-07-29 14:22:23 +0200, Andres Freund wrote:

On 2015-07-29 15:14:23 +0300, Heikki Linnakangas wrote:

Ah, ok, that should work, as long as you also re-check the variable's value
after queueing. Want to write the patch, or should I?


I'll try. Shouldn't be too hard.


What do you think about something roughly like the attached?


Hmm. Imagine this:

Backend A has called LWLockWaitForVar(X) on a lock, and is now waiting 
on it. The lock holder releases the lock, and wakes up A. But before A 
wakes up and sees that the lock is free, another backend acquires the 
lock again. It runs LWLockAcquireWithVar to the point just before 
setting the variable's value. Now A wakes up, sees that the lock is 
still (or again) held, and that the variable's value still matches the 
old one, and goes back to sleep. The new lock holder won't wake it up 
until it updates the value again, or to releases the lock.


I think that's OK given the current usage of this in WALInsertLocks, but 
it's scary. At the very least you need comments to explain that race 
condition. You didn't like the new LW_FLAG_VAR_SET flag and the API 
changes I proposed? That eliminates the race condition.


Either way, there is a race condition that if the new lock holder sets 
the variable to the *same* value as before, the old waiter won't 
necessarily wake up even though the lock was free or had a different 
value in between. But that's easier to explain and understand than the 
fact that the value set by LWLockAcquireWithVar() might not be seen by a 
waiter, until you release the lock or update it again.


Another idea is to have LWLockAcquire check the wait queue after setting 
the variable, and wake up anyone who's already queued up. You could just 
call LWLockUpdateVar() from LWLockAcquireCommon to set the variable. I 
would prefer the approach from my previous patch more, though. That 
would avoid having to acquire the spinlock in LWLockAcquire altogether, 
and I actually like the API change from that independently from any 
correctness issues.


The changes in LWLockWaitForVar() and LWLockConflictsWithVar() seem OK 
in principle. You'll want to change LWLockConflictsWithVar() so that the 
spinlock is held only over the "value = *valptr" line, though; the other 
stuff just modifies local variables and don't need to be protected by 
the spinlock. Also, when you enter LWLockWaitForVar(), you're checking 
if the lock is held twice in a row; first at the top of the function, 
and again inside LWLockConflictsWithVar. You could just remove the 
"quick test" at the top.


- Heikki



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

Re: [HACKERS] upgrade failure from 9.5 to head

[Robert Haas]

On Wed, Jul 29, 2015 at 11:28 AM, Tom Lane  wrote:
> It's possible that the problem here is not so much reliance on
> shared_preload_libraries as it is that there's no provision in
> pg_upgrade for dealing with the need to set it.  But one way or
> the other, this is a usability fail.

Andres pretty much put his finger on the problem here when he
mentioned labels on shared objects.  You can imagine trying to design
this API so that when someone makes reference to label provider xyzzy
we look for a function with that name that has some magic return type
and call it, similar to what we already do with FDWs and what you
recently implemented for TABLESAMPLE.  But if the label is on a shared
object, in which database shall we call that function?  Even for
database objects, it won't do at all if the same label provider name
is used to mean different things in different databases.

Speaking as the guy who came up with much of the design for feature
and committed KaiGai's patch implementing it, I have to admit that I
didn't think of what problems this would create for pg_upgrade.  It
seemed to me at the time that this really wasn't that different from
something like pg_stat_statements, which also won't work properly
unless loaded via shared_preload_libraries.  In retrospect, there is a
difference, which is that if you don't load pg_stat_statements, your
DDL and queries will still execute, but in this case, they won't.
That's definitely raising the table stakes, but I'm not sure what to
do about it.  Preserving shared_preload_libraries across pg_upgrade is
a tempting solution, but that isn't guaranteed to solve more problems
than it creates.

-- 
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] dblink: add polymorphic functions.

[Joe Conway]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 07/29/2015 09:40 AM, Corey Huinker wrote:
> Say I've got a table my_partitioned_table (key1 integer, key2
> integer, metric1 integer, metric2 integer);
> 
> And I've got many partitions on that table. My code lets you do
> something like this:
> 
> select key1, key2, sum(metric1) as a_sum_of_sums, sum(metric2) as 
> another_sum_of_sums from
> execute_buncha_queries(null::my_partitioned_table, 
> 'connection_string_thats_just_a_loopback', array['select key1,
> key2, sum(metric1), sum(metric2) from my_partition_p1 group by
> 1,2', 'select key1, key2, sum(metric1), sum(metric2) from
> my_partition_p2 group by 1,2', ...]) group by 1,2

> I can't put a cast around execute_buncha_queries because the
> function won't know how to cast the results coming back from
> dblink.

Ok, gotcha. So Tom's nearby comment about allowing the
"p_rowtype%TYPE" syntax to be used in the CAST is spot on (as usual).
In other words, to get a complete solution for you we would need to
make both things work, so you could do this inside plpgsql:

  select * from cast(dblink(connstr, sql) as p_rowtype%TYPE);

where potentially connstr, sql, p_rowtype are all passed to plpgsql as
arguments. Correct?

Joe

-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJVuRMWAAoJEDfy90M199hlT5sP/3GuKbvZC7Ods3i2SqtTGbTo
raFiKM87ZznswldNjDHVBEp+OntXzb0JbPUf6n/YJoEJGWE95wb40jez5snAV9lO
aJ7CD9P235OgVh7QVTeWIW5Who8Yj1Xx6NF/Gz/06pGoAXQj1QoznnUPYixur4dT
znjWW3XY6eFpfLzIBKIJmcskOKezgqj2F/kRJpgGYVaEm3okVkjubDjmPM5Vbaaa
sd/lDI5ByceIImzL8LaFeBWwUGLYRsP02zamfPiz3p1zMb+ViBvS+NiBG0kMZMCt
bzy6g0kxbLaxkKy/oEQXqinCNY3hEn8eZ7w4Os/3Zk9PCacZAKDrXfqBDTuE6zio
wy/nwBnoTvdBSk0gl+MKoVlmoy0iAV7Hl/R/KtdNdpCTL4Ja6R5V2c/sjWazxAg4
PymaTXi4/SNWTFwAYGJUfGL+i3CMNQfp4U/tGTVPGFZ8thss7FTVNIVR6ZcAzuPM
EHYDZ8cGaewqDF/HdPlJl4ypxF3aS8tzzApiFVpu35+2WHEacwljDV40l8z9Ee1V
E7R0oxG55fgRJKvLSn5Oj59U2iBXgcu0zLLhBhaVyOYhcIZbWe6xvF1UN/RNcOuz
Se10lYSXCTmz3q61HyCNnWFcOtgYSeFA3Lc79vgxJoZWmwnpHYoFEjQxr3qHFeeK
svkoix7k7t7YZUXGebbg
=vM1F
-END PGP SIGNATURE-


-- 
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] dblink: add polymorphic functions.

[Corey Huinker]

>
> Ok, gotcha. So Tom's nearby comment about allowing the
> "p_rowtype%TYPE" syntax to be used in the CAST is spot on (as usual).
> In other words, to get a complete solution for you we would need to
> make both things work, so you could do this inside plpgsql:
>
>   select * from cast(dblink(connstr, sql) as p_rowtype%TYPE);
>
> where potentially connstr, sql, p_rowtype are all passed to plpgsql as
> arguments. Correct?
>


Correct.

Re: [HACKERS] Remaining 'needs review' patchs in July commitfest

[Robert Haas]

On Tue, Jul 28, 2015 at 3:51 PM, Heikki Linnakangas  wrote:
>> plpgsql raise statement with context
>
> Impasse. Everyone wants this feature in some form, but no consensus on
> whether to do this client-side or server-side.

+1 for server-side.  Does anyone other than you even think that the
client side is a reasonable way to go?

>> dblink: add polymorphic result functions
>
>
> Seems pretty ugly to me to add a dummy argument to functions, just so that
> you can specify the result type. The problem it's trying to solve is real,
> though. Should we take it as it is, or wait for some cleaner approach?

+1 for take it.  If we wait for something better, we may be waiting a long time.

>> Asynchronous execution on postgres-fdw
>
> If we're going to have some sort of general-purpose Parallel node support
> soon, should we just forget about this? Or is it still useful? This adds a
> fair amount of new infrastructure, for a fairly niche feature..

IMHO, that's an extremely important feature.  I believe that it will
not be obsoleted by other parallelism work.  Actually, I think that
some of the infrastructure that it introduces may be quite helpful for
parallelism as well, but I haven't looked at in detail yet.  The core
design concern that I have is what to do about this:

Append
-> Scan
-> Scan
-> Scan

Right now, we start each scan when the previous scan finishes.  But
what if - either through parallelism or through this patch - we could
launch scans 2, 3, etc. before scan 1 completes?  If there's a Limit
node above this somewhere we may regret our decision bitterly.  But if
there isn't, we may do an enormous amount of good by starting that
plan early on the speculation that we will in fact need the results
eventually.

I haven't applied a whole lot of brainpower to this problem yet; I'm
sort of hoping someone else will have a good idea.  Kyotaro
Horiguchi's approach seems to be to say that we don't need to solve
this problem at this stage and that we should just not worry about the
possible waste of resources on the remote machine for now; fix it
later, as part of a future patch.   I'm not terribly confident in that
approach, but I don't currently have anything better to propose.

-- 
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: multiple psql option -c

[Pavel Stehule]

Hi

here is proof concept patch

It should be cleaned, but it demonstrates a work well

[pavel@localhost psql]$ ./psql  -C 'select 10 x; select 20 y;'  -C "\l"
postgres
 x

 10
(1 row)

 y

 20
(1 row)

  List of databases
   Name|  Owner   | Encoding |   Collate   |Ctype|   Access
privileges
---+--+--+-+-+---
 postgres  | postgres | UTF8 | en_US.UTF-8 | en_US.UTF-8 |
 template0 | postgres | UTF8 | en_US.UTF-8 | en_US.UTF-8 |
=c/postgres  +
   |  |  | | |
postgres=CTc/postgres
 template1 | postgres | UTF8 | en_US.UTF-8 | en_US.UTF-8 |
=c/postgres  +
   |  |  | | |
postgres=CTc/postgres
(3 rows)


2015-07-28 18:46 GMT+02:00 Andrew Dunstan :

>
> On 07/28/2015 11:52 AM, Pavel Stehule wrote:
>
>>
>>
>> 2015-07-28 15:16 GMT+02:00 Andrew Dunstan > and...@dunslane.net>>:
>>
>>
>> On 07/28/2015 12:08 AM, Pavel Stehule wrote:
>>
>>
>>
>> 2015-07-28 5:24 GMT+02:00 Pavel Stehule
>> mailto:pavel.steh...@gmail.com>
>> > >>:
>>
>>
>>
>> 2015-07-27 21:57 GMT+02:00 Andrew Dunstan
>> mailto:and...@dunslane.net>
>> >>:
>>
>>
>>
>> On 07/27/2015 02:53 PM, Pavel Stehule wrote:
>>
>>
>>
>>
>>
>> I am trying to run parallel execution
>>
>> psql -At -c "select datname from pg_database"
>> postgres |
>> xargs -n 1 -P 3 psql -c "select current_database()"
>>
>>
>>
>>
>> I don't think it's going to be a hugely important
>> feature, but
>> I don't see a problem with creating a new option (-C seems
>> fine) which would have the same effect as if the arguments
>> were contatenated into a file which is then used with
>> -f. IIRC
>> -c has some special characteristics which means it's
>> probably
>> best not to try to extend it for this feature.
>>
>>
>> ok, I'll try to write patch.
>>
>>
>> I have a question. Can be -C option multiple?
>>
>> The SQL is without problem, but what about \x command?
>>
>> postgres=# \dt \dn select 10;
>> No relations found.
>> List of schemas
>> ┌──┬───┐
>> │ Name │ Owner │
>> ╞══╪═══╡
>> └──┴───┘
>> (0 rows)
>>
>> \dn: extra argument "10;" ignored
>>
>>
>>
>> I don't understand the question.
>>
>> You should include one sql or psql command per -C option, ISTM. e.g.
>>
>> psql -C '\dt' -C '\dn' -C 'select 10;'
>>
>>
>> Isn't that what we're talking about with this whole proposal?
>>
>>
>>
>> I am searching some agreement, how to solve a current "-c" limits. I am
>> 100% for >>> psql -C '\dt' -C '\dn' -C 'select 10;' <<<
>>
>>
>>
> I think you're probably best off leaving -c alone. If there are issues to
> be solved for -c they should be handled separately from the feature we
> agree on.
>
> cheers
>
> andrew
>
>
>
>
diff --git a/src/bin/psql/mainloop.c b/src/bin/psql/mainloop.c
new file mode 100644
index b6cef94..bfd5bf5
*** a/src/bin/psql/mainloop.c
--- b/src/bin/psql/mainloop.c
*** MainLoop(FILE *source)
*** 43,48 
--- 43,49 
  	volatile promptStatus_t prompt_status = PROMPT_READY;
  	volatile int count_eof = 0;
  	volatile bool die_on_error = false;
+ 	Commands *cmd = pset.commands;
  
  	/* Save the prior command source */
  	FILE	   *prev_cmd_source;
*** MainLoop(FILE *source)
*** 135,140 
--- 136,156 
  prompt_status = PROMPT_READY;
  			line = gets_interactive(get_prompt(prompt_status));
  		}
+ 		else if (pset.commands != NULL)
+ 		{
+ 			pset.cur_cmd_interactive = false;
+ 
+ 			if (cmd != NULL)
+ 			{
+ line = cmd->actions;
+ cmd = cmd->next;
+ 			}
+ 			else
+ 			{
+ successResult = EXIT_SUCCESS;
+ break;
+ 			}
+ 		}
  		else
  		{
  			line = gets_fromFile(source);
diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h
new file mode 100644
index d34dc28..46d2a81
*** a/src/bin/psql/settings.h
--- b/src/bin/psql/settings.h
*** enum trivalue
*** 77,82 
--- 77,88 
  	TRI_YES
  };
  
+ typedef struct _Commands
+ {
+ 	char *actions;
+ 	struct _Commands *next;
+ } Commands;
+ 
  typedef struct _psqlSettings
  {
  	PGconn	   *db;/* connection to backend */
*** typedef struct _psqlSettings
*** 129,134 
--- 135,141 
  	const char *prompt2;
  	const char *prompt3;
  	PGVerbosity verbosity;		/* current error verbosity level */
+ 	Commands *commands;
  } PsqlSettings;
  
  extern PsqlSettings pset;
diff --git a/src/bin/psql/start

Re: [HACKERS] Planner debug views

[Qingqing Zhou]

On Tue, Jul 28, 2015 at 6:13 PM, Tom Lane  wrote:
> Another point is that we decided a long time ago that EXPLAIN's plain-text
> output format is not intended to be machine-parsable, and so objecting to
> a design on the grounds that it makes machine parsing harder is pretty
> wrongheaded.  I'd think there is plenty of room for dropping in additional
> output data in the non-text output formats.

I think this will work, for example, I can put several sections of the
JSON output:

{
"plan": {
// original EXPLAIN plan tree sits here
},
"paths":{
// paths considered sits here
}
// ...
}

But still, it requires an extra step for user: he will needs to
programming to read through output (easier) and persists into a table for
later query.

Can we simplify above with foreign table methods? There are two major
concerns about this method per previous discussions: security and
usability. I think the main cause is the sharing foreign table design. How
about we put foreign table in separate pg_stat_tmp/ folders, similar
to what Alvaro proposes, and similar to /proc file system. Meanwhile, we
introduce a function to help user create foreign table mapping to these
files. This looks solves the security and usability issues to me:

postgres=# select pg_debug_planner_init();
Foreign table 'pg_planner_rels', 'pg_planner_paths' created.
postgres=# EXPLAIN (debug_planner=on, ...) ...
...
postgres=# select * from pg_planner_paths;
...

Thoughts?

Qngqing


-- 
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] Division by zero in planner.c:grouping_planner()

[Qingqing Zhou]

On Wed, Jul 29, 2015 at 11:26 AM, Piotr Stefaniak
 wrote:
> +   Assert(path_rows != 0);
> if (tuple_fraction >= 1.0)
> tuple_fraction /= path_rows;
> }
>

This does not sounds right: path_rows only used when tuple_fractions
>= 1.0. So the new Assert is an overkill.

Regards,
Qingqing


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

Re: [HACKERS] more RLS oversights

[Joe Conway]

On 07/29/2015 02:41 AM, Dean Rasheed wrote:
> I don't think there is any point in adding the new function
> transformPolicyClause(), which is identical to transformWhereClause().
> You can just use transformWhereClause() with EXPR_KIND_POLICY. It's
> already used for lots of other expression kinds.


Ok -- I went back to using transformWhereClause. I'd still prefer to
change the name -- more than half the uses of the function are for other
than EXPR_KIND_WHERE -- but I don't feel that strongly about it.


> There are a couple of places in test_rls_hooks.c that also need updating.

done

> I think that check_agglevels_and_constraints() and
> transformWindowFuncCall() could be made to emit more targeted error
> messages for EXPR_KIND_POLICY, for example "aggregate functions are
> not allowed in policy USING and WITH CHECK expressions".

done

Unless there are other comments/objections, will commit this later today.

Joe

diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index d8b4390..bcf4a8f 100644
*** a/src/backend/commands/policy.c
--- b/src/backend/commands/policy.c
*** CreatePolicy(CreatePolicyStmt *stmt)
*** 534,545 
  
  	qual = transformWhereClause(qual_pstate,
  copyObject(stmt->qual),
! EXPR_KIND_WHERE,
  "POLICY");
  
  	with_check_qual = transformWhereClause(with_check_pstate,
  		   copyObject(stmt->with_check),
! 		   EXPR_KIND_WHERE,
  		   "POLICY");
  
  	/* Fix up collation information */
--- 534,545 
  
  	qual = transformWhereClause(qual_pstate,
  copyObject(stmt->qual),
! EXPR_KIND_POLICY,
  "POLICY");
  
  	with_check_qual = transformWhereClause(with_check_pstate,
  		   copyObject(stmt->with_check),
! 		   EXPR_KIND_POLICY,
  		   "POLICY");
  
  	/* Fix up collation information */
*** AlterPolicy(AlterPolicyStmt *stmt)
*** 707,713 
  		addRTEtoQuery(qual_pstate, rte, false, true, true);
  
  		qual = transformWhereClause(qual_pstate, copyObject(stmt->qual),
! 	EXPR_KIND_WHERE,
  	"POLICY");
  
  		/* Fix up collation information */
--- 707,713 
  		addRTEtoQuery(qual_pstate, rte, false, true, true);
  
  		qual = transformWhereClause(qual_pstate, copyObject(stmt->qual),
! 	EXPR_KIND_POLICY,
  	"POLICY");
  
  		/* Fix up collation information */
*** AlterPolicy(AlterPolicyStmt *stmt)
*** 730,736 
  
  		with_check_qual = transformWhereClause(with_check_pstate,
  			   copyObject(stmt->with_check),
! 			   EXPR_KIND_WHERE,
  			   "POLICY");
  
  		/* Fix up collation information */
--- 730,736 
  
  		with_check_qual = transformWhereClause(with_check_pstate,
  			   copyObject(stmt->with_check),
! 			   EXPR_KIND_POLICY,
  			   "POLICY");
  
  		/* Fix up collation information */
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 478d8ca..e33adf5 100644
*** a/src/backend/parser/parse_agg.c
--- b/src/backend/parser/parse_agg.c
*** check_agglevels_and_constraints(ParseSta
*** 373,378 
--- 373,385 
  		case EXPR_KIND_WHERE:
  			errkind = true;
  			break;
+ 		case EXPR_KIND_POLICY:
+ 			if (isAgg)
+ err = _("aggregate functions are not allowed in POLICY USING and WITH CHECK expressions");
+ 			else
+ err = _("grouping operations are not allowed in POLICY USING and WITH CHECK expressions");
+ 
+ 			break;
  		case EXPR_KIND_HAVING:
  			/* okay */
  			break;
*** transformWindowFuncCall(ParseState *psta
*** 770,775 
--- 777,785 
  		case EXPR_KIND_WHERE:
  			errkind = true;
  			break;
+ 		case EXPR_KIND_POLICY:
+ 			err = _("aggregate functions are not allowed in POLICY USING and WITH CHECK expressions");
+ 			break;
  		case EXPR_KIND_HAVING:
  			errkind = true;
  			break;
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 0ff46dd..fa77ef1 100644
*** a/src/backend/parser/parse_expr.c
--- b/src/backend/parser/parse_expr.c
*** transformSubLink(ParseState *pstate, Sub
*** 1672,1677 
--- 1672,1678 
  		case EXPR_KIND_FROM_SUBSELECT:
  		case EXPR_KIND_FROM_FUNCTION:
  		case EXPR_KIND_WHERE:
+ 		case EXPR_KIND_POLICY:
  		case EXPR_KIND_HAVING:
  		case EXPR_KIND_FILTER:
  		case EXPR_KIND_WINDOW_PARTITION:
*** ParseExprKindName(ParseExprKind exprKind
*** 3173,3178 
--- 3174,3181 
  			return "function in FROM";
  		case EXPR_KIND_WHERE:
  			return "WHERE";
+ 		case EXPR_KIND_POLICY:
+ 			return "POLICY";
  		case EXPR_KIND_HAVING:
  			return "HAVING";
  		case EXPR_KIND_FILTER:
diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h
index 7ecaffc..5249945 100644
*** a/src/include/parser/parse_node.h
--- b/src/include/parser/parse_node.h
*** typedef enum ParseExprKind
*** 63,69 
  	EXPR_KIND_INDEX_PREDICATE,	/* index predicate */
  	EXPR_

Re: [HACKERS] Planner debug views

[Alvaro Herrera]

Qingqing Zhou wrote:

> Can we simplify above with foreign table methods? There are two major
> concerns about this method per previous discussions: security and
> usability. I think the main cause is the sharing foreign table design.

I think foreign data wrappers are great.  I do not think that we should
try to shape every problem to look like foreign data so that we can
solve it with a foreign data wrapper.  I am a bit nervous that this
keeps being brought up.

-- 
Á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] more RLS oversights

[Dean Rasheed]

On 29 July 2015 at 20:36, Joe Conway  wrote:
> On 07/29/2015 02:41 AM, Dean Rasheed wrote:
>> I don't think there is any point in adding the new function
>> transformPolicyClause(), which is identical to transformWhereClause().
>> You can just use transformWhereClause() with EXPR_KIND_POLICY. It's
>> already used for lots of other expression kinds.
>
>
> Ok -- I went back to using transformWhereClause. I'd still prefer to
> change the name -- more than half the uses of the function are for other
> than EXPR_KIND_WHERE -- but I don't feel that strongly about it.
>
>
>> There are a couple of places in test_rls_hooks.c that also need updating.
>
> done
>
>> I think that check_agglevels_and_constraints() and
>> transformWindowFuncCall() could be made to emit more targeted error
>> messages for EXPR_KIND_POLICY, for example "aggregate functions are
>> not allowed in policy USING and WITH CHECK expressions".
>
> done
>
> Unless there are other comments/objections, will commit this later today.
>

In the final error s/aggregate/window/. Other than that it looks good to me.

Regards,
Dean


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

Re: [HACKERS] more RLS oversights

[Alvaro Herrera]

Joe Conway wrote:
> On 07/29/2015 02:41 AM, Dean Rasheed wrote:
> > I don't think there is any point in adding the new function
> > transformPolicyClause(), which is identical to transformWhereClause().
> > You can just use transformWhereClause() with EXPR_KIND_POLICY. It's
> > already used for lots of other expression kinds.
> 
> Ok -- I went back to using transformWhereClause. I'd still prefer to
> change the name -- more than half the uses of the function are for other
> than EXPR_KIND_WHERE -- but I don't feel that strongly about it.

Currently the comment about it says "for WHERE and allied", maybe this
should be a bit more explicit.  I think it was originally for things
like WHERE and HAVING, and usage slowly extended beyond that.  Does it
make sense to consider the USING clause in CREATE/ALTER POLICY as an
"allied" clause of WHERE?  I don't particularly think so.  I'm just
talking about a small rewording of the comment.

> > I think that check_agglevels_and_constraints() and
> > transformWindowFuncCall() could be made to emit more targeted error
> > messages for EXPR_KIND_POLICY, for example "aggregate functions are
> > not allowed in policy USING and WITH CHECK expressions".
>
> done

> + case EXPR_KIND_POLICY:
> + if (isAgg)
> + err = _("aggregate functions are not allowed in 
> POLICY USING and WITH CHECK expressions");
> + else
> + err = _("grouping operations are not allowed in 
> POLICY USING and WITH CHECK expressions");

I think this reads a bit funny.  What's a "POLICY USING" clause?  I
expect that translators will treat the two words POLICY USING as a
single token, and the result is not going to make any sense.

Maybe "in a policy's USING and WITH CHECK expressions", or perhaps "in
policies's USING and WITH CHECK exprs", not sure.

-- 
Á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] pg_basebackup and replication slots

[Peter Eisentraut]

On 7/29/15 8:00 AM, Andres Freund wrote:
> As far as I understand this subthread the goal is to have a
> pg_basebackup that internally creates a slot, so it can guarantee that
> all the required WAL is present till streamed out by -X
> stream/fetch. The problem with just creating a slot is that it'd "leak"
> if pg_basebackup is killed, or the connection breaks.  The idea with the
> ephemeral slot is that it'd automatically kept alive until the base
> backup is complete, but would also automatically be dropped if the base
> backup fails.

I don't think anyone actually said that.  I think the goal was to add
the functionality that pg_basebackup can optionally create a
(persistent) replication slot, both for its own use and for later use by
streaming.  Just so you don't have to call pg_create...slot() or
pg_receivexlog separately to create the slot.  And it was pointed out
that this created slot would need to be removed if pg_basebackup failed
for some reason.

What you describe also seems useful, but is a different sub-issue.



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

Re: [HACKERS] Transactions involving multiple postgres foreign servers

[Robert Haas]

On Wed, Jul 29, 2015 at 6:58 AM, Ashutosh Bapat
 wrote:
> A user may set atomic_foreign_transaction to ON to guarantee atomicity, IOW
> it throws error when atomicity can not be guaranteed. Thus if application
> accidentally does something to a foreign server, which doesn't support 2PC,
> the transaction would abort. A user may set it to OFF (consciously and takes
> the responsibility of the result) so as not to use 2PC (probably to reduce
> the overheads) even if the foreign server is 2PC compliant. So, I thought a
> GUC would be necessary. We can incorporate the behaviour you are suggesting
> by having atomic_foreign_transaction accept three values "full" (ON
> behaviour), "partial" (behaviour you are describing), "none" (OFF
> behaviour). Default value of this GUC would be "partial". Will that be fine?

I don't really see the point.  If the user attempts a distributed
transaction involving FDWs that can't support atomic foreign
transactions, then I think it's reasonable to assume that they want
that to work rather than arbitrarily fail.  The only situation in
which it's desirable for that to fail is when the user doesn't realize
that the FDW in question doesn't support atomic foreign commit, and
the error message warns them that their assumptions are unfounded.
But can't the user find that out easily enough by reading the
documentation?   So I think that in practice the "full" value of this
GUC would get almost zero use; I think that nearly everyone will be
happy with what you are here calling "partial" or "none".  I'll defer
to any other consensus that emerges, but that's my view.

I think that we should not change the default behavior.  Currently,
the only behavior is not to attempt 2PC.  Let's stick with that.

> About table level atomic commit attribute, I agree that some foreign tables
> might hold "more critical" data than others from the same server, but I am
> not sure whether only that attribute should dictate the atomicity or not. A
> transaction collectively might need to be "atomic" even if the individual
> tables it modified are not set atomic_commit attribute. So, we need a
> transaction level attribute for atomicity, which may be overridden by a
> table level attribute. Should we add support to the table level atomicity
> setting as version 2+?

I'm not hung up on the table-level attribute, but I think having a
server-level attribute rather than a global GUC is a good idea.
However, I welcome other thoughts on that.

>> We should consider other possible designs as well; the choices we make
>> here may have a significant impact on usability.
>
> I looked at other RBDMSes like IBM's federated database or Oracle. They
> support only "full" behaviour as described above with some optimizations
> like LRO. But, I would like to hear about other options.

Yes, I hope others will weigh in.

>> HandleForeignTransaction is not very descriptive, and I think you're
>> jamming together things that ought to be separated.  Let's have a
>> PrepareForeignTransaction and a ResolvePreparedForeignTransaction.
>
> Done, there are three hooks now
> 1. For preparing a foreign transaction
> 2. For resolving a prepared foreign transaction
> 3. For committing/aborting a running foreign transaction (more explanation
> later)

(2) and (3) seem like the same thing.  I don't see any further
explanation later in your email; what am I missing?

> IP might be fine, but consider altering dbname option or dropping it; we
> won't find the prepared foreign transaction in new database.

Probably not, but I think that's the DBA's problem, not ours.

> I think we
> should at least warn the user that there exist a prepared foreign
> transaction on given foreign server or user mapping; better even if we let
> FDW decide which options are allowed to be altered when there exists a
> foreign prepared transaction. The later requires some surgery in the way we
> handle the options.

We can consider that, but I don't think it's an essential part of the
patch, and I'd punt it for now in the interest of keeping this as
simple as possible.

>> Is this a change from the current behavior?
>
> There is no current behaviour defined per say.

My point is that you had some language in the email describing what
happens if the GUC is turned off.  You shouldn't have to describe
that, because there should be absolutely zero difference.  If there
isn't, that's a problem for this patch, and probably a subject for a
different one.

> I have added a built-in pg_fdw_remove() (or any suitable name), which
> removes the prepared foreign transaction entry from the memory and disk. The
> function needs to be called before attempting PITR.  If the recovery points
> to a past time without removing file, we abort the recovery. In such case, a
> DBA can remove the foreign prepared transaction file manually before
> recovery. I have added a hint with that effect in the error message. Is that
> enough?

That seems totally broken.  Before PITR, the database might b

Re: [HACKERS] more RLS oversights

[Robert Haas]

On Wed, Jul 29, 2015 at 3:58 PM, Alvaro Herrera
 wrote:
> I think this reads a bit funny.  What's a "POLICY USING" clause?  I
> expect that translators will treat the two words POLICY USING as a
> single token, and the result is not going to make any sense.
>
> Maybe "in a policy's USING and WITH CHECK expressions", or perhaps "in
> policies's USING and WITH CHECK exprs", not sure.

Yeah, I don't see why we would capitalize POLICY there.

-- 
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] CustomScan and readfuncs.c

[Robert Haas]

On Sun, Jul 26, 2015 at 11:14 AM, Tom Lane  wrote:
> Um ... wait a second.  There is no support in readfuncs for any
> plan node type, and never has been, and I seriously doubt that there
> ever should be.

As KaiGai says, the parallel query stuff contemplates changing this;
how else will we get the plan from the master to each worker?  We
could invent a bespoke serialization format, but that seems to have
exactly nothing to recommend it.

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


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

Re: [HACKERS] upgrade failure from 9.5 to head

[Andrew Dunstan]

On 07/29/2015 11:28 AM, Tom Lane wrote:

Stephen Frost  writes:

* Tom Lane (t...@sss.pgh.pa.us) wrote:

Really?  What aspect of postgis requires mucking with
shared_preload_libraries?

Having to have the libraries in place is what I was getting at, which is
what Andres was also talking about, if I understood correctly.

Right, I agree with that: you need to have installed all the software
you're using, where "installed" means "the executable files are built
and placed where they need to be in the filesystem".


Having to also deal with shared_preload_libraries for some cases doesn't
strike me as a huge issue.

I think it is, especially if what we're offering as a workaround is "write
a custom script and make sure that your pg_upgrade wrapper script has an
option to call that halfway through".  Rube Goldberg would be proud.

It's possible that the problem here is not so much reliance on
shared_preload_libraries as it is that there's no provision in
pg_upgrade for dealing with the need to set it.  But one way or
the other, this is a usability fail.




FWIW, having the test driver add the shared_preload_libraries setting 
got over this hump - the shared library is indeed present in my setup.


The next hump is this, in restoring contrib_regression_test_ddl_parse:

   pg_restore: creating FUNCTION "public"."text_w_default_in("cstring")"
   pg_restore: [archiver (db)] Error while PROCESSING TOC:
   pg_restore: [archiver (db)] Error from TOC entry 243; 1255 62534
   FUNCTION text_w_default_in("cstring") buildfarm
   pg_restore: [archiver (db)] could not execute query: ERROR: pg_type
   OID value not set when in binary upgrade mode
Command was: CREATE FUNCTION "text_w_default_in"("cstring")
   RETURNS "text_w_default"
LANGUAGE "internal" STABLE STRICT
AS $$texti...

Is this worth bothering about, or should I simply remove the database 
before trying to upgrade?


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] Typo in a comment in set_foreignscan_references

[Robert Haas]

On Tue, Jul 28, 2015 at 3:16 AM, Amit Langote
 wrote:
> Attached fixes a minor typo:
>
> s/custom/foreign/g

Committed, 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] dblink: add polymorphic functions.

[Merlin Moncure]

On Wed, Jul 29, 2015 at 12:53 PM, Joe Conway  wrote:
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA1
>
> On 07/29/2015 09:40 AM, Corey Huinker wrote:
>> Say I've got a table my_partitioned_table (key1 integer, key2
>> integer, metric1 integer, metric2 integer);
>>
>> And I've got many partitions on that table. My code lets you do
>> something like this:
>>
>> select key1, key2, sum(metric1) as a_sum_of_sums, sum(metric2) as
>> another_sum_of_sums from
>> execute_buncha_queries(null::my_partitioned_table,
>> 'connection_string_thats_just_a_loopback', array['select key1,
>> key2, sum(metric1), sum(metric2) from my_partition_p1 group by
>> 1,2', 'select key1, key2, sum(metric1), sum(metric2) from
>> my_partition_p2 group by 1,2', ...]) group by 1,2
>
>> I can't put a cast around execute_buncha_queries because the
>> function won't know how to cast the results coming back from
>> dblink.
>
> Ok, gotcha. So Tom's nearby comment about allowing the
> "p_rowtype%TYPE" syntax to be used in the CAST is spot on (as usual).
> In other words, to get a complete solution for you we would need to
> make both things work, so you could do this inside plpgsql:
>
>   select * from cast(dblink(connstr, sql) as p_rowtype%TYPE);

Would this be a pl/pgsql only solution?

merlin


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

Re: [HACKERS] Improving test coverage of extensions with pg_dump

[Andreas Karlsson]

I have reviewed this patch and it compiles runs and the new test case 
passes. The code is also clean and the test seems like a useful 
regression test.


What I do not like though is how the path src/test/tables_fk/t/ tells us 
nothing about what features are of PostgreSQL are tested here. For this 
I personally prefer the earlier versions where I think that was clear.


Another though: would it be worthwhile to also add an assertion to check 
if the data really was restored properly or would that just be redundant 
code?


--
Andreas


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

Re: [HACKERS] more RLS oversights

[Joe Conway]

On 07/29/2015 01:26 PM, Robert Haas wrote:
> On Wed, Jul 29, 2015 at 3:58 PM, Alvaro Herrera
>  wrote:
>> I think this reads a bit funny.  What's a "POLICY USING" clause?  I
>> expect that translators will treat the two words POLICY USING as a
>> single token, and the result is not going to make any sense.
>>
>> Maybe "in a policy's USING and WITH CHECK expressions", or perhaps "in
>> policies's USING and WITH CHECK exprs", not sure.
> 
> Yeah, I don't see why we would capitalize POLICY there.

The equivalent message for functions is:
  ".. are not allowed in functions in FROM"

So how does this sound:
"... are not allowed in policies in USING and WITH CHECK expressions"
or perhaps more simply:
"... are not allowed in policies in USING and WITH CHECK"

Joe



-- 
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] dblink: add polymorphic functions.

[Tom Lane]

Merlin Moncure  writes:
> On Wed, Jul 29, 2015 at 12:53 PM, Joe Conway  wrote:
>> Ok, gotcha. So Tom's nearby comment about allowing the
>> "p_rowtype%TYPE" syntax to be used in the CAST is spot on (as usual).
>> In other words, to get a complete solution for you we would need to
>> make both things work, so you could do this inside plpgsql:

>> select * from cast(dblink(connstr, sql) as p_rowtype%TYPE);

> Would this be a pl/pgsql only solution?

Well, it would depend on how we fixed %TYPE, but my thought is that
we should teach the core parser to accept variable%TYPE anywhere that
a suitable "variable" is in scope.  The core already allows related
syntaxes in some utility commands, but not within DML commands.

(I am not sure offhand if the existing core syntax is exactly compatible
with what plpgsql does, though; and that could be a problem.)

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