Re: [HACKERS] proposal: session server side variables

[Pavel Stehule]

2017-01-02 3:06 GMT+01:00 Craig Ringer :

>
>
> On 1 Jan. 2017 20:03, "Fabien COELHO"  wrote:
>
>
>
> What if setup_user() succeeds as a function but the transaction it belongs
> to fails for some reason (eg deferred constraints, other operation related
> to setting user up but outside of this function fails, there is replication
> issue... whatever, a transaction may fail by definition)?
>
> ISTM that the security models requires that USER_IS_AUDITOR is reverted,
> so although it is definitely a session variable, it must be transactional
> (MVCC) nevertheless.
>
>
> No strong opinion here.
>
> IMO the simplest answer should be the main focus here: if it's session
> level, it's session level. Not kinda-sesion-level kinda-transaction-level.
>
> I can see occasional uses for what you describe though. If we landed up
> with an xact scope option like we have for SET LOCAL GUCs, the option to
> mark it ON COMMIT RESET or ON COMMIT SET would be useful I guess. I'm not
> sure if it's worth the complexity.
>

In my proposal was support for transaction scope - ON COMMIT RESET clause
should be ok

Regards

Pavel


>
> I guess defaulting to rolling back variable effects on xact rollback would
> be ok too. Just kind of limiting.
>

Re: [HACKERS] Odd behavior with PG_TRY

[Amit Kapila]

On Mon, Jan 2, 2017 at 11:14 AM, Jim Nasby  wrote:
> In the attached patch (snippet below), I'm seeing something strange with
> args->in.r.atts[]. Prior to entering the PG_TRY block, I can inspect things
> in lldb just fine:
>
> (lldb) call args->in.r.atts[0].func
> (PLyDatumToObFunc) $49 = 0x00010fc4dc70
> (plpython3.so`PLyString_FromDatum at plpy_typeio.c:621)
> (lldb) call >in.r.atts[0]
> (PLyDatumToOb *) $50 = 0x7fd2b302f6d0
> (lldb) call args->in.r.atts[0]
> (PLyDatumToOb) $51 = {
>   func = 0x00010fc4dc70 (plpython3.so`PLyString_FromDatum at
> plpy_typeio.c:621)
>   typfunc = {
> fn_addr = 0x00010f478b50 (postgres`textout at varlena.c:521)
> fn_oid = 47
> ...
>
> But I'm getting a EXC_BAD_ACCESS (code=1, address=0xb302f6d0) on the last if
> in the snippet below. Looking at the variables again, I see:
>
> (lldb) call args->in.r.atts[i].func
> error: Couldn't apply expression side effects : Couldn't dematerialize a
> result variable: couldn't read its memory
> (lldb) call args->in.r.atts[i]
> error: Couldn't apply expression side effects : Couldn't dematerialize a
> result variable: couldn't read its memory
>

Looks strange, what is the value of 'i'?  Did you get the same result
if you try to print args->in.r.atts[0] inside PG_TRY?

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


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

Re: [HACKERS] rewrite HeapSatisfiesHOTAndKey

[Amit Kapila]

On Mon, Jan 2, 2017 at 10:59 AM, Pavan Deolasee
 wrote:
>
> On Mon, Jan 2, 2017 at 10:17 AM, Amit Kapila 
> wrote:
>>
>> On Mon, Jan 2, 2017 at 9:28 AM, Pavan Deolasee 
>> wrote:
>> >
>> >
>> > On Mon, Jan 2, 2017 at 8:52 AM, Amit Kapila 
>> > wrote:
>> >>
>> >>
>> >> I think there is some chance that such a change could induce
>> >> regression for the cases when there are many index columns or I think
>> >> even when index is on multiple columns (consider index is on first and
>> >> eight column in a ten column table).
>> >>
>> >
>> > I don't see that as a problem because the routine only checks for
>> > columns
>> > that are passed as "interesting_cols".
>> >
>>
>> Right, but now it will evaluate for all interesting_cols whereas
>> previously it would just stop at first if that column is changed.
>>
>
> Ah ok. I read your comment "consider index is on first and
> eight column in a ten column table" as s/eight/eighth. But may be you're
> referring to
> the case where there is an index on eight or nine columns of a ten column
> table.
>

I am talking about both kinds of cases.  The scenario where we can see
some performance impact is when there is variable-width column before
the index column (in above context before the eighth column) as there
will be cached offset optimization won't work for such a column.

> You're right. That's an additional cost as Alvaro himself explained in the
> original
> post. But both indirect indexes and WARM needs to know information about all
> modified columns. So assuming either of these patches are going to make it,
> we've to bear that cost.
>

Okay, but I think if we know how much is the additional cost in
average and worst case, then we can take a better call.  Also, if we
agree, then doing an update-intensive test on a unlogged table or with
asynchronous commit mode can show us the overhead if there is any.

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


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

Re: [HACKERS] Cache Hash Index meta page.

[Amit Kapila]

On Tue, Dec 27, 2016 at 1:36 PM, Mithun Cy  wrote:
> On Thu, Dec 22, 2016 at 12:17 PM, Amit Kapila  wrote:
>> On Wed, Dec 21, 2016 at 9:26 PM, Robert Haas  wrote:
>>> On Tue, Dec 20, 2016 at 2:25 PM, Mithun Cy  
>>> wrote:
 -- I think if it is okay, I can document same for each member of 
 HashMetaPageData whether to read from cached from meta page or directly 
 from current meta page. Below briefly I have commented for each member. If 
 you suggest I can go with that approach, I will produce a neat patch for 
 same.
>>>
>>> Plain text emails are preferred on this list.
>
> Sorry, I have set the mail to plain text mode now.
>
>> I think this will make metpage cache access somewhat
>> similar to what we have in btree where we use cache to access
>> rootpage.  Will something like that address your concern?
>
> Thanks, just like _bt_getroot I am introducing a new function
> _hash_getbucketbuf_from_hashkey() which will give the target bucket
> buffer for the given hashkey. This will use the cached metapage
> contents instead of reading meta page buffer if cached data is valid.
> There are 2 places which can use this service 1. _hash_first and 2.
> _hash_doinsert.
>

This version of the patch looks much better than the previous version.
I have few review comments:

1.
+ * pin so we can use the metabuf while writing into to it below.
+ */
+ metabuf =
_hash_getbuf(rel, HASH_METAPAGE, HASH_NOLOCK, LH_META_PAGE);

usage "into to .." in above comment seems to be wrong.

2.
- pageopaque->hasho_prevblkno = InvalidBlockNumber;
+
+ /*
+ *
Setting hasho_prevblkno of bucket page with latest maxbucket number
+ * to indicate
bucket has been initialized and need to reconstruct
+ * HashMetaCache if it is older.
+
*/
+ pageopaque->hasho_prevblkno = metap->hashm_maxbucket;

In the above comment, a reference to HashMetaCache is confusing, are
your referring to any structure here?  If you change this, consider to
change the similar usage at other places in the patch as well.

Also, it is not clear to what do you mean by ".. to indicate bucket
has been initialized .."?  assigning maxbucket as a special value to
prevblkno is not related to initializing a bucket page.

3.
 typedef struct HashPageOpaqueData
 {
+ /*
+ * If this is an ovfl page this stores previous ovfl
(or bucket) blkno.
+ * Else if this is a bucket page we use this for a special purpose. We
+ *
store hashm_maxbucket value, whenever this page is initialized or
+ * split. So this helps us
to know whether the bucket has been split after
+ * caching the some of the meta page data.
See _hash_doinsert(),
+ * _hash_first() to know how to use same.
+ */

In above comment, where you are saying ".. caching the some of the
meta page data .." is slightly confusing, because it appears to me
that you are caching whole of the metapage not some part of it.

4.
+_hash_getbucketbuf_from_hashkey(uint32 hashkey, Relation rel,

Generally, for _hash_* API's, we use rel as the first parameter, so I
think it is better to maintain the same with this API as well.

5.
  _hash_finish_split(rel, metabuf, buf, pageopaque->hasho_bucket,
-   maxbucket, highmask, lowmask);
+   usedmetap->hashm_maxbucket,
+   usedmetap->hashm_highmask,
+   usedmetap->hashm_lowmask);

I think we should add an Assert for the validity of usedmetap before using it.

6. Getting few compilation errors in assert-enabled build.

1>src/backend/access/hash/hashinsert.c(85): error C2065: 'bucket' :
undeclared identifier
1>src/backend/access/hash/hashinsert.c(155): error C2065: 'bucket' :
undeclared identifier

1>src/backend/access/hash/hashsearch.c(223): warning C4101: 'blkno' :
unreferenced local variable
1>  hashutil.c
1>\src\backend\access\hash\hashsearch.c(284): warning C4700:
uninitialized local variable 'bucket' used

7.
+ /*
+ * Check if this bucket is split after we have cached the hash meta
+ * data. To do this we need to check whether cached maxbucket number
+ * is less than or equal to maxbucket number stored in bucket page,
+ * which was set with that times maxbucket number during bucket page
+ * splits. In case of upgrade hashno_prevblkno of old bucket page will
+ * be set with InvalidBlockNumber. And as of now maximum value the
+ * hashm_maxbucket can take is 1 less than InvalidBlockNumber (see
+ * _hash_expandtable). So an explicit check for InvalidBlockNumber in
+ * hasho_prevblkno will tell whether current bucket has been split
+ * after caching hash meta data.
+ */

I can understand what you want to say in above comment, but I think
you can write it in somewhat shorter form.  There is no need to
explain the exact check.

8.
@@ -152,6 +152,11 @@ _hash_readprev(IndexScanDesc scan,
  _hash_relbuf(rel, *bufp);

  *bufp = InvalidBuffer;
+
+ /* If it is a bucket page there will not be a prevblkno. */
+ if ((*opaquep)->hasho_flag & LH_BUCKET_PAGE)
+ return;
+

I don't think above check is 

[HACKERS] Odd behavior with PG_TRY

[Jim Nasby]

In the attached patch (snippet below), I'm seeing something strange with 
args->in.r.atts[]. Prior to entering the PG_TRY block, I can inspect 
things in lldb just fine:


(lldb) call args->in.r.atts[0].func
(PLyDatumToObFunc) $49 = 0x00010fc4dc70 
(plpython3.so`PLyString_FromDatum at plpy_typeio.c:621)

(lldb) call >in.r.atts[0]
(PLyDatumToOb *) $50 = 0x7fd2b302f6d0
(lldb) call args->in.r.atts[0]
(PLyDatumToOb) $51 = {
  func = 0x00010fc4dc70 (plpython3.so`PLyString_FromDatum at 
plpy_typeio.c:621)

  typfunc = {
fn_addr = 0x00010f478b50 (postgres`textout at varlena.c:521)
fn_oid = 47
...

But I'm getting a EXC_BAD_ACCESS (code=1, address=0xb302f6d0) on the 
last if in the snippet below. Looking at the variables again, I see:


(lldb) call args->in.r.atts[i].func
error: Couldn't apply expression side effects : Couldn't dematerialize a 
result variable: couldn't read its memory

(lldb) call args->in.r.atts[i]
error: Couldn't apply expression side effects : Couldn't dematerialize a 
result variable: couldn't read its memory


I saw the comment on PG_TRY about marking things as volatile, but my 
understanding from the comment is I shouldn't even need to do that, 
since these variables aren't being modified.



static bool
PLy_CSreceive(TupleTableSlot *slot, DestReceiver *self)
{
volatile TupleDesc  desc = slot->tts_tupleDescriptor;
volatile CallbackState *myState = (CallbackState *) self;
volatile PLyTypeInfo *args = myState->args;

PLyExecutionContext *old_exec_ctx = 
PLy_switch_execution_context(myState->exec_ctx);
MemoryContext scratch_context = 
PLy_get_scratch_context(myState->exec_ctx);
MemoryContext oldcontext = CurrentMemoryContext;

/* Verify saved state matches incoming slot */
Assert(myState->desc == desc);
Assert(args->in.r.natts == desc->natts);

/* Make sure the tuple is fully deconstructed */
slot_getallattrs(slot);

MemoryContextSwitchTo(scratch_context);

PG_TRY();
{
int i, rv;

/*
 * Do the work in the scratch context to avoid leaking memory 
from the
 * datatype output function calls.
 */
for (i = 0; i < desc->natts; i++)
{
PyObject   *value = NULL;

if (desc->attrs[i]->attisdropped)
continue;

if (myState->lists[i] == NULL)
ereport(ERROR,
(errmsg("missing list for attribute 
%d", i)));
/* XXX If the function can't be null, ditch that check 
*/
if (slot->tts_isnull[i] || args->in.r.atts[i].func == 
NULL)

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 80fc4c4725..ace30d75f0 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -55,7 +55,8 @@ static void _SPI_prepare_oneshot_plan(const char *src, 
SPIPlanPtr plan);
 
 static int _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
  Snapshot snapshot, Snapshot 
crosscheck_snapshot,
- bool read_only, bool fire_triggers, uint64 
tcount);
+ bool read_only, bool fire_triggers, uint64 
tcount,
+ DestReceiver *callback);
 
 static ParamListInfo _SPI_convert_params(int nargs, Oid *argtypes,
Datum *Values, const char *Nulls);
@@ -320,7 +321,34 @@ SPI_execute(const char *src, bool read_only, long tcount)
 
res = _SPI_execute_plan(, NULL,
InvalidSnapshot, 
InvalidSnapshot,
-   read_only, true, 
tcount);
+   read_only, true, 
tcount, NULL);
+
+   _SPI_end_call(true);
+   return res;
+}
+int
+SPI_execute_callback(const char *src, bool read_only, long tcount,
+   DestReceiver *callback)
+{
+   _SPI_plan   plan;
+   int res;
+
+   if (src == NULL || tcount < 0)
+   return SPI_ERROR_ARGUMENT;
+
+   res = _SPI_begin_call(true);
+   if (res < 0)
+   return res;
+
+   memset(, 0, sizeof(_SPI_plan));
+   plan.magic = _SPI_PLAN_MAGIC;
+   plan.cursor_options = 0;
+
+   _SPI_prepare_oneshot_plan(src, );
+
+   res = _SPI_execute_plan(, NULL,
+   InvalidSnapshot, 
InvalidSnapshot,
+

Re: [HACKERS] rewrite HeapSatisfiesHOTAndKey

[Pavan Deolasee]

On Mon, Jan 2, 2017 at 10:17 AM, Amit Kapila 
wrote:

> On Mon, Jan 2, 2017 at 9:28 AM, Pavan Deolasee 
> wrote:
> >
> >
> > On Mon, Jan 2, 2017 at 8:52 AM, Amit Kapila 
> wrote:
> >>
> >>
> >> I think there is some chance that such a change could induce
> >> regression for the cases when there are many index columns or I think
> >> even when index is on multiple columns (consider index is on first and
> >> eight column in a ten column table).
> >>
> >
> > I don't see that as a problem because the routine only checks for columns
> > that are passed as "interesting_cols".
> >
>
> Right, but now it will evaluate for all interesting_cols whereas
> previously it would just stop at first if that column is changed.
>
>
Ah ok. I read your comment "consider index is on first and
eight column in a ten column table" as s/eight/eighth. But may be you're
referring to
the case where there is an index on eight or nine columns of a ten column
table.

You're right. That's an additional cost as Alvaro himself explained in the
original
post. But both indirect indexes and WARM needs to know information about all
modified columns. So assuming either of these patches are going to make it,
we've to bear that cost. Having said that, given that attributes are
usually cached,
the cost may not be significant  since for non-HOT updates, the
attributes will be later fetched anyways while preparing index tuples. So
we're
probably doing that work in advance.

Obviously, I'm not against doing additional performance tests to ensure
that the
cost is not significant, especially if neither indirect indexes nor WARM
gets
committed in 10.0

Thanks,
Pavan
-- 
 Pavan Deolasee   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] rewrite HeapSatisfiesHOTAndKey

[Amit Kapila]

On Mon, Jan 2, 2017 at 9:28 AM, Pavan Deolasee  wrote:
>
>
> On Mon, Jan 2, 2017 at 8:52 AM, Amit Kapila  wrote:
>>
>>
>> I think there is some chance that such a change could induce
>> regression for the cases when there are many index columns or I think
>> even when index is on multiple columns (consider index is on first and
>> eight column in a ten column table).
>>
>
> I don't see that as a problem because the routine only checks for columns
> that are passed as "interesting_cols".
>

Right, but now it will evaluate for all interesting_cols whereas
previously it would just stop at first if that column is changed.


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


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

Re: [HACKERS] safer node casting

[Ashutosh Bapat]

On Sat, Dec 31, 2016 at 11:30 PM, Tom Lane  wrote:
> Peter Eisentraut  writes:
>> I propose a macro castNode() that combines the assertion and the cast,
>> so this would become
>> RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));
>

+1. That would be wonderful.

> Seems like an OK idea, but I'm concerned by the implied multiple
> evaluations, particularly if you're going to apply this to function
> results --- as the above example does.  I think you need to go the
> extra mile to make it single-evaluation.  See newNode() for ideas.
>

+1.

In case the Assert fails, the debugger would halt at castNode macro
and a first time reader would be puzzled to see no assert there.
Obviously looking at the #define should clarify the confusion. But I
don't see how that can be avoided. I was thinking of using a function
castNodeFunc(), but it can't accomodate Assert with _type_. Will
calling this function as checkAndCastNode() help?

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


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

Re: [HACKERS] Logical Replication WIP

[Steve Singer]

On 12/30/2016 05:53 AM, Petr Jelinek wrote:

Hi,

I rebased this for the changes made to inheritance and merged in the
fixes that I previously sent separately.






I'm not sure if the following is expected or not

I have 1 publisher and 1 subscriber.
I then do pg_dump on my subscriber
./pg_dump -h localhost --port 5441 --include-subscriptions 
--no-create-subscription-slot test|./psql --port 5441 test_b


I now can't do a drop database test_b  , which is expected

but I can't drop the subscription either


test_b=# drop subscription mysub;
ERROR:  could not drop replication origin with OID 1, in use by PID 24996

 alter subscription mysub disable;
ALTER SUBSCRIPTION
drop subscription mysub;
ERROR:  could not drop replication origin with OID 1, in use by PID 24996

drop subscription mysub nodrop slot;

doesn't work either.  If I first drop the working/active subscription on 
the original 'test' database it works but I can't seem to drop the 
subscription record on test_b






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

Re: [HACKERS] rewrite HeapSatisfiesHOTAndKey

[Pavan Deolasee]

On Mon, Jan 2, 2017 at 8:52 AM, Amit Kapila  wrote:

>
> I think there is some chance that such a change could induce
> regression for the cases when there are many index columns or I think
> even when index is on multiple columns (consider index is on first and
> eight column in a ten column table).
>
>
I don't see that as a problem because the routine only checks for columns
that are passed as "interesting_cols".

Noticed below comment in interesting-attrs-2.patch
> + * are considered the "key" of rows in the table, and columns that are
> + * part of indirect indexes.
>
> Is it right to mention about indirect indexes in above comment
> considering indirect indexes are still not part of core code?
>

I agree. We can add details about indirect indexes or WARM later, as and
when those patches get committed.


> Pavan, please rebase your WARM patch on top of this and let me know how
> you like it.  I'll post a new version of indirect indexes later this
> week.
>
>
I've rebased WARM on top of this patch and the proposed changes look fine
from WARM's perspective too. I'll send rebased patches separately.

Thanks,
Pavan

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

Re: [HACKERS] rewrite HeapSatisfiesHOTAndKey

[Amit Kapila]

On Thu, Dec 29, 2016 at 4:50 AM, Alvaro Herrera
 wrote:
> Pursuant to my comments at
> https://www.postgresql.org/message-id/20161223192245.hx4rbrxbrwtgwj6i@alvherre.pgsql
> and because of a stupid bug I found in my indirect indexes patch, I
> rewrote (read: removed) HeapSatisfiesHOTAndKey.  The replacement
> function HeapDetermineModifiedColumns returns a bitmapset with a bit set
> for each modified column, for those columns that are listed as
> "interesting" -- currently that set is the ID columns, the "key"
> columns, and the indexed columns.  The new code is much simpler, at the
> expense of a few bytes of additional memory used during heap_update().
>
> All tests pass.
>
> Both WARM and indirect indexes should be able to use this infrastructure
> in a way that better serves them than the current HeapSatisfiesHOTAndKey
> (both patches modify that function in a rather ugly way).  I intend to
> get this pushed shortly unless objections are raised.
>
> The new coding prevents stopping the check early as soon as the three
> result booleans could be determined.
>

I think there is some chance that such a change could induce
regression for the cases when there are many index columns or I think
even when index is on multiple columns (consider index is on first and
eight column in a ten column table).  The reason for such a suspicion
is that heap_getattr() is not so cheap that doing it multiple times is
free.  Do you think it is worth to do few tests before committing the
patch?

Noticed below comment in interesting-attrs-2.patch
+ * are considered the "key" of rows in the table, and columns that are
+ * part of indirect indexes.

Is it right to mention about indirect indexes in above comment
considering indirect indexes are still not part of core code?


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


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

Re: [HACKERS] WIP: [[Parallel] Shared] Hash

[Peter Geoghegan]

On Sat, Dec 31, 2016 at 2:52 AM, Thomas Munro
 wrote:
> Unfortunately it's been a bit trickier than I anticipated to get the
> interprocess batch file sharing and hash table shrinking working
> correctly and I don't yet have a new patch in good enough shape to
> post in time for the January CF.  More soon.

I noticed a bug in your latest revision:

> +   /*
> +* In HJ_NEED_NEW_OUTER, we already selected the current inner batch for
> +* reading from.  If there is a shared hash table, we may have already
> +* partially loaded the hash table in ExecHashJoinPreloadNextBatch.
> +*/
> +   Assert(hashtable->batch_reader.batchno = curbatch);
> +   Assert(hashtable->batch_reader.inner);

Obviously this isn't supposed to be an assignment.

-- 
Peter Geoghegan


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

Re: [HACKERS] proposal: session server side variables

[Craig Ringer]

On 1 Jan. 2017 20:03, "Fabien COELHO"  wrote:



What if setup_user() succeeds as a function but the transaction it belongs
to fails for some reason (eg deferred constraints, other operation related
to setting user up but outside of this function fails, there is replication
issue... whatever, a transaction may fail by definition)?

ISTM that the security models requires that USER_IS_AUDITOR is reverted, so
although it is definitely a session variable, it must be transactional
(MVCC) nevertheless.


No strong opinion here.

IMO the simplest answer should be the main focus here: if it's session
level, it's session level. Not kinda-sesion-level kinda-transaction-level.

I can see occasional uses for what you describe though. If we landed up
with an xact scope option like we have for SET LOCAL GUCs, the option to
mark it ON COMMIT RESET or ON COMMIT SET would be useful I guess. I'm not
sure if it's worth the complexity.

I guess defaulting to rolling back variable effects on xact rollback would
be ok too. Just kind of limiting.

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

[Simon Riggs]

On 20 December 2016 at 15:11, Simon Riggs  wrote:
> On 20 December 2016 at 15:03, Fujii Masao  wrote:
>
>> API for crash recovery will never be changed. That is, crash recovery needs
>> neither recovery.trigger nor standby.trigger. When the server starts a crash
>> recovery without any trigger file, any recovery parameter settings in
>> postgresql.conf are ignored. Right?
>
> Yes. There are no conceptual changes, just the API.
>
> The goals are: visibility and reloading of recovery parameters,
> removal of special case code.

OK, so here's the patch, plus doc cleanup patch.

Trying to fit recovery targets into one parameter was really not
feasible, though I tried. Michael's suggested approach of using
recovery_target_type and recovery_target_value has been more
practical, bearing in mind the 3 other relevant parameters.


1. Any use of the earlier recovery.conf or any of the old recovery
parameter names causes an ERROR, so we have a clear API break

2. Signal files can be placed in a writable directory outside of the
data directory, signal_file_directory

3. To signal recovery, create an empty recovery.signal file. Same as
recovery.conf with standby_mode = off

4. To signal standby, create an empty standby.signal file. Same as
recovery.conf with standby_mode = on

5. recovery.conf parameters are all moved to postgresql.conf, with these changes
a) standby_mode no longer exists
b) trigger_file no longer exists… create a file in signal_directory
c)
recovery_target_type
recovery_target_value
are two new parameters that specify the most important type and value of
targeted recovery, though there are other parameters that affect the
outcome also.

e.g.
recovery_target_type = ‘xid’
recovery_target_value = ‘1234’

d) recovery target are now reloadable, not server start only

e) recovery parameters are shown in the postgresql.conf.sample, but
commented out

f) primary_conninfo and primary_slotinfo are renamed to
sender_conninfo and sender_slotinfo to more accurately reflect that these
parameters are no longer primary/master only

g) Recovery Config chapter in docs is retained (for now, at least) and
will later add a link from the main docs to explain this.

I've left the following points for later discussion/cleanup

* recovery.conf.sample will be removed

* pg_basebackup -R will generate a parameter file written to data
directory that will allow it to work, even when postgresql.conf is in
a different directory
(this bit was the most awkward part of the list of changes)

* hot_standby parameter would be removed


Tests have been modified also, but regrettably at this point only 5
out of 8 regression tests pass. This is most likely because I've had
to make more complex changes around timeline handling. I'll post again
tomorrow when I fix them.

Next steps I plan for this release

* Relocate some more code out of xlog.c and guc.c

* Full rewrite of main text docs

* pg_ctl demote
Allows us to avoid incrementing the timeline when performing a switchover

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


newRecoveryAPI_doc1.v101.patch
Description: Binary data


newRecoveryAPI.v101y.patch
Description: Binary data

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

Re: [HACKERS] Fixing pgbench's logging of transaction timestamps

[Tom Lane]

Fabien COELHO  writes:
>> 3. Forget about using the instr_time result and just have doLog() execute
>> gettimeofday() to obtain the timestamp to print.  This is kind of
>> conceptually ugly, but realistically the added overhead is probably
>> insignificant.  A larger objection might be that on Windows, the result
>> of gettimeofday() isn't very high precision ... but it'd still be a huge
>> improvement over the non-answer you get now.

> Yep.

>> I'm inclined to think that #2 isn't a very good choice; it appears to
>> preserve the current behavior but really doesn't.  So we should either
>> change the behavior as in #1 or expend an extra system call as in #3.
>> Preferences?

> Marginal preference for #3 for KIS? Otherwise any three options seems 
> better than the current status.

OK, done that way.

BTW, why is it that the --aggregate-interval option is unsupported on
Windows?  Is that an artifact of the same disease of assuming too much
about how instr_time is represented?  I don't see any very good reason
for it other than the weird decision to store the result of
INSTR_TIME_GET_DOUBLE in a "long", which seems rather broken in any case.

regards, tom lane


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

Re: [HACKERS] merging some features from plpgsql2 project

[Pavel Stehule]

Hi

I wrote some initial patch

Do you think so has sense to continue in this topic?

Regards

Pavel
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 77e7440..15b867fa 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3616,6 +3616,24 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
 	long		tcount;
 	int			rc;
 	PLpgSQL_expr *expr = stmt->sqlstmt;
+	bool		too_many_rows_check;
+	int			too_many_rows_level;
+
+	if (plpgsql_extra_errors & PLPGSQL_XCHECK_TOOMANYROWS)
+	{
+		too_many_rows_check = true;
+		too_many_rows_level = ERROR;
+	}
+	else if (plpgsql_extra_warnings & PLPGSQL_XCHECK_TOOMANYROWS)
+	{
+		too_many_rows_check = true;
+		too_many_rows_level = WARNING;
+	}
+	else
+	{
+		too_many_rows_check = false;
+		too_many_rows_level = NOTICE;
+	}
 
 	/*
 	 * On the first call for this statement generate the plan, and detect
@@ -3666,7 +3684,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
 	 */
 	if (stmt->into)
 	{
-		if (stmt->strict || stmt->mod_stmt)
+		if (stmt->strict || stmt->mod_stmt || too_many_rows_check)
 			tcount = 2;
 		else
 			tcount = 1;
@@ -3786,7 +3804,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
 		}
 		else
 		{
-			if (n > 1 && (stmt->strict || stmt->mod_stmt))
+			if (n > 1 && (stmt->strict || stmt->mod_stmt || too_many_rows_check))
 			{
 char	   *errdetail;
 
@@ -3795,7 +3813,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
 else
 	errdetail = NULL;
 
-ereport(ERROR,
+ereport(too_many_rows_level == WARNING && !stmt->strict ? WARNING : ERROR,
 		(errcode(ERRCODE_TOO_MANY_ROWS),
 		 errmsg("query returned more than one row"),
 		 errdetail ? errdetail_internal("parameters: %s", errdetail) : 0));
@@ -6009,12 +6027,48 @@ exec_move_row(PLpgSQL_execstate *estate,
 		int			t_natts;
 		int			fnum;
 		int			anum;
+		bool		strict_multiassignment_check;
+		int			strict_multiassignment_level;
+
+		if (plpgsql_extra_errors & PLPGSQL_XCHECK_STRICTMULTIASSIGNMENT)
+		{
+			strict_multiassignment_check = true;
+			strict_multiassignment_level = ERROR;
+		}
+		else if (plpgsql_extra_warnings & PLPGSQL_XCHECK_STRICTMULTIASSIGNMENT)
+		{
+			strict_multiassignment_check = true;
+			strict_multiassignment_level = WARNING;
+		}
+		else
+		{
+			strict_multiassignment_check = false;
+			strict_multiassignment_level = NOTICE;
+		}
 
 		if (HeapTupleIsValid(tup))
 			t_natts = HeapTupleHeaderGetNatts(tup->t_data);
 		else
 			t_natts = 0;
 
+		if (strict_multiassignment_check)
+		{
+			int		i;
+
+			anum = 0;
+			for (i = 0; i < td_natts; i++)
+if (!tupdesc->attrs[i]->attisdropped)
+	anum++;
+
+			if (anum != row->nfields)
+			{
+ereport(strict_multiassignment_level,
+		(errcode(ERRCODE_DATATYPE_MISMATCH),
+		 errmsg("Number of evaluated attributies (%d) does not match expected attributies (%d)",
+	anum, row->nfields)));
+			}
+		}
+
 		anum = 0;
 		for (fnum = 0; fnum < row->nfields; fnum++)
 		{
diff --git a/src/pl/plpgsql/src/pl_handler.c b/src/pl/plpgsql/src/pl_handler.c
index 36868fb..09bec86 100644
--- a/src/pl/plpgsql/src/pl_handler.c
+++ b/src/pl/plpgsql/src/pl_handler.c
@@ -89,6 +89,10 @@ plpgsql_extra_checks_check_hook(char **newvalue, void **extra, GucSource source)
 
 			if (pg_strcasecmp(tok, "shadowed_variables") == 0)
 extrachecks |= PLPGSQL_XCHECK_SHADOWVAR;
+			else if (pg_strcasecmp(tok, "too_many_rows") == 0)
+extrachecks |= PLPGSQL_XCHECK_TOOMANYROWS;
+			else if (pg_strcasecmp(tok, "strict_multi_assignment") == 0)
+extrachecks |= PLPGSQL_XCHECK_STRICTMULTIASSIGNMENT;
 			else if (pg_strcasecmp(tok, "all") == 0 || pg_strcasecmp(tok, "none") == 0)
 			{
 GUC_check_errdetail("Key word \"%s\" cannot be combined with other key words.", tok);
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index c84a97b..820afe4 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -1025,7 +1025,9 @@ extern bool plpgsql_check_asserts;
 
 /* extra compile-time checks */
 #define PLPGSQL_XCHECK_NONE			0
-#define PLPGSQL_XCHECK_SHADOWVAR	1
+#define PLPGSQL_XCHECK_SHADOWVAR	(1 << 1)
+#define PLPGSQL_XCHECK_TOOMANYROWS	(1 << 2)
+#define PLPGSQL_XCHECK_STRICTMULTIASSIGNMENT	(1 << 3)
 #define PLPGSQL_XCHECK_ALL			((int) ~0)
 
 extern int	plpgsql_extra_warnings;
diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 79513e4..b09e83a 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -3422,6 +3422,54 @@ select shadowtest(1);
  t
 (1 row)
 
+-- runtime extra checks
+set plpgsql.extra_warnings to 'too_many_rows';
+do $$
+declare x int;
+begin
+  select v from generate_series(1,2) g(v) into x;
+end;
+$$;
+WARNING:  query returned more than one row
+set plpgsql.extra_errors to 'too_many_rows';
+do $$
+declare x int;
+begin
+  select v from generate_series(1,2) g(v) into x;
+end;
+$$;
+ERROR:  query returned more than one row
+CONTEXT:  PL/pgSQL 

Re: [HACKERS] Commit fest 2017-01 will begin soon!

[Fabrízio de Royes Mello]

Em sáb, 31 de dez de 2016 às 21:52, Michael Paquier <
michael.paqu...@gmail.com> escreveu:

> On Tue, Dec 27, 2016 at 12:41 PM, Michael Paquier
>
>  wrote:
>
> > There are still a couple of days to register patches! So if you don't
>
> > want your fancy feature to be forgotten, please add it in time to the
>
> > CF app. Speaking of which, I am going to have a low bandwidth soon as
>
> > that's a period of National Holidays in Japan for the new year, and I
>
> > don't think I'll be able to mark the CF as in progress AoE time. So if
>
> > somebody could do it for me that would be great :)
>
>
>
> Reminder: just 1 more day. Be careful to have your patches registered!
>
> Hi,

I changed the status to "In Progress".

Regards,

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Peter Eisentraut]

On 12/30/16 9:57 AM, Stephen Frost wrote:
> Additionally, people who are actually using these bits of the system are
> almost certainly going to have to adjust things for the directory
> change,

Some *xlog* functions are commonly used to measure replay lag.  That
usage would not be affected by the directory renaming.  Renaming those
functions would only serve to annoy users and have them delay their
upgrades.

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


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

Re: [HACKERS] proposal: session server side variables

[Pavel Stehule]

2017-01-01 11:28 GMT+01:00 Fabien COELHO :

>
> Hello Pavel, and Happy new year!
>
> (1) Having some kind of variable, especially in interactive mode, allows to
>>>
 manipulate previous results and reuse them later, without having to
> resort to repeated sub-queries or to retype non trivial values.
>
> Client side psql :-variables are untyped and unescaped, thus not very
> convenient for this purpose.
>

 You can currently (ab)use user defined GUCs for this.

>>>
>>> How? It seems that I have missed the syntax to assign the result of a
>>> query to a user-defined guc, and to reuse it simply in a query.
>>>
>>
> postgres=# select set_config('myvar.text', (select
>> current_timestamp::text), false);
>>
>
> Thanks for the pointer! The documentation is rather scarse...
>
> They are indeed session or transaction-alive. They seem to be
> user-private, which is good. However they are text only, casts are needed
> in practice as shown by your example, and I find the syntax quite
> unfriendly for interactive use. I'm not sure about performance.
>

With some simple getter/setter functions you can get better comfort.

For not text variables you needs one cast more - probably only "date"
"timestamp" can be noticeable slower.

Regards

Pavel


>
> I have added a subsection about them in the wiki.
>
> --
> Fabien.
>

Re: [HACKERS] proposal: session server side variables

[Fabien COELHO]

Hello Craig, and happy new year,


Someone asked me off-list what use cases such a thing would have,
since it seems not to be spelled out very clearly in this discussion.
I think we're all assuming knowledge here.

So.

* Session starts
* app does SELECT setup_user('user-auth-key-data', 'some-other-blob')
**  setup_user is SECURITY DEFINER to 'appadmin'
**  'appadmin' owns a variable IS_AUDITOR. Other roles have only read
access to it.
**  setup_user(...) does whatever expensive/slow work it has to do
**   setup_user sets USER_IS_AUDITOR var
* Later RLS policies simply reference USER_IS_AUDITOR var. They don't
need to know the 'user-auth-key-data', or do whatever expensive
processing that it does.
* Other later triggers, etc, also reference USER_IS_AUDITOR
* User cannot make themselves an auditor by SETting USER_IS_AUDITOR

That's the general idea.


After giving it some thoughts, I have a question about this use case wrt 
to transactions:


What if setup_user() succeeds as a function but the transaction it belongs 
to fails for some reason (eg deferred constraints, other operation related 
to setting user up but outside of this function fails, there is 
replication issue... whatever, a transaction may fail by definition)?


ISTM that the security models requires that USER_IS_AUDITOR is reverted, 
so although it is definitely a session variable, it must be transactional 
(MVCC) nevertheless.


--
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] Fixing pgbench's logging of transaction timestamps

[Fabien COELHO]

Hello and happy new year,


-l [...]


My 0.02€:


There are at least three ways we could fix it:

1. Switch over to printing the timestamp in the form of elapsed seconds 
since the pgbench run start, [...] About the only reason I can see for 
liking the current definition is that it makes it possible to correlate 
the pgbench log with external events, and I'm not sure whether that's 
especially useful.


I have found that the ability to correlate different logs, esp. pgbench 
and postgres, is "sometimes" useful.


2. Have pgbench save both the INSTR_TIME_SET_CURRENT() and 
gettimeofday() results for the run start instant. However, it's got a 
nasty problem [...]


I'm not sure how wide the problem would be. I would not expect the 
correlation to be perfect, as there are various protocol/client overheads 
here and there anyway.



3. Forget about using the instr_time result and just have doLog() execute
gettimeofday() to obtain the timestamp to print.  This is kind of
conceptually ugly, but realistically the added overhead is probably
insignificant.  A larger objection might be that on Windows, the result
of gettimeofday() isn't very high precision ... but it'd still be a huge
improvement over the non-answer you get now.


Yep.


I'm inclined to think that #2 isn't a very good choice; it appears to
preserve the current behavior but really doesn't.  So we should either
change the behavior as in #1 or expend an extra system call as in #3.
Preferences?


Marginal preference for #3 for KIS? Otherwise any three options seems 
better than the current status.


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

[Michael Paquier]

On Sun, Jan 1, 2017 at 12:34 PM, David Fetter  wrote:
> I've rolled your patches into this next one and clarified the commit
> message, as there appears to have been some confusion about the scope.

Not all the comments have been considered!

Here are some example..

=# set require_where.delete to on;
SET
=# copy (delete from aa returning a) to stdout;
ERROR:  42601: DELETE requires a WHERE clause when
require_where.delete is set to on
HINT:  To delete all rows, use "WHERE true" or similar.
LOCATION:  require_where_check, require_where.c:42

COPY with DML returning rows are correctly restricted.

Now if I look at WITH clauses...
=# with delete_query as (delete from aa returning a) select * from delete_query;
 a
---
(0 rows)
=# with update_query as (update aa set a = 3 returning a) select *
from update_query;
 a
---
(0 rows)
Oops. That's not cool.

CTAS also perform no checks:
=# create table ab as with delete_query as (delete from aa returning
a) select * from delete_query;
SELECT 0

Is there actually a meaning to have two options? Couldn't we leave
with just one? Actually, I'd just suggest to rip off the option and
just to make the checks enabled when the library is loaded, to get a
module as simple as passwordcheck.

--- /dev/null
+++ b/contrib/require_where/data/test_require_where.data
@@ -0,0 +1,16 @@
+Four
There is no need to load fake data as you need to just check the
parsing of the query. So let's simplify the tests and remove that.

Except that the shape of the module is not that bad. The copyright
notices need to be updated to 2017, and it would be nice to have some
comments at the top of require_where.c to describe what it does.
-- 
Michael


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

Re: [HACKERS] proposal: session server side variables

[Fabien COELHO]

Hello Pavel, and Happy new year!


(1) Having some kind of variable, especially in interactive mode, allows to

manipulate previous results and reuse them later, without having to
resort to repeated sub-queries or to retype non trivial values.

Client side psql :-variables are untyped and unescaped, thus not very
convenient for this purpose.


You can currently (ab)use user defined GUCs for this.


How? It seems that I have missed the syntax to assign the result of a
query to a user-defined guc, and to reuse it simply in a query.



postgres=# select set_config('myvar.text', (select
current_timestamp::text), false);


Thanks for the pointer! The documentation is rather scarse...

They are indeed session or transaction-alive. They seem to be 
user-private, which is good. However they are text only, casts are needed 
in practice as shown by your example, and I find the syntax quite 
unfriendly for interactive use. I'm not sure about performance.


I have added a subsection about them in the wiki.

--
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: [WIP] RE: [HACKERS] DECLARE STATEMENT setting up a connection in ECPG

[Michael Meskes]

Hi Ideriha-san,

> I created a patch.

Thank you. Would it be possible for you to re-create the patch without
the white-space changes? 

> I marked [WIP] to the title because some documentation is lacked and
> format needs some fixing.

I noticed that the docs say the statement is a PostgreSQL addon.
However, I think other databases do have the same statement, or don't
they? 

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


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

[HACKERS] An isolation test for SERIALIZABLE READ ONLY DEFERRABLE

[Thomas Munro]

Hi hackers,

Here is a small patch to add a test exercising SERIALIZABLE READ ONLY
DEFERRABLE.  It shows a well known example of a serialisation anomaly
caused by a read-only transaction under REPEATABLE READ (snapshot
isolation), then shows the different ways that SERIALIZABLE and
SERIALIZABLE READ ONLY DEFERRABLE avoid the anomaly.

To be able to do this, the patch modifies the isolation tester so that
it recognises wait_event SafeSnapshot.

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


read-only-anomaly-under-si.patch
Description: Binary data

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