Re: [HACKERS] Partitioned tables and relfilenode

[Ashutosh Bapat]

On Thu, Feb 23, 2017 at 1:08 PM, Amit Langote
 wrote:
> Thanks for the review.
>
> On 2017/02/23 15:44, Ashutosh Bapat wrote:
>> On Thu, Feb 23, 2017 at 11:19 AM, Amit Langote wrote:
>>> Rewrote that comment block as:
>>>
>>>  *
>>>  * If the parent is a partitioned table, we already set the nominal
>>>  * relation.
>>>  */
>>>
>>
>> I reworded those comments a bit and corrected grammar. Please check in
>> the attached patch.
>
> What was there sounds grammatically correct to me, but fine.
>
 Following condition is not very readable. It's not evident that it's of the
 form (A && B) || C, at a glance it looks like it's A && (B || C).
 +   if ((rte->relkind != RELKIND_PARTITIONED_TABLE &&
 +list_length(appinfos) < 2) || list_length(appinfos) < 1)

 Instead you may rearrage it as
 min_child_rels = (rte->relkind == RELKIND_PARTITIONED_TABLE ? 1 : 2);
 if (list_length(appinfos) < min_child_rels)
>>>
>>> OK, done that way.
>>
>> On a second thought, I added a boolean to check if there were any
>> children created and then reset rte->inh based on that value. That's
>> better than relying on appinfos length now.
>
> @@ -996,10 +996,20 @@ inheritance_planner(PlannerInfo *root)
> /*
> +* Partitioned tables do not have storage for themselves and should 
> not be
> +* scanned.
>
> @@ -1450,6 +1451,21 @@ expand_inherited_rtentry(PlannerInfo *root,
> RangeTblEntry *rte, Index rti)
> /*
> +* Partitioned tables themselves do not have any storage and 
> should not
> +* be scanned. So, do not create child relations for those.
> +*/
>
> I guess we should not have to repeat "partitioned tables do not have
> storage" in all these places.

Hmm, you are right. But they are two different functions and the code
blocks are located away from each other. So, I thought it would be
better to have complete comment there, instead of pointing to other
places. I am fine, if we can reword it without compromising
readability.

>
> + * a partitioned relation as dummy. The duplicate RTE we added for the
> + * parent table is harmless, so we don't bother to get rid of it; ditto for
> + * the useless PlanRowMark node.
>
> There is no duplicate RTE in the partitioned table case, which even my
> original comment failed to consider.  Can you, maybe?

May be we could says "If we have added duplicate RTE for the parent
table, it is harmless ...". Does that sound good?

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

[HACKERS] ToDo: Schema Private Function

[Pavel Stehule]

Hi

I propose a schema private functions as analogy to Oracle package functions.

My target of this proposal is better isolation generally available top
level callable functions from other auxiliary functions. A execution of
functions can be little bit more robust due less dependency on SEARCH_PATH.
The other significant benefit can come with schema session variables.

We can different between three kinds of functions:

1. stored in schema - current implementation - the function has not any
special relation to schema, where it is stored. The most important is
SEARCH_PATH property

2. global in schema - top level callable functions where access to objects
in schema (functions, tables, variables) is preferred - first searching is
in schema, second in SEARCH_PATH. This property is related to visibility
(search-ability) of database objects only. The access rights are
independent.

3. private in schema - second level callable functions where access to
objects in schema is preferred. These functions can be called from any
private in schema, global in schema or stored in schema functions. These
functions can use stored in schema and global in schema functions from
other schema. These functions cannot be called from top-level where schema
is not locked. The access rights are independent feature - so access can be
restricted to some roles for this kind of functions too.

Important questions
===
1. relation to SEARCH_PATH
2. relation to access rights

I propose this feature as orthogonal to access rights.

I have not a plan to implement it immediately - (any volunteer?). I would
to start a discussion and collect requests - and I would to create more
complete image of two joined features: schema pinned functions (private,
global) and schema session variables.

Comments, notes?

Regards

Pavel

Re: [HACKERS] Partitioned tables and relfilenode

[Amit Langote]

Thanks for the review.

On 2017/02/23 15:44, Ashutosh Bapat wrote:
> On Thu, Feb 23, 2017 at 11:19 AM, Amit Langote wrote:
>> Rewrote that comment block as:
>>
>>  *
>>  * If the parent is a partitioned table, we already set the nominal
>>  * relation.
>>  */
>>
> 
> I reworded those comments a bit and corrected grammar. Please check in
> the attached patch.

What was there sounds grammatically correct to me, but fine.

>>> Following condition is not very readable. It's not evident that it's of the
>>> form (A && B) || C, at a glance it looks like it's A && (B || C).
>>> +   if ((rte->relkind != RELKIND_PARTITIONED_TABLE &&
>>> +list_length(appinfos) < 2) || list_length(appinfos) < 1)
>>>
>>> Instead you may rearrage it as
>>> min_child_rels = (rte->relkind == RELKIND_PARTITIONED_TABLE ? 1 : 2);
>>> if (list_length(appinfos) < min_child_rels)
>>
>> OK, done that way.
> 
> On a second thought, I added a boolean to check if there were any
> children created and then reset rte->inh based on that value. That's
> better than relying on appinfos length now.

@@ -996,10 +996,20 @@ inheritance_planner(PlannerInfo *root)
/*
+* Partitioned tables do not have storage for themselves and should not 
be
+* scanned.

@@ -1450,6 +1451,21 @@ expand_inherited_rtentry(PlannerInfo *root,
RangeTblEntry *rte, Index rti)
/*
+* Partitioned tables themselves do not have any storage and 
should not
+* be scanned. So, do not create child relations for those.
+*/

I guess we should not have to repeat "partitioned tables do not have
storage" in all these places.

+ * a partitioned relation as dummy. The duplicate RTE we added for the
+ * parent table is harmless, so we don't bother to get rid of it; ditto for
+ * the useless PlanRowMark node.

There is no duplicate RTE in the partitioned table case, which even my
original comment failed to consider.  Can you, maybe?

Thanks,
Amit




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

Re: [HACKERS] Declarative partitioning - another take

[Amit Langote]

Nagata-san,

On 2017/02/23 16:17, Yugo Nagata wrote:
> Hi,
> 
> I found that a comment for PartitionRoot in ResultRelInfo is missing.
> Although this is trivial, since all other members have comments, I
> think it is needed. Attached is the patch to fix it.

Thanks for taking care of that.

+ * PartitionRoot   relation descriptor for parent 
relation

Maybe: relation descriptor for root parent relation

Thanks,
Amit




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

Re: [HACKERS] Supporting huge pages on Windows

[Tsunakawa, Takayuki]

From: Amit Kapila [mailto:amit.kapil...@gmail.com]
> > Hmm, the large-page requires contiguous memory for each page, so this
> error could occur on a long-running system where the memory is heavily
> fragmented.  For example, please see the following page and check the memory
> with RAMMap program referred there.
> >
> 
> I don't have RAMMap and it might take some time to investigate what is going
> on, but I think in such a case even if it works we should keep the default
> value of huge_pages as off on Windows.  I request somebody else having
> access to Windows m/c to test this patch and if it works then we can move
> forward.

You are right.  I modified the patch so that the code falls back to the 
non-huge page when CreateFileMapping() fails due to the shortage of large 
pages.  That's what the Linux version does.

The other change is to parameterize the Win32 function names in the messages in 
EnableLockPagePrivileges().  This is to avoid adding almost identical messages 
unnecessarily.  I followed Alvaro's comment.  I didn't touch the two existing 
sites that embed Win32 function names.  I'd like to leave it up to the 
committer to decide whether to change as well, because changing them might make 
it a bit harder to apply some bug fixes to earlier releases.

FYI, I could reproduce the same error as Amit on 32-bit Win7, where the total 
RAM is 3.5 GB and available RAM is 2 GB.  I used the attached largepage.c.  
Immediately after the system boot, I could only allocate 8 large pages.  When I 
first tried to allocate 32 large pages, the test program produced:

large page size = 2097152
allocating 32 large pages...
CreateFileMapping failed: error code = 1450

You can build the test program as follows:

cl largepage.c advapi32.lib

Regards
Takayuki Tsunakawa



#include 
#include 
#include 

static void EnableLockPagesPrivilege(void);

void main(int argc, char *argv[])
{
SIZE_T  largePageSize = 0;
HANDLE hmap;
int pages = 1;

largePageSize = GetLargePageMinimum();
printf("large page size = %u\n", largePageSize);

EnableLockPagesPrivilege();

if (argc > 1)
pages = atoi(argv[1]);
printf("allocating %d large pages...\n", pages);

hmap = CreateFileMapping(INVALID_HANDLE_VALUE, NULL,
PAGE_READWRITE | SEC_COMMIT | SEC_LARGE_PAGES,
0, largePageSize * pages,
"myshmem");
if (hmap)
printf("allocated large pages successfully\n");
else
printf("CreateFileMapping failed: error code = %u", 
GetLastError());
}

static void
EnableLockPagesPrivilege(void)
{
HANDLE hToken;
TOKEN_PRIVILEGES tp;
LUID luid;

if (!OpenProcessToken(GetCurrentProcess(), TOKEN_ADJUST_PRIVILEGES | 
TOKEN_QUERY, ))
{
printf("OpenProcessToken failed: error code = %u", 
GetLastError());
exit(1);
}

if (!LookupPrivilegeValue(NULL, SE_LOCK_MEMORY_NAME, ))
{
printf("LookupPrivilegeValue failed: error code = %u", 
GetLastError());
exit(1);
}
tp.PrivilegeCount = 1;
tp.Privileges[0].Luid = luid;
tp.Privileges[0].Attributes = SE_PRIVILEGE_ENABLED;

if (!AdjustTokenPrivileges(hToken, FALSE, , 0, NULL, NULL))
{
printf("AdjustTokenPrivileges failed: error code = %u", 
GetLastError());
exit(1);
}

if (GetLastError() != ERROR_SUCCESS)
{
if (GetLastError() == ERROR_NOT_ALL_ASSIGNED)
printf("could not enable Lock Pages in Memory user 
right");
else
printf("AdjustTokenPrivileges failed: error code = %u", 
GetLastError());
exit(1);
}

CloseHandle(hToken);
}


win_large_pages_v8.patch
Description: win_large_pages_v8.patch

-- 
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] DROP SUBSCRIPTION and ROLLBACK

[Masahiko Sawada]

On Wed, Feb 22, 2017 at 2:17 AM, Masahiko Sawada  wrote:
> On Wed, Feb 22, 2017 at 1:52 AM, Fujii Masao  wrote:
>> On Tue, Feb 21, 2017 at 7:52 PM, Masahiko Sawada  
>> wrote:
>>> On Tue, Feb 21, 2017 at 3:42 AM, Fujii Masao  wrote:
 On Thu, Feb 16, 2017 at 11:40 AM, Masahiko Sawada  
 wrote:
> On Thu, Feb 16, 2017 at 7:52 AM, Petr Jelinek
>  wrote:
>> On 15/02/17 06:43, Masahiko Sawada wrote:
>>> On Tue, Feb 14, 2017 at 1:13 AM, Fujii Masao  
>>> wrote:
 On Mon, Feb 13, 2017 at 4:05 PM, Masahiko Sawada 
  wrote:
> On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinek
>  wrote:
>> On 10/02/17 19:55, Masahiko Sawada wrote:
>>> On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek
>>>  wrote:
 On 08/02/17 07:40, Masahiko Sawada wrote:
> On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier
>  wrote:
>> On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao 
>>  wrote:
>>> On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
>>>  wrote:
 For example what happens if apply crashes during the DROP
 SUBSCRIPTION/COMMIT and is not started because the delete from 
 catalog
 is now visible so the subscription is no longer there?
>>>
>>> Another idea is to treat DROP SUBSCRIPTION in the same way as 
>>> VACUUM, i.e.,
>>> make it emit an error if it's executed within user's 
>>> transaction block.
>>
>> It seems to me that this is exactly Petr's point: using
>> PreventTransactionChain() to prevent things to happen.
>
> Agreed. It's better to prevent to be executed inside user 
> transaction
> block. And I understood there is too many failure scenarios we 
> need to
> handle.
>
>>> Also DROP SUBSCRIPTION should call CommitTransactionCommand() 
>>> just
>>> after removing the entry from pg_subscription, then connect to 
>>> the publisher
>>> and remove the replication slot.
>>
>> For consistency that may be important.
>
> Agreed.
>
> Attached patch, please give me feedback.
>

 This looks good (and similar to what initial patch had btw). Works 
 fine
 for me as well.

 Remaining issue is, what to do about CREATE SUBSCRIPTION then, 
 there are
 similar failure scenarios there, should we prevent it from running
 inside transaction as well?

>>>
>>> Hmm,  after thought I suspect current discussing approach. For
>>> example, please image the case where CRAETE SUBSCRIPTION creates
>>> subscription successfully but fails to create replication slot for
>>> whatever reason, and then DROP SUBSCRIPTION drops the subscription 
>>> but
>>> dropping replication slot is failed. In such case, CREAET 
>>> SUBSCRIPTION
>>> and DROP SUBSCRIPTION return ERROR but the subscription is created 
>>> and
>>> dropped successfully. I think that this behaviour confuse the user.
>>>
>>> I think we should just prevent calling DROP SUBSCRIPTION in user's
>>> transaction block. Or I guess that it could be better to separate 
>>> the
>>> starting/stopping logical replication from subscription management.
>>>
>>
>> We need to stop the replication worker(s) in order to be able to drop
>> the slot. There is no such issue with startup of the worker as that 
>> one
>> is launched by launcher after the transaction has committed.
>>
>> IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION 
>> inside a
>> transaction block and don't do any commits inside of those (so that
>> there are no rollbacks, which solves your initial issue I believe). 
>> That
>> way failure to create/drop slot will result in subscription not being
>> created/dropped which is what we want.

 On second thought, +1.

> I basically agree this option, but why do we need to change CREATE
> SUBSCRIPTION as well?

 Because the window between the creation of replication slot and the 
 transaction
 commit of CREATE SUBSCRIPTION 

Re: [HACKERS] Speedup twophase transactions

[Michael Paquier]

On Thu, Feb 2, 2017 at 3:07 PM, Nikhil Sontakke  wrote:
>>> Yeah. Was thinking about this yesterday. How about adding entries in
>>> TwoPhaseState itself (which become valid later)? Only if it does not
>>> cause a lot of code churn.
>>
>> That's possible as well, yes.
>
> PFA a patch which does the above. It re-uses the TwoPhaseState gxact
> entries to track 2PC PREPARE/COMMIT in shared memory. The advantage
> being that CheckPointTwoPhase() becomes the only place where the fsync
> of 2PC files happens.
>
> A minor annoyance in the patch is the duplication of the code to add
> the 2nd while loop to go through these shared memory entries in
> PrescanPreparedTransactions, RecoverPreparedTransactions and
> StandbyRecoverPreparedTransactions.
>
> Other than this, I ran TAP tests and they succeed as needed.

Thanks for the new patch. Finally I am looking at it... The regression
tests already committed are all able to pass.

twophase.c: In function ‘PrepareRedoAdd’:
twophase.c:2539:20: warning: variable ‘gxact’ set but not used
[-Wunused-but-set-variable]
  GlobalTransaction gxact;
There is a warning at compilation.

The comment at the top of PrescanPreparedTransactions() needs to be
updated. Not only does this routine look for the contents of
pg_twophase, but it does also look at the shared memory contents, at
least those ones marked with inredo and not on_disk.

+   ereport(WARNING,
+   (errmsg("removing future two-phase state data from
memory \"%u\"",
+   xid)));
+   PrepareRedoRemove(xid);
+   continue
Those are not normal (partially because unlink is atomic, but not
durable)... But they match the correct coding pattern regarding
incorrect 2PC entries... I'd really like to see those switched to a
FATAL with unlink() made durable for those calls.

+   /* Deconstruct header */
+   hdr = (TwoPhaseFileHeader *) buf;
+   Assert(TransactionIdEquals(hdr->xid, xid));
+
+   if (TransactionIdPrecedes(xid, result))
+   result = xid;
This portion is repeated three times and could be easily refactored.
You could just have a routine that returns the oldes transaction ID
used, and ignore the result for StandbyRecoverPreparedTransactions()
by casting a (void) to let any kind of static analyzer understand that
we don't care about the result for example. Handling for subxids is
necessary as well depending on the code path. Spliting things into a
could of sub-routines may be more readable as well. There are really a
couple of small parts that can be gathered and strengthened.

+   /*
+* Recreate its GXACT and dummy PGPROC
+*/
+   gxactnew = MarkAsPreparing(xid, gid,
+   hdr->prepared_at,
+   hdr->owner, hdr->database,
+   gxact->prepare_start_lsn,
+   gxact->prepare_end_lsn);
MarkAsPreparing() does not need to be extended with two new arguments.
RecoverPreparedTransactions() is used only at the end of recovery,
where it is not necessary to look at the 2PC state files from the
records. In this code path inredo is also set to false :)

+   {
+   /*
+* Entry could be on disk. Call with giveWarning=false
+* since it can be expected during replay.
+*/
+   RemoveTwoPhaseFile(xid, false);
+   }
This would be removed at the end of recovery anyway as a stale entry,
so that's not necessary.

+   /* Delete TwoPhaseState gxact entry and/or 2PC file. */
+   PrepareRedoRemove(parsed.twophase_xid);
Both things should not be present, no? If the file is pushed to disk
it means that the checkpoint horizon has already moved.

-   ereport(ERROR,
+   /* It's ok to find an entry in the redo/recovery case */
+   if (!gxact->inredo)
+   ereport(ERROR,
(errcode(ERRCODE_DUPLICATE_OBJECT),
 errmsg("transaction identifier \"%s\" is already in use",
gid)));
+   else
+   {
+   found = true;
+   break;
+   }
I would not have thought so.

MarkAsPreparing and MarkAsPreparingInRedo really share the same code.
What if the setup of the dummy PGPROC entry is made conditional?
-- 
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] Declarative partitioning - another take

[Yugo Nagata]

Hi,

I found that a comment for PartitionRoot in ResultRelInfo is missing.
Although this is trivial, since all other members have comments, I
think it is needed. Attached is the patch to fix it.

Regards,
Yugo Nagata

On Tue, 27 Dec 2016 17:59:05 +0900
Amit Langote  wrote:

> On 2016/12/26 19:46, Amit Langote wrote:
> > (Perhaps, the following should be its own new thread)
> > 
> > I noticed that ExecProcessReturning() doesn't work properly after tuple
> > routing (example shows how returning tableoid currently fails but I
> > mention some other issues below):
> > 
> > create table p (a int, b int) partition by range (a);
> > create table p1 partition of p for values from (1) to (10);
> > insert into p values (1) returning tableoid::regclass, *;
> >  tableoid | a | b
> > --+---+---
> >  -| 1 |
> > (1 row)
> > 
> > INSERT 0 1
> > 
> > I tried to fix that in 0007 to get:
> > 
> > insert into p values (1) returning tableoid::regclass, *;
> >  tableoid | a | b
> > --+---+---
> >  p| 1 |
> > (1 row)
> > 
> > INSERT 0 1
> > 
> > But I think it *may* be wrong to return the root table OID for tuples
> > inserted into leaf partitions, because with select we get partition OIDs:
> > 
> > select tableoid::regclass, * from p;
> >  tableoid | a | b
> > --+---+---
> >  p1   | 1 |
> > (1 row)
> > 
> > If so, that means we should build the projection info (corresponding to
> > the returning list) for each target partition somehow.  ISTM, that's going
> > to have to be done within the planner by appropriate inheritance
> > translation of the original returning targetlist.
> 
> Turns out getting the 2nd result may not require planner tweaks after all.
> Unless I'm missing something, translation of varattnos of the RETURNING
> target list can be done as late as ExecInitModifyTable() for the insert
> case, unlike update/delete (which do require planner's attention).
> 
> I updated the patch 0007 to implement the same, including the test. While
> doing that, I realized map_partition_varattnos introduced in 0003 is
> rather restrictive in its applicability, because it assumes varno = 1 for
> the expressions it accepts as input for the mapping.  Mapping returning
> (target) list required modifying map_partition_varattnos to accept
> target_varno as additional argument.  That way, we can map arbitrary
> expressions from the parent attributes numbers to partition attribute
> numbers for expressions not limited to partition constraints.
> 
> Patches 0001 to 0006 unchanged.
> 
> Thanks,
> Amit


-- 
Yugo Nagata 
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 6332ea0..845bdf2 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -326,6 +326,7 @@ typedef struct JunkFilter
  *		onConflictSetWhere		list of ON CONFLICT DO UPDATE exprs (qual)
  *		PartitionCheck			partition check expression
  *		PartitionCheckExpr		partition check expression state
+ *		PartitionRoot			relation descriptor for parent relation
  * 
  */
 typedef struct ResultRelInfo

-- 
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] case_preservation_and_insensitivity = on

[Robert Haas]

On Mon, Feb 20, 2017 at 3:06 PM, Joel Jacobson  wrote:
> On Mon, Feb 20, 2017 at 1:45 AM, Tom Lane  wrote:
>> The versions of autocommit that have actually stood the test of time were
>> implemented on the client side (in psql and JDBC, and I think ODBC as
>> well), where the scope of affected code was lots smaller.  I wonder
>> whether there's any hope of providing something useful for case-folding
>> in that way.  psql's lexer is already smart enough that you could teach it
>> rules like "smash any unquoted identifier to lower case" (probably it
>> would fold keywords too, but that seems OK).  That's probably not much
>> help for custom applications, which aren't likely to be going through
>> psql scripts; but the fact that such behavior is in reach at all on the
>> client side seems encouraging.
>
> This sounds like a really good solution to me,
> since there is actually nothing missing on the PostgreSQL server-side,
> it's merely a matter of inconvenience on the client-side.

It doesn't sound like a good solution to me, because there can be SQL
code inside stored procedures that clients never see.  In fact, a
function or procedure can assemble an SQL query text using arbitrary
Turing-complete logic and then execute it.  In fact, you don't even
really need a function or procedure; the client could send a DO block
that does this directly.  We don't run into this problem with
autocommit because a function or procedure has to run entirely within
a single transaction, so I don't think that is really the same thing.

If you only care about rewriting queries that come directly from a
client and you don't care about DO blocks, then you could probably
make this work, but it still requires that the client parse the query
using a lexer and parser that are very similar to the quite
complicated ones on the server side.  That might be hard to get right,
and it's probably also expensive.

I think that solving this problem on the server side is likely to be a
huge amount of really unrewarding work that might get rejected anyway
after tons of effort, but if you did happen to succeed in solving it,
it would be a good clean solution.  Doing something on the client side
is just a kludge.

-- 
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] case_preservation_and_insensitivity = on

[Jim Nasby]

On 2/20/17 3:30 AM, Joel Jacobson wrote:

Also, I think the --lowercase-uniqueness feature would be useful by
itself even without the --case-preserving feature,
since that might be a good way to enforce a good design of new databases,
as a mix of "users" and "Users" is probably considered ugly by many
system designers.


FWIW, I don't think --lowercase-uniqueness is a good name. 
--case-insensitive-unique would be better.


In addition to that, it'd be interesting to allow for a user-supplied 
name validation function that can throw an error if it sees something it 
doesn't like (such as a name that contains spaces, or one that's longer 
than NAMEDATALEN). I suspect it'd be pretty hard to add that though.


BTW, keep in mind that what you're suggesting here means changing 
*every* catalog that contains a name field. A query against info_schema 
will show you that that's most of them.

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


--
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] Partitioned tables and relfilenode

[Ashutosh Bapat]

On Thu, Feb 23, 2017 at 11:19 AM, Amit Langote
 wrote:
> Thanks for the review.
>
> On 2017/02/22 21:58, Ashutosh Bapat wrote:
 Also we should add tests to make sure the scans on partitioned tables
 without any partitions do not get into problems. PFA patch which adds
 those tests.
>>>
>>> I added the test case you suggest, but kept just the first one.
>>
>> The second one was testing multi-level partitioning case, which
>> deserves a testcase by its own.
>
> Ah, okay.  Added it back.

Thanks.

>
>> Some more comments
>>
>> The comment below seems too verbose. All it can say is "A partitioned table
>> without any partitions results in a dummy relation." I don't think we need to
>> be explicit about rte->inh. But it's more of personal preference. We will 
>> leave
>> it to the committer, if you don't agree.
>> +   /*
>> +* An empty partitioned table, i.e., one without any leaf
>> +* partitions, as signaled by rte->inh being set to false
>> +* by the prep phase (see expand_inherited_rtentry).
>> +*/
>
> It does seem poorly worded.  How about:
>
> /*
>  * A partitioned table without leaf partitions is marked
>  * as a dummy rel.
>  */
>
>> We don't need this comment as well. Rather than repeating what's been said at
>> the top of the function, it should just refer to it like "nominal relation 
>> has
>> been set above for partitioned tables. For other parent relations, we'll use
>> the first child ...".
>> +*
>> +* If the parent is a partitioned table, we do not have a separate 
>> RTE
>> +* representing it as a member of the inheritance set, because we do
>> +* not create a scan plan for it.  As mentioned at the top of this
>> +* function, we make the parent RTE itself the nominal relation.
>>  */
>
> Rewrote that comment block as:
>
>  *
>  * If the parent is a partitioned table, we already set the nominal
>  * relation.
>  */
>

I reworded those comments a bit and corrected grammar. Please check in
the attached patch.

>
>> Following condition is not very readable. It's not evident that it's of the
>> form (A && B) || C, at a glance it looks like it's A && (B || C).
>> +   if ((rte->relkind != RELKIND_PARTITIONED_TABLE &&
>> +list_length(appinfos) < 2) || list_length(appinfos) < 1)
>>
>> Instead you may rearrage it as
>> min_child_rels = (rte->relkind == RELKIND_PARTITIONED_TABLE ? 1 : 2);
>> if (list_length(appinfos) < min_child_rels)
>
> OK, done that way.

On a second thought, I added a boolean to check if there were any
children created and then reset rte->inh based on that value. That's
better than relying on appinfos length now.

Let me know what you think.

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


0003-Avoid-creating-scan-nodes-for-partitioned-tables_v2.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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Welcome to v15, highlights:


Files "conditional.h" and "conditional.c" are missing from the patch.

Also, is there a particular reason why tap test have been removed?

--
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] Enabling parallelism for queries coming from SQL or other PL functions

[Rafia Sabih]

On Wed, Feb 22, 2017 at 10:22 PM, Dilip Kumar  wrote:
>
> Some initial comments.
>
> --
> if (numberTuples || dest->mydest == DestIntoRel)
> use_parallel_mode = false;
>
> if (use_parallel_mode)
> EnterParallelMode();
> + else if (estate->es_plannedstmt->parallelModeNeeded &&
> + (dest->mydest == DestSPI || dest->mydest == DestSQLFunction))
> + {
> + use_parallel_mode = true;
> + EnterParallelMode();
> + }
>
> I think we can simplify this, can we replace above code with something
> like this?
>
> if (dest->mydest == DestIntoRel ||
> numberTuples && (dest->mydest != DestSPI || dest->mydest !=
> DestSQLFunction))
> use_parallel_mode = false;

Yes, it can be simplified to
if (dest->mydest == DestIntoRel || (numberTuples && (dest->mydest !=
DestSPI && dest->mydest ! DestSQLFunction)))
Thanks.
>
> -
>
> + {
> + /* Allow parallelism if the function is not trigger type. */
> + if (estate->func->fn_is_trigger == PLPGSQL_NOT_TRIGGER)
> + exec_res = SPI_execute(querystr, estate->readonly_func,
> CURSOR_OPT_PARALLEL_OK);
> + else
> + exec_res = SPI_execute(querystr, estate->readonly_func, 0);
> + }
>
> The last parameter of SPI_execute is tuple count, not cursorOption,
> you need to fix this. Also, this is crossing the 80 line boundary.
>
Oops, corrected.

> ---
> Any specific reason for not changing SPI_execute_with_args, EXECUTE
> with USING will take this path.
>
Fixed.


-- 
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/


pl_parallel_v2.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] [PATCH] Add pg_disable_checksums() and supporting infrastructure

[Jim Nasby]

On 2/20/17 11:22 AM, David Christensen wrote:

- If an entire cluster is going to be considered as checksummed, then even 
databases that don't allow connections would need to get enabled.

Yeah, the workaround for now would be to require “datallowconn" to be set to 
’t' for all databases before proceeding, unless there’s a way to connect to those 
databases internally that bypasses that check.  Open to ideas, though for a first 
pass seems like the “datallowconn” approach is the least amount of work.


The problem with ignoring datallowconn is any database where that's 
false is fair game for CREATE DATABASE. I think just enforcing that 
everything's connectable is good enough for now.



I like the idea of revalidation, but I'd suggest leaving that off of the first 
pass.

Yeah, agreed.


It might be easier on a first pass to look at supporting per-database checksums 
(in this case, essentially treating shared catalogs as their own database). All 
normal backends do per-database stuff (such as setting current_database) during 
startup anyway. That doesn't really help for things like recovery and 
replication though. :/ And there's still the question of SLRUs (or are those 
not checksum'd today??).

So you’re suggesting that the data_checksums GUC get set per-database context, 
so once it’s fully enabled in the specific database it treats it as in 
enforcing state, even if the rest of the cluster hasn’t completed?  Hmm, might 
think on that a bit, but it seems pretty straightforward.


Something like that, yeah.


What issues do you see affecting replication and recovery specifically (other 
than the entire cluster not being complete)?  Since the checksum changes are 
WAL logged, seems you be no worse the wear on a standby if you had to change 
things.


I'm specifically worried about the entire cluster not being complete. 
That makes it harder for replicas to know what blocks they can and can't 
verify the checksum on.


That *might* still be simpler than trying to handle converting the 
entire cluster in one shot. If it's not simpler I certainly wouldn't do 
it right now.



BTW, it occurs to me that this is related to the problem we have with trying to make changes that 
break page binary compatibility. If we had a method for handling that it would probably be useful 
for enabling checksums as well. You'd essentially treat an un-checksum'd page as if it was an 
"old page version". The biggest problem there is dealing with the potential that the new 
page needs to be larger than the old one was, but maybe there's some useful progress to be had in 
this area before tackling the "page too small" problem.

I agree it’s very similar; my issue is I don’t want to have to postpone 
handling a specific case for some future infrastructure.


Yeah, I was just mentioning it.
--
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)


--
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] dropping partitioned tables without CASCADE

[Ashutosh Bapat]

I think this is ready for committer.

On Thu, Feb 23, 2017 at 12:02 PM, Amit Langote
 wrote:
> On 2017/02/22 21:24, Ashutosh Bapat wrote:
>> On Wed, Feb 22, 2017 at 11:11 AM, Amit Langote wrote:
>>> +   /*
>>> +* Unlike inheritance children, partition tables are expected to be 
>>> dropped
>>> +* when the parent partitioned table gets dropped.
>>> +*/
>>>
>>> Hmm.  Partitions *are* inheritance children, so we perhaps don't need the
>>> part before the comma.  Also, adding "automatically" somewhere in there
>>> would be nice.
>>>
>>> Or, one could just write: /* add an auto dependency for partitions */
>>
>> I changed it in the attached patch to
>> +/*
>> + * Partition tables are expected to be dropped when the parent 
>> partitioned
>> + * table gets dropped.
>> + */
>
> OK, thanks.
>
> Thanks,
> Amit
>
>



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

[HACKERS] Automatic cleanup of oldest WAL segments with pg_receivexlog

[Michael Paquier]

Hi all,

When storing WAL segments on a dedicated partition with
pg_receivexlog, for some deployments, the removal of past WAL segments
depends on the frequency of base backups happening on the server. In
short, once a new base backup is taken, it may not be necessary to
keep around those past WAL segments. Of course a lot of things in this
area depend on the retention policy of the deployments. Still, if a
base backup kicks in rather late, there is a risk to have
pg_receivexlog fail because a full disk in the middle of a segment.
Using a replication slot, this would cause Postgres to get down
because of a bloated pg_wal. I am not talking about such cases here :)

On some other types of deployments I work on, one or more standbys are
waiting behind to take over the cluster in case of a failure of the
primary. In such cases, cold backups are still taken and saved
separately. Those are self-contained and can be restored independently
on the rest to put the cluster back on track using a past state.
Archiving using pg_receivexlog still happens to allow the standbys to
catch up if they get offline for a long period of time, something that
may be caused by an outage or simply by the cloning of a new VM that
can take minutes or dozens of minutes to finish deployment. This new
VM can be as well an atomic copy of the primary ready to be used as a
standby. As the primary server may have already recycled the oldest
wal segments in its pg_wal after two checkpoints, archiving plays an
important role in being sure that things can replay successfully. In
short what matters is that the VM cloning does not take longer than 2
checkpoints, but there is no guarantee that the cloning would finish
on time.

In short for such deployments, and contrary to the type of the first
paragraph, we don't care actually care about the past WAL segments, we
do care more about the newest ones (well, mainly about segments older
than the last 2 checkpoints to be correct still having the newest
segments at hand makes replay faster with restore_command with a local
archive). In such cases, I have found useful the possibility to
automatically remove the past WAL segments from the archive partition
if it gets full and allow archiving to move on to the latest data even
if a new base backup has not kicked in to make the past segments
useless.

The idea is really simple, in order to keep the newest WAL history as
large as possible (it is useful for debuggability purposes to exploit
as much as possible the archive partition), we look at the amount free
space available on the partition of the archives when switching to the
next segment, and simply remove as much data as needed to save space
worth one complete segment. Note that compressed WAL segments this may
be several segments removed at once as there is no way to be sure how
much compression will save. The central point of this reasoning is
really to do the decision-making within pg_receivexlog itself as it is
the only point where we know that a new segment is created. So this
makes the cleanup logic independent on the load of Postgres itself.

On any non-Windows systems, statvfs() would be enough to get the
amount of free space available on a partition and it is
posix-compliant. For Windows, there is GetDiskFreeSpace() available.

Is there any interest for a feature like that? I have a non-polished
patch at hand but I can work on that for the upcoming CF if there are
voices in favor of such a feature. The feature could be simply
activated with a dedicated switch, like --clean-oldest-wal,
--clean-tail-wal, or something like that.

Thanks,
-- 
Michael


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

Re: [HACKERS] dropping partitioned tables without CASCADE

[Amit Langote]

On 2017/02/22 21:24, Ashutosh Bapat wrote:
> On Wed, Feb 22, 2017 at 11:11 AM, Amit Langote wrote:
>> +   /*
>> +* Unlike inheritance children, partition tables are expected to be 
>> dropped
>> +* when the parent partitioned table gets dropped.
>> +*/
>>
>> Hmm.  Partitions *are* inheritance children, so we perhaps don't need the
>> part before the comma.  Also, adding "automatically" somewhere in there
>> would be nice.
>>
>> Or, one could just write: /* add an auto dependency for partitions */
> 
> I changed it in the attached patch to
> +/*
> + * Partition tables are expected to be dropped when the parent 
> partitioned
> + * table gets dropped.
> + */

OK, thanks.

Thanks,
Amit




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

Re: [HACKERS] Measuring replay lag

[Thomas Munro]

On Thu, Feb 23, 2017 at 11:52 AM, Thomas Munro
 wrote:
> The overall graph looks pretty similar, but it is more likely to short
> hiccups caused by occasional slow WAL fsyncs in walreceiver.   See the

I meant to write "more likely to *miss* short hiccups".

-- 
Thomas Munro
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] Measuring replay lag

[Thomas Munro]

On Tue, Feb 21, 2017 at 6:21 PM, Simon Riggs  wrote:
> I think what we need to show some test results with the graph of lag
> over time for these cases:
> 1. steady state - pgbench on master, so we can see how that responds
> 2. blocked apply on standby - so we can see how the lag increases but
> also how the accuracy goes down as the lag increases and whether the
> reported value changes (depending upon algo)
> 3. burst mode - where we go from not moving to moving at high speed
> and then stop again quickly
> +other use cases you or others add

Here are graphs of the 'burst' example from my previous email, with
LAG_TRACKER_BUFFER_SIZE set to 4 (really small so that it fills up)
and 8192 (the size I'm proposing in this patch).  It looks like the
resampling and interpolation work pretty well to me when the buffer is
full.

The overall graph looks pretty similar, but it is more likely to short
hiccups caused by occasional slow WAL fsyncs in walreceiver.   See the
attached graphs with 'spike' in the name: in the large buffer version
we see a short spike in write/flush lag and that results in apply
falling behind, but in the small buffer version we can only guess that
that might have happened because apply fell behind during the 3rd and
4th write bursts.  We don't know exactly why because we didn't have
sufficient samples to detect a short lived write/flush delay.

The workload just does this in a loop:

  DROP TABLE IF EXISTS foo;
  CREATE TABLE foo AS SELECT generate_series(1, 1000);
  SELECT pg_sleep(10);

While testing with a small buffer I found a thinko when write_head is
moved back, fixed in the attached.

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


replication-lag-v4.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] Partitioned tables and relfilenode

[Amit Langote]

Thanks for the review.

On 2017/02/22 21:58, Ashutosh Bapat wrote:
>>> Also we should add tests to make sure the scans on partitioned tables
>>> without any partitions do not get into problems. PFA patch which adds
>>> those tests.
>>
>> I added the test case you suggest, but kept just the first one.
> 
> The second one was testing multi-level partitioning case, which
> deserves a testcase by its own.

Ah, okay.  Added it back.

> Some more comments
> 
> The comment below seems too verbose. All it can say is "A partitioned table
> without any partitions results in a dummy relation." I don't think we need to
> be explicit about rte->inh. But it's more of personal preference. We will 
> leave
> it to the committer, if you don't agree.
> +   /*
> +* An empty partitioned table, i.e., one without any leaf
> +* partitions, as signaled by rte->inh being set to false
> +* by the prep phase (see expand_inherited_rtentry).
> +*/

It does seem poorly worded.  How about:

/*
 * A partitioned table without leaf partitions is marked
 * as a dummy rel.
 */

> We don't need this comment as well. Rather than repeating what's been said at
> the top of the function, it should just refer to it like "nominal relation has
> been set above for partitioned tables. For other parent relations, we'll use
> the first child ...".
> +*
> +* If the parent is a partitioned table, we do not have a separate RTE
> +* representing it as a member of the inheritance set, because we do
> +* not create a scan plan for it.  As mentioned at the top of this
> +* function, we make the parent RTE itself the nominal relation.
>  */

Rewrote that comment block as:

 *
 * If the parent is a partitioned table, we already set the nominal
 * relation.
 */


> Following condition is not very readable. It's not evident that it's of the
> form (A && B) || C, at a glance it looks like it's A && (B || C).
> +   if ((rte->relkind != RELKIND_PARTITIONED_TABLE &&
> +list_length(appinfos) < 2) || list_length(appinfos) < 1)
> 
> Instead you may rearrage it as
> min_child_rels = (rte->relkind == RELKIND_PARTITIONED_TABLE ? 1 : 2);
> if (list_length(appinfos) < min_child_rels)

OK, done that way.

Thanks,
Amit
>From ed28c10fa4c4ae30beb1dbc621b4d38d31f718e1 Mon Sep 17 00:00:00 2001
From: amit 
Date: Mon, 6 Feb 2017 18:03:28 +0900
Subject: [PATCH 1/3] Partitioned tables are empty themselves

So, there is not much point in trying to do things to them that need
accessing files (a later commit will make it so that there is no file
at all to access.)  Things that needed attention are: vacuum, analyze,
truncate, ATRewriteTables.
---
 doc/src/sgml/ddl.sgml   |  7 ++--
 src/backend/commands/analyze.c  | 39 ++--
 src/backend/commands/tablecmds.c| 15 ++--
 src/backend/commands/vacuum.c   | 71 +
 src/backend/postmaster/autovacuum.c | 20 +++
 5 files changed, 123 insertions(+), 29 deletions(-)

diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index ef0f7cf727..c10d312139 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -3840,8 +3840,11 @@ UNION ALL SELECT * FROM measurement_y2008m01;
 
 ANALYZE measurement;
 
-  will only process the master table.  This is true even for partitioned
-  tables.
+  will only process the master table.
+ 
+
+ 
+  This does not apply to partitioned tables though.
  
 
 
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index ed3acb1673..12cd0110b0 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -201,8 +201,7 @@ analyze_rel(Oid relid, RangeVar *relation, int options,
 	 * locked the relation.
 	 */
 	if (onerel->rd_rel->relkind == RELKIND_RELATION ||
-		onerel->rd_rel->relkind == RELKIND_MATVIEW ||
-		onerel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+		onerel->rd_rel->relkind == RELKIND_MATVIEW)
 	{
 		/* Regular table, so we'll use the regular row acquisition function */
 		acquirefunc = acquire_sample_rows;
@@ -234,6 +233,12 @@ analyze_rel(Oid relid, RangeVar *relation, int options,
 			return;
 		}
 	}
+	else if (onerel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+	{
+		/*
+		 * For partitioned tables, we want to do the recursive ANALYZE below.
+		 */
+	}
 	else
 	{
 		/* No need for a WARNING if we already complained during VACUUM */
@@ -253,13 +258,15 @@ analyze_rel(Oid relid, RangeVar *relation, int options,
 	LWLockRelease(ProcArrayLock);
 
 	/*
-	 * Do the normal non-recursive ANALYZE.
+	 * Do the normal non-recursive ANALYZE, non-partitioned relations.
 	 */
-	do_analyze_rel(onerel, options, params, va_cols, acquirefunc, relpages,
-   false, in_outer_xact, elevel);
+	if 

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Wed, Feb 22, 2017 at 6:15 PM, Corey Huinker 
wrote:

> On Wed, Feb 22, 2017 at 5:59 PM, Tom Lane  wrote:
>
>> Ah, I see why *that* wants to know about it ... I think.  I suppose you're
>> arguing that variable expansion shouldn't be able to insert, say, an \else
>> in a non-active branch?  Maybe, but if it can insert an \else in an active
>> branch, then why not non-active too?  Seems a bit inconsistent.
>>
>
> The major reason was avoiding situations like what Daniel showed: where
> value of a variable that is meaningless/undefined in the current
> false-block context gets expanded anyway, and thus code inside a false
> block has effects outside of that block. Granted, his example was
> contrived. I'm open to removing that feature and seeing what breaks in the
> test cases.
>


Welcome to v15, highlights:
- all conditional data structure management moved to conditional.h and
conditional.c
- conditional state lives in mainloop.c and is passed to
HandleSlashCommands, exec_command and get_prompt as needed
- no more pset.active_branch, uses conditional_active(conditional_stack)
instead
- PsqlScanState no longer has branching state
- Implements the %R '@' prompt on false branches.
- Variable expansion is never suppressed even in false blocks, regression
test edited to reflect this.
- ConditionalStack could morph into PsqlFileState without too much work.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..dac8e37 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,79 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+-- set ON_ERROR_STOP in case the variables are not valid boolean expressions
+\set ON_ERROR_STOP on
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+Expressions that do not properly evaluate to true or false will
+generate an error and cause the \if or
+\elif command to fail.  Because that behavior may
+change branching context in undesirable ways (executing code which
+was intended to be skipped, causing \elif,
+\else, and \endif commands to
+pair with the wrong \if, etc), it is
+recommended that scripts which use conditionals also set
+ON_ERROR_STOP.
+
+
+Lines within false branches are not evaluated in any way: queries are
+not sent to the server, non-conditional commands are not evaluated but
+bluntly ignored, nested if-expressions in such branches are also not
+evaluated but are tallied to check for proper nesting.
+
+
+  
 
   
 \ir or \include_relative 
filename
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index c53733f..1492b66 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -21,7 +21,7 @@ REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref
 override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
 LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq
 
-OBJS=  command.o common.o help.o input.o stringutils.o mainloop.o copy.o \
+OBJS=  command.o common.o conditional.o help.o input.o stringutils.o 
mainloop.o copy.o \
startup.o prompt.o variables.o large_obj.o describe.o \
crosstabview.o tab-complete.o \
sql_help.o psqlscanslash.o \
@@ -61,8 +61,16 @@ uninstall:
 
 clean distclean:

Re: [HACKERS] tablesample with partitioned tables

[Amit Langote]

On 2017/02/23 0:54, David Fetter wrote:
> On Wed, Feb 22, 2017 at 04:51:46PM +0900, Amit Langote wrote:
>> Attached patch fixes an oversight that tablesample cannot be used with
>> partitioned tables:
>>
>> create table p (a int) partition by list (a);
>> select * from p tablesample bernoulli (50);
>> ERROR:  TABLESAMPLE clause can only be applied to tables and materialized
>> views
> 
> Thanks!
> 
> Should the error message change somehow to reflect that partitioned
> tables are included?  Is complete transparency of partitioned tables
> the goal, and reasonable in this context?

We avoid mentioning partitioned tables separately during most of the
errors caused by relkind checks.  I mentioned recently [1] that in most of
these sites such as this one, a table's being partitioned is not significant.

> Also, is there a good reason apart from tuits not to expand
> TABLESAMPLE to the rest of our SQL-visible relation structures?  I'm
> guessing this could have something to do with the volatility they
> might have, whether in views that call volatile functions or in
> foreign tables that might not make the right guarantees...

I wouldn't be able to say much about that, but I found an email from the
original discussion that occurred around development of this feature that
posed the same question.  There might be some answers there.

[1]
https://www.postgresql.org/message-id/854ad246-4dfa-5c68-19ad-867b6800f313%40lab.ntt.co.jp
[2] https://www.postgresql.org/message-id/5526D369.1070905%40gmx.net




-- 
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] Range Partitioning behaviour - query

[Amit Langote]

Hi,

On 2017/02/23 11:55, Venkata B Nagothi wrote:
> Hi Hackers,
> 
> I have noticed the following behaviour in range partitioning which i felt
> is not quite correct (i missed reporting this) -
> 
> I have tested by creating a date ranged partition.
> 
> I created the following table.
> 
> db03=# CREATE TABLE orders (
> o_orderkey INTEGER,
> o_custkey INTEGER,
> o_orderstatus CHAR(1),
> o_totalprice REAL,
> o_orderdate DATE,
> o_orderpriority CHAR(15),
> o_clerk CHAR(15),
> o_shippriority INTEGER,
> o_comment VARCHAR(79)) partition by range (o_orderdate);
> CREATE TABLE
> 
> Created the following partitioned tables :
> 
> 
> db03=# CREATE TABLE orders_y1992
> PARTITION OF orders FOR VALUES FROM ('1992-01-01') TO ('1992-12-31');
>   CREATE TABLE
> 
> db03=# CREATE TABLE orders_y1993
> PARTITION OF orders FOR VALUES FROM ('1993-01-01') TO ('*1993-12-31'*);
> CREATE TABLE
> 
> db03=# CREATE TABLE orders_y1994
>PARTITION OF orders FOR VALUES FROM ('1994-01-01') TO ('1994-12-31');
> CREATE TABLE
> 
> 
> The rows with the date "1993-12-31" gets rejected as shown below -
> 
> db03=# copy orders from '/data/orders.csv' delimiter '|';
> ERROR:  no partition of relation "orders" found for row
> DETAIL:  Failing row contains (353, 8878, F, 273342, *1993-12-31*, 5-LOW
>, Clerk#02241, 0,  quiet ideas sleep. even instructions cajole
> slyly. silently spe).
> CONTEXT:  COPY orders, line 89: "353|8878|F|273342|*1993-12-31*|5-LOW
>|Clerk#02241|0| quiet ideas sleep. even instructions..."
> 
> I would want the partition "orders_y1993" to accept all the rows with the
> date 1993-12-31.

[ ... ]

> Am i missing anything here ?

Upper bound of a range partition is an exclusive bound.  A note was added
recently to the CREATE TABLE page to make this clear.

https://www.postgresql.org/docs/devel/static/sql-createtable.html

So do the following instead:

CREATE TABLE orders_y1993
  PARTITION OF orders FOR VALUES FROM ('1993-01-01') TO ('1994-01-01');

Thanks,
Amit




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

Re: [HACKERS] Make subquery alias optional in FROM clause

[Tom Lane]

Robert Haas  writes:
> On Wed, Feb 22, 2017 at 10:33 PM, Nico Williams  wrote:
>> I suspect most users, like me, just roll their eyes, grumble, and put up
>> with it rather than complain.  It's a pain point, but tolerable enough
>> that no one bothers to demand a change.  Now that it's been done though,
>> allow me to add my voice in favor of it!

> +1 to all of that.

[ shrug... ]  Well, I won't resist this hard as long as it's done
competently, which to me means "the subquery name doesn't conflict with
anything else".  Not "it doesn't conflict unless you're unlucky enough
to have used the same name elsewhere".  There are a couple ways we could
achieve that result, but the submitted patch fails to.

(Or, in words of one syllable: if I thought this way was okay, I would
have done it like that back in 2000.)

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] remove deprecated COMMENT ON RULE syntax

[Robert Haas]

On Wed, Feb 22, 2017 at 8:05 PM, Tom Lane  wrote:
> Peter Eisentraut  writes:
>> There is support for COMMENT ON RULE  without specifying a table
>> name, for upgrading from pre-7.3 instances.  I think it might be time
>> for that workaround to go.
>
> No objection here.

Yeah, probably so.

-- 
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] Make subquery alias optional in FROM clause

[Andres Freund]

On 2017-02-23 08:21:41 +0530, Robert Haas wrote:
> On Wed, Feb 22, 2017 at 10:33 PM, Nico Williams  wrote:
> > On Wed, Feb 22, 2017 at 10:08:38AM -0500, Tom Lane wrote:
> >> Bernd Helmle  writes:
> >> >> From time to time, especially during migration projects from Oracle to
> >> > PostgreSQL, i'm faced with people questioning why the alias in the FROM
> >> > clause for subqueries in PostgreSQL is mandatory. The default answer
> >> > here is, the SQL standard requires it.
> >>
> >> Indeed.  When I wrote the comment you're referring to, quite a few years
> >> ago now, I thought that popular demand might force us to allow omitted
> >> aliases.  But the demand never materialized.  At this point it seems
> >> clear to me that there isn't really good reason to exceed the spec here.
> >> It just encourages people to write unportable SQL code.
> >
> > I suspect most users, like me, just roll their eyes, grumble, and put up
> > with it rather than complain.  It's a pain point, but tolerable enough
> > that no one bothers to demand a change.  Now that it's been done though,
> > allow me to add my voice in favor of it!
> 
> +1 to all of that.

+1, too.


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

[HACKERS] Range Partitioning behaviour - query

[Venkata B Nagothi]

Hi Hackers,

I have noticed the following behaviour in range partitioning which i felt
is not quite correct (i missed reporting this) -

I have tested by creating a date ranged partition.

I created the following table.

db03=# CREATE TABLE orders (
o_orderkey INTEGER,
o_custkey INTEGER,
o_orderstatus CHAR(1),
o_totalprice REAL,
o_orderdate DATE,
o_orderpriority CHAR(15),
o_clerk CHAR(15),
o_shippriority INTEGER,
o_comment VARCHAR(79)) partition by range (o_orderdate);
CREATE TABLE

Created the following partitioned tables :


db03=# CREATE TABLE orders_y1992
PARTITION OF orders FOR VALUES FROM ('1992-01-01') TO ('1992-12-31');
  CREATE TABLE

db03=# CREATE TABLE orders_y1993
PARTITION OF orders FOR VALUES FROM ('1993-01-01') TO ('*1993-12-31'*);
CREATE TABLE

db03=# CREATE TABLE orders_y1994
   PARTITION OF orders FOR VALUES FROM ('1994-01-01') TO ('1994-12-31');
CREATE TABLE


The rows with the date "1993-12-31" gets rejected as shown below -

db03=# copy orders from '/data/orders.csv' delimiter '|';
ERROR:  no partition of relation "orders" found for row
DETAIL:  Failing row contains (353, 8878, F, 273342, *1993-12-31*, 5-LOW
   , Clerk#02241, 0,  quiet ideas sleep. even instructions cajole
slyly. silently spe).
CONTEXT:  COPY orders, line 89: "353|8878|F|273342|*1993-12-31*|5-LOW
   |Clerk#02241|0| quiet ideas sleep. even instructions..."

I would want the partition "orders_y1993" to accept all the rows with the
date 1993-12-31.


To confirm this behaviour, I did another simple test with numbers -


I created two partitioned tables with range values from 1 to 5 and from 6
to 10 as shown below -


db03=# create table test_part ( col int) partition by range (col);
CREATE TABLE
db03=# create table test_part_5 partition of test_part for values from (1)
to (5);
CREATE TABLE
db03=# create table test_part_10 partition of test_part for values from (6)
to (10);
CREATE TABLE


When i try to insert value 5, it gets rejected as shown below

db03=# insert into test_part values (5);
ERROR:  no partition of relation "test_part" found for row
DETAIL:  Failing row contains (5).


The table partition "test_part_5" is not supposed to accept value 5 ?

Am i missing anything here ?

Regards,

Venkata B N
Database Consultant

Re: [HACKERS] Make subquery alias optional in FROM clause

[Robert Haas]

On Wed, Feb 22, 2017 at 10:33 PM, Nico Williams  wrote:
> On Wed, Feb 22, 2017 at 10:08:38AM -0500, Tom Lane wrote:
>> Bernd Helmle  writes:
>> >> From time to time, especially during migration projects from Oracle to
>> > PostgreSQL, i'm faced with people questioning why the alias in the FROM
>> > clause for subqueries in PostgreSQL is mandatory. The default answer
>> > here is, the SQL standard requires it.
>>
>> Indeed.  When I wrote the comment you're referring to, quite a few years
>> ago now, I thought that popular demand might force us to allow omitted
>> aliases.  But the demand never materialized.  At this point it seems
>> clear to me that there isn't really good reason to exceed the spec here.
>> It just encourages people to write unportable SQL code.
>
> I suspect most users, like me, just roll their eyes, grumble, and put up
> with it rather than complain.  It's a pain point, but tolerable enough
> that no one bothers to demand a change.  Now that it's been done though,
> allow me to add my voice in favor of it!

+1 to all of 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] PATCH: Batch/pipelining support for libpq

[Craig Ringer]

On 22 Feb. 2017 14:14, "Vaishnavi Prabakaran" 
wrote:

Thanks for reviewing the patch.
>

Thanks for picking it up! I've wanted to see this process for some time,
but just haven't had the bandwidth for it.

Re: [HACKERS] WARM and indirect indexes

[Bruce Momjian]

On Wed, Jan 11, 2017 at 04:38:10PM -0500, Robert Haas wrote:
> More broadly, I don't share Bruce's negativity about indirect indexes.
> My estimate of what needs to be done for them to be really useful is -
> I think - higher than your estimate of what needs to be done, but I
> think the concept is great.  I also think that some of the concepts -
> like allowing the tuple pointers to have widths other than 6 byes -
> could turn out to be a good foundation for global indexes in the
> future.  In fact, it might be considerably easier to make an indirect
> index span a partitioning hierarchy than it would be to do the same
> for a regular index.  But regardless of that, the feature is good for
> what it offers today.

I am worried that indirect indexes might have such limited usefulness
with a well-designed WARM feature that the syntax/feature would be
useless for 99% of users.   In talking to Alexander Korotkov, he
mentioned that indirect indexes could be used for global/cross-partition
indexes, and for index-organized tables (heap and index together in a
single file).  This would greatly expand the usefulness of indirect
indexes and would be exciting.

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

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


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

Re: [HACKERS] Logical replication existing data copy

[Petr Jelinek]

On 23/02/17 01:02, Erik Rijkers wrote:
> On 2017-02-22 18:13, Erik Rijkers wrote:
>> On 2017-02-22 14:48, Erik Rijkers wrote:
>>> On 2017-02-22 13:03, Petr Jelinek wrote:
>>>
 0001-Skip-unnecessary-snapshot-builds.patch
 0002-Don-t-use-on-disk-snapshots-for-snapshot-export-in-l.patch
 0003-Fix-xl_running_xacts-usage-in-snapshot-builder.patch
 0001-Use-asynchronous-connect-API-in-libpqwalreceiver.patch
 0002-Fix-after-trigger-execution-in-logical-replication.patch
 0003-Add-RENAME-support-for-PUBLICATIONs-and-SUBSCRIPTION.patch
 0001-Logical-replication-support-for-initial-data-copy-v5.patch
>>>
>>> It works well now, or at least my particular test case seems now solved.
>>
>> Cried victory too early, I'm afraid.
>>
> 
> I got into a 'hung' state while repeating  pgbench_derail2.sh.
> 
> Below is some state.  I notice that master pg_stat_replication.syaye is
> 'startup'.
> Maybe I should only start the test after that state has changed. Any of the
> other possible values (catchup, streaming) wuold be OK, I would think.
> 

I think that's known issue (see comment in tablesync.c about hanging
forever). I think I may have fixed it locally.

I will submit patch once I fixed the other snapshot issue (I managed to
reproduce it as well, although very rarely so it's rather hard to test).

Thanks for all this testing btw, I really appreciate it.

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


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

[HACKERS] objsubid vs subobjid

[Jim Nasby]

pg_get_object_address() currently returns a field called subobjid, while 
pg_depend calls that objsubid. I'm guessing that wasn't on purpose 
(especially because internally the function uses objsubid), and it'd be 
nice to fix it.


Attached does that, as well as updating the input naming on the other 
functions for consistency. I stopped short of changing the instances of 
subobjid in the C code to reduce backpatch issues, but maybe that should 
be done too...

--
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/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 05652e86c2..5233089d87 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -3096,13 +3096,13 @@ DESCR("get transaction Id and commit timestamp of 
latest transaction commit");
 DATA(insert OID = 3537 (  pg_describe_object   PGNSP PGUID 12 1 0 0 0 
f f f f t f s s 3 0 25 "26 26 23" _null_ _null_ _null_ _null_ _null_ 
pg_describe_object _null_ _null_ _null_ ));
 DESCR("get identification of SQL object");
 
-DATA(insert OID = 3839 (  pg_identify_object   PGNSP PGUID 12 1 0 0 0 
f f f f t f s s 3 0 2249 "26 26 23" "{26,26,23,25,25,25,25}" "{i,i,i,o,o,o,o}" 
"{classid,objid,subobjid,type,schema,name,identity}" _null_ _null_ 
pg_identify_object _null_ _null_ _null_ ));
+DATA(insert OID = 3839 (  pg_identify_object   PGNSP PGUID 12 1 0 0 0 
f f f f t f s s 3 0 2249 "26 26 23" "{26,26,23,25,25,25,25}" "{i,i,i,o,o,o,o}" 
"{classid,objid,objsubid,type,schema,name,identity}" _null_ _null_ 
pg_identify_object _null_ _null_ _null_ ));
 DESCR("get machine-parseable identification of SQL object");
 
-DATA(insert OID = 3382 (  pg_identify_object_as_address PGNSP PGUID 12 1 0 0 0 
f f f f t f s s 3 0 2249 "26 26 23" "{26,26,23,25,1009,1009}" "{i,i,i,o,o,o}" 
"{classid,objid,subobjid,type,object_names,object_args}" _null_ _null_ 
pg_identify_object_as_address _null_ _null_ _null_ ));
+DATA(insert OID = 3382 (  pg_identify_object_as_address PGNSP PGUID 12 1 0 0 0 
f f f f t f s s 3 0 2249 "26 26 23" "{26,26,23,25,1009,1009}" "{i,i,i,o,o,o}" 
"{classid,objid,objsubid,type,object_names,object_args}" _null_ _null_ 
pg_identify_object_as_address _null_ _null_ _null_ ));
 DESCR("get identification of SQL object for pg_get_object_address()");
 
-DATA(insert OID = 3954 (  pg_get_object_addressPGNSP PGUID 12 1 0 0 0 f f 
f f t f s s 3 0 2249 "25 1009 1009" "{25,1009,1009,26,26,23}" "{i,i,i,o,o,o}" 
"{type,name,args,classid,objid,subobjid}" _null_ _null_ pg_get_object_address 
_null_ _null_ _null_ ));
+DATA(insert OID = 3954 (  pg_get_object_addressPGNSP PGUID 12 1 0 0 0 f f 
f f t f s s 3 0 2249 "25 1009 1009" "{25,1009,1009,26,26,23}" "{i,i,i,o,o,o}" 
"{type,name,args,classid,objid,objsubid}" _null_ _null_ pg_get_object_address 
_null_ _null_ _null_ ));
 DESCR("get OID-based object address from name/args arrays");
 
 DATA(insert OID = 2079 (  pg_table_is_visible  PGNSP PGUID 12 10 0 0 0 
f f f f t f s s 1 0 16 "26" _null_ _null_ _null_ _null_ _null_ 
pg_table_is_visible _null_ _null_ _null_ ));
diff --git a/src/test/regress/expected/object_address.out 
b/src/test/regress/expected/object_address.out
index ec5ada97ad..08f9826c9e 100644
--- a/src/test/regress/expected/object_address.out
+++ b/src/test/regress/expected/object_address.out
@@ -401,14 +401,14 @@ WITH objects (type, name, args) AS (VALUES
('publication relation', '{addr_nsp, 
gentable}', '{addr_pub}'),
('subscription', '{addr_sub}', '{}')
 )
-SELECT (pg_identify_object(addr1.classid, addr1.objid, addr1.subobjid)).*,
+SELECT (pg_identify_object(addr1.classid, addr1.objid, addr1.objsubid)).*,
-- test roundtrip through pg_identify_object_as_address
-   ROW(pg_identify_object(addr1.classid, addr1.objid, addr1.subobjid)) =
-   ROW(pg_identify_object(addr2.classid, addr2.objid, addr2.subobjid))
+   ROW(pg_identify_object(addr1.classid, addr1.objid, addr1.objsubid)) =
+   ROW(pg_identify_object(addr2.classid, addr2.objid, addr2.objsubid))
  FROM objects, pg_get_object_address(type, name, args) addr1,
-   pg_identify_object_as_address(classid, objid, subobjid) 
ioa(typ,nms,args),
+   pg_identify_object_as_address(classid, objid, objsubid) 
ioa(typ,nms,args),
pg_get_object_address(typ, nms, ioa.args) as addr2
-   ORDER BY addr1.classid, addr1.objid, addr1.subobjid;
+   ORDER BY addr1.classid, addr1.objid, addr1.objsubid;
type|   schema   |   name|  
 identity   | ?column? 
 

Re: [HACKERS] Change in "policy" on dump ordering?

[Jim Nasby]

On 2/22/17 5:38 PM, Michael Banck wrote:

diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index ea643397ba..708a47f3cb 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -26,6 +26,9 @@ static const char *modulename = gettext_noop("sorter");
  * Sort priority for database object types.
  * Objects are sorted by type, and within a type by name.
  *
+ * Because materialized views can potentially reference system views,
+ * DO_REFRESH_MATVIEW should always be the last thing on the list.
+ *

I think this comment is overly specific: any materialized view that
references a view or table in a different schema (pg_catalog or not)
will likely not refresh on pg_restore AIUI, so singling out system views
doesn't look right to me.


This isn't a matter of excluded schemas. The problem is that if you had 
a matview that referenced a system view for something that was restored 
after DO_REFRESH_MATVIEW (such as subscriptions) then the view would be 
inaccurate after the restore.


Stephen, hopefully that answers your question as well. :)
--
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)


--
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 existing data copy

[Erik Rijkers]

On 2017-02-22 18:13, Erik Rijkers wrote:

On 2017-02-22 14:48, Erik Rijkers wrote:

On 2017-02-22 13:03, Petr Jelinek wrote:


0001-Skip-unnecessary-snapshot-builds.patch
0002-Don-t-use-on-disk-snapshots-for-snapshot-export-in-l.patch
0003-Fix-xl_running_xacts-usage-in-snapshot-builder.patch
0001-Use-asynchronous-connect-API-in-libpqwalreceiver.patch
0002-Fix-after-trigger-execution-in-logical-replication.patch
0003-Add-RENAME-support-for-PUBLICATIONs-and-SUBSCRIPTION.patch
0001-Logical-replication-support-for-initial-data-copy-v5.patch


It works well now, or at least my particular test case seems now 
solved.


Cried victory too early, I'm afraid.



I got into a 'hung' state while repeating  pgbench_derail2.sh.

Below is some state.  I notice that master pg_stat_replication.syaye is 
'startup'.
Maybe I should only start the test after that state has changed. Any of 
the

other possible values (catchup, streaming) wuold be OK, I would think.


$  ( dbactivity.sh ; echo "; table pg_subscription; table 
pg_subscription_rel;" ) | psql -qXp 6973
 now |  pid  | app   
  | state  | wt_evt | wt_evt_type | query_start 
| duration |  query

-+---+-+++-+-+--+--
 2017-02-23 00:37:57 | 31352 | logical replication worker 47435  
  | active | relation   | Lock| 
|  |
 2017-02-23 00:37:57 |   397 | psql  
  | active | BgWorkerShutdown   | IPC | 2017-02-23 00:22:14 
| 00:15:42 | drop subscription if exists sub1
 2017-02-23 00:37:57 | 31369 | logical replication worker 47435 sync 
47423 || LogicalSyncStateChange | IPC |  
   |  |
 2017-02-23 00:37:57 |   398 | logical replication worker 47435 sync 
47418 || transactionid  | Lock|  
   |  |

(4 rows)

 subdbid | subname | subowner | subenabled | subconninfo | subslotname | 
subpublications

-+-+--++-+-+-
   16384 | sub1|   10 | t  | port=6972   | sub1| 
{pub1}

(1 row)

 srsubid | srrelid | srsubstate |  srsublsn
-+-++
   47435 |   47423 | w  | 2/CB078260
   47435 |   47412 | r  |
   47435 |   47415 | r  |
   47435 |   47418 | c  | 2/CB06E158
(4 rows)


Replica (port 6973):

[bulldog aardvark] [local]:6973 (Thu) 00:52:47 [pid:5401] [testdb] # 
table pg_stat_subscription ;
 subid | subname |  pid  | relid | received_lsn |  
last_msg_send_time   | last_msg_receipt_time | 
latest_end_lsn |latest_end_time

---+-+---+---+--+---+---++---
 47435 | sub1| 31369 | 47423 |  | 2017-02-23 
00:20:45.758072+01 | 2017-02-23 00:20:45.758072+01 || 
2017-02-23 00:20:45.758072+01
 47435 | sub1|   398 | 47418 |  | 2017-02-23 
00:22:14.896471+01 | 2017-02-23 00:22:14.896471+01 || 
2017-02-23 00:22:14.896471+01
 47435 | sub1| 31352 |   | 2/CB06E158   |
   | 2017-02-23 00:20:47.034664+01 || 2017-02-23 
00:20:45.679245+01

(3 rows)


Master  (port 6972):

[bulldog aardvark] [local]:6972 (Thu) 00:48:27 [pid:5307] [testdb] # \x 
on \\ table pg_stat_replication ;

Expanded display is on.
-[ RECORD 1 ]+--
pid  | 399
usesysid | 10
usename  | aardvark
application_name | sub1_47435_sync_47418
client_addr  |
client_hostname  |
client_port  | -1
backend_start| 2017-02-23 00:22:14.902701+01
backend_xmin |
state| startup
sent_location|
write_location   |
flush_location   |
replay_location  |
sync_priority| 0
sync_state   | async
-[ RECORD 2 ]+--
pid  | 31371
usesysid | 10
usename  | aardvark
application_name | sub1_47435_sync_47423
client_addr  |
client_hostname  |
client_port  | -1
backend_start| 2017-02-23 00:20:45.762852+01
backend_xmin |
state| startup
sent_location|
write_location   |
flush_location   |
replay_location  |
sync_priority| 0
sync_state   | async



( above 'dbactivity.sh' is:

select
  rpad(now()::text,19) as now
, pid   as pid
, application_name  as app
, state as state
, wait_eventas wt_evt
, wait_event_type   as wt_evt_type
, date_trunc('second', query_start::timestamp)  as query_start
, substring((now() - query_start)::text, 1, position('.' in 

Re: [HACKERS] Packages: Again

[Bruce Momjian]

On Fri, Feb  3, 2017 at 07:59:26AM +0100, Pavel Stehule wrote:
> It should be documented and presented (who is read a documentation? :-))
> 
> It is not only PostgreSQL issue, same issue has to have any other databases.
> The Oracle architecture is very specific and often question is, how to map
> Oracle database to PostgreSQL. A common questions - how schema should be used,
> where schema should be used, where database should be used. What is practical
> limit of size of PostgreSQL catalogue.

I did write a blog entry on this topic:

http://momjian.us/main/blogs/pgblog/2012.html#April_23_2012

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

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


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

Re: [HACKERS] pg_dump does not refresh matviews from extensions

[Jim Nasby]

On 2/21/17 2:05 PM, Stephen Frost wrote:

As for $SUBJECT, I feel like it really depends, doesn't it?  If the
extension creates the matview w/ no data in it, and doesn't mark it as a
config table, should we really refresh it?  On the other hand, if the
extension creates the matview and either refreshes it, or something
else refreshes it later, then perhaps we should do so too, to get us
back to the same state.


I didn't think to test marking the matview as dumpable. If I do that 
then a refresh item does get created, and it's actually based on whether 
the view contains any data. We should at least document that.


Now that I know that, I guess I'm kinda on the fence about doing it 
automatically, because AFAIK there'd be no way to override that 
automatic behavior. I can't really conceive of any reason you wouldn't 
want the refresh, but since it's not happening today...

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


--
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] Change in "policy" on dump ordering?

[Michael Banck]

Hi,

On Wed, Feb 22, 2017 at 05:24:49PM -0600, Jim Nasby wrote:
> On 2/22/17 12:29 PM, Peter Eisentraut wrote:
> >On 2/22/17 10:14, Jim Nasby wrote:
> >>CREATE MATERIALIZED VIEW tmv AS SELECT * FROM pg_subscription;
> >>SELECT 0
> >>
> >>IOW, you can create matviews that depend on any other
> >>table/view/matview, but right now if the matview includes certain items
> >>it will mysteriously end up empty post-restore.
> >
> >Yes, by that logic matview refresh should always be last.

Glad to hear - I vaguely remember this coming up in a different thread
some time ago, and I though you (Peter) had reservations about moving
things past after the ACL step, but I couldn't find this thread now
anymore, only
https://www.postgresql.org/message-id/11166.1424357659%40sss.pgh.pa.us

> Patches for head attached.

Yay.

> diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
> index ea643397ba..708a47f3cb 100644
> --- a/src/bin/pg_dump/pg_dump_sort.c
> +++ b/src/bin/pg_dump/pg_dump_sort.c
> @@ -26,6 +26,9 @@ static const char *modulename = gettext_noop("sorter");
>   * Sort priority for database object types.
>   * Objects are sorted by type, and within a type by name.
>   *
> + * Because materialized views can potentially reference system views,
> + * DO_REFRESH_MATVIEW should always be the last thing on the list.
> + *

I think this comment is overly specific: any materialized view that
references a view or table in a different schema (pg_catalog or not)
will likely not refresh on pg_restore AIUI, so singling out system views
doesn't look right to me.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] Change in "policy" on dump ordering?

[Stephen Frost]

Jim,

* Jim Nasby (jim.na...@bluetreble.com) wrote:
> On 2/22/17 12:29 PM, Peter Eisentraut wrote:
> >On 2/22/17 10:14, Jim Nasby wrote:
> >>CREATE MATERIALIZED VIEW tmv AS SELECT * FROM pg_subscription;
> >>SELECT 0
> >>
> >>IOW, you can create matviews that depend on any other
> >>table/view/matview, but right now if the matview includes certain items
> >>it will mysteriously end up empty post-restore.
> >
> >Yes, by that logic matview refresh should always be last.
> 
> Patches for head attached.
> 
> RLS was the first item added after DO_REFRESH_MATVIEW, which was
> added in 9.5. So if we want to treat this as a bug, they'd need to
> be patched as well, which is a simple matter of swapping 33 and 34.

Can you clarify what misbehavior there is with RLS that would warrent
this being a bug..?  I did consider where in the dump I thought policies
should go, though I may certainly have overlooked something.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Change in "policy" on dump ordering?

[Jim Nasby]

On 2/22/17 12:29 PM, Peter Eisentraut wrote:

On 2/22/17 10:14, Jim Nasby wrote:

CREATE MATERIALIZED VIEW tmv AS SELECT * FROM pg_subscription;
SELECT 0

IOW, you can create matviews that depend on any other
table/view/matview, but right now if the matview includes certain items
it will mysteriously end up empty post-restore.


Yes, by that logic matview refresh should always be last.


Patches for head attached.

RLS was the first item added after DO_REFRESH_MATVIEW, which was added 
in 9.5. So if we want to treat this as a bug, they'd need to be patched 
as well, which is a simple matter of swapping 33 and 34.

--
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/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index ea643397ba..708a47f3cb 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -26,6 +26,9 @@ static const char *modulename = gettext_noop("sorter");
  * Sort priority for database object types.
  * Objects are sorted by type, and within a type by name.
  *
+ * Because materialized views can potentially reference system views,
+ * DO_REFRESH_MATVIEW should always be the last thing on the list.
+ *
  * NOTE: object-type priorities must match the section assignments made in
  * pg_dump.c; that is, PRE_DATA objects must sort before DO_PRE_DATA_BOUNDARY,
  * POST_DATA objects must sort after DO_POST_DATA_BOUNDARY, and DATA objects
@@ -70,11 +73,11 @@ static const int dbObjectTypePriority[] =
22, /* 
DO_PRE_DATA_BOUNDARY */
26, /* 
DO_POST_DATA_BOUNDARY */
33, /* 
DO_EVENT_TRIGGER */
-   34, /* 
DO_REFRESH_MATVIEW */
-   35, /* DO_POLICY */
-   36, /* 
DO_PUBLICATION */
-   37, /* 
DO_PUBLICATION_REL */
-   38  /* 
DO_SUBSCRIPTION */
+   38, /* 
DO_REFRESH_MATVIEW */
+   34, /* DO_POLICY */
+   35, /* 
DO_PUBLICATION */
+   36, /* 
DO_PUBLICATION_REL */
+   37  /* 
DO_SUBSCRIPTION */
 };
 
 static DumpId preDataBoundId;

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

[HACKERS] Index usage for elem-contained-by-const-range clauses

[Pritam Baral]

The topic has been previously discussed[0] on the -performance mailing list,
about four years ago.

In that thread, Tom suggested[0] the planner could be made to "expand
"intcol <@
'x,y'::int4range" into "intcol between x and y", using something similar
to the
index LIKE optimization (ie, the "special operator" stuff in indxpath.c)".

This patch tries to do exactly that. It's not tied to any specific datatype,
and has been tested with both builtin types and custom range types. Most
of the
checking for proper datatypes, operators, and btree index happens before
this
code, so I haven't run into any issues yet in my testing. But I'm not
familiar
enough with the internals to be able to confidently say it can handle
all cases
just yet.

[0]:
https://www.postgresql.org/message-id/flat/9860.1364013108%40sss.pgh.pa.us#9860.1364013...@sss.pgh.pa.us

-- 
#!/usr/bin/env regards
Chhatoi Pritam Baral
diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index 2952bfb7c2..84dfd8362a 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -30,21 +30,23 @@
 #include "optimizer/pathnode.h"
 #include "optimizer/paths.h"
 #include "optimizer/predtest.h"
 #include "optimizer/prep.h"
 #include "optimizer/restrictinfo.h"
 #include "optimizer/var.h"
 #include "utils/builtins.h"
 #include "utils/bytea.h"
 #include "utils/lsyscache.h"
 #include "utils/pg_locale.h"
+#include "utils/rangetypes.h"
 #include "utils/selfuncs.h"
+#include "utils/typcache.h"
 
 
 #define IsBooleanOpfamily(opfamily) \
 	((opfamily) == BOOL_BTREE_FAM_OID || (opfamily) == BOOL_HASH_FAM_OID)
 
 #define IndexCollMatchesExprColl(idxcollation, exprcollation) \
 	((idxcollation) == InvalidOid || (idxcollation) == (exprcollation))
 
 /* Whether we are looking for plain indexscan, bitmap scan, or either */
 typedef enum
@@ -180,20 +182,22 @@ static Expr *expand_boolean_index_clause(Node *clause, int indexcol,
 			IndexOptInfo *index);
 static List *expand_indexqual_opclause(RestrictInfo *rinfo,
 		  Oid opfamily, Oid idxcollation);
 static RestrictInfo *expand_indexqual_rowcompare(RestrictInfo *rinfo,
 			IndexOptInfo *index,
 			int indexcol);
 static List *prefix_quals(Node *leftop, Oid opfamily, Oid collation,
 			 Const *prefix, Pattern_Prefix_Status pstatus);
 static List *network_prefix_quals(Node *leftop, Oid expr_op, Oid opfamily,
 	 Datum rightop);
+static List *range_elem_contained_quals(Node *leftop, Oid expr_op, Oid opfamily,
+	 Datum rightop);
 static Datum string_to_datum(const char *str, Oid datatype);
 static Const *string_to_const(const char *str, Oid datatype);
 
 
 /*
  * create_index_paths()
  *	  Generate all interesting index paths for the given relation.
  *	  Candidate paths are added to the rel's pathlist (using add_path).
  *
  * To be considered for an index scan, an index must match one or more
@@ -3286,20 +3290,23 @@ match_special_index_operator(Expr *clause, Oid opfamily, Oid idxcollation,
 			/* the right-hand const is type text for all of these */
 			pstatus = pattern_fixed_prefix(patt, Pattern_Type_Regex_IC, expr_coll,
 		   , NULL);
 			isIndexable = (pstatus != Pattern_Prefix_None);
 			break;
 
 		case OID_INET_SUB_OP:
 		case OID_INET_SUBEQ_OP:
 			isIndexable = true;
 			break;
+		case OID_RANGE_ELEM_CONTAINED_OP:
+			isIndexable = true;
+			break;
 	}
 
 	if (prefix)
 	{
 		pfree(DatumGetPointer(prefix->constvalue));
 		pfree(prefix);
 	}
 
 	/* done if the expression doesn't look indexable */
 	if (!isIndexable)
@@ -3614,20 +3621,27 @@ expand_indexqual_opclause(RestrictInfo *rinfo, Oid opfamily, Oid idxcollation)
 			break;
 
 		case OID_INET_SUB_OP:
 		case OID_INET_SUBEQ_OP:
 			if (!op_in_opfamily(expr_op, opfamily))
 			{
 return network_prefix_quals(leftop, expr_op, opfamily,
 			patt->constvalue);
 			}
 			break;
+		case OID_RANGE_ELEM_CONTAINED_OP:
+			if (!op_in_opfamily(expr_op, opfamily))
+			{
+return range_elem_contained_quals(leftop, expr_op, opfamily,
+			patt->constvalue);
+			}
+			break;
 	}
 
 	/* Default case: just make a list of the unmodified indexqual */
 	return list_make1(rinfo);
 }
 
 /*
  * expand_indexqual_rowcompare --- expand a single indexqual condition
  *		that is a RowCompareExpr
  *
@@ -4096,20 +4110,124 @@ network_prefix_quals(Node *leftop, Oid expr_op, Oid opfamily, Datum rightop)
 			InvalidOid, /* not collatable */
 			-1, opr2right,
 			false, false),
 		 InvalidOid, InvalidOid);
 	result = lappend(result, make_simple_restrictinfo(expr));
 
 	return result;
 }
 
 /*
+ * Given an element leftop and a range rightop, and an elem contained-by range
+ * operator, generate suitable indexqual condition(s).
+ */
+static List *
+range_elem_contained_quals(Node *leftop, Datum rightop)
+{
+	Oiddatatype;
+	Oidopfamily;
+	Oidopr1oid;
+	Oidopr2oid;
+	List			*result = NIL;
+	Expr			*expr;
+	RangeType		*range;
+	

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Wed, Feb 22, 2017 at 5:59 PM, Tom Lane  wrote:

> Ah, I see why *that* wants to know about it ... I think.  I suppose you're
> arguing that variable expansion shouldn't be able to insert, say, an \else
> in a non-active branch?  Maybe, but if it can insert an \else in an active
> branch, then why not non-active too?  Seems a bit inconsistent.
>

The major reason was avoiding situations like what Daniel showed: where
value of a variable that is meaningless/undefined in the current
false-block context gets expanded anyway, and thus code inside a false
block has effects outside of that block. Granted, his example was
contrived. I'm open to removing that feature and seeing what breaks in the
test cases.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

Corey Huinker  writes:
> After some research, GetVariable is called by psql_get_variable, which is
> one of the callback functions passed to psql_scan_create(). So passing a
> state variable around probably isn't going to work and PsqlFileState now
> looks like the best option.

Ah, I see why *that* wants to know about it ... I think.  I suppose you're
arguing that variable expansion shouldn't be able to insert, say, an \else
in a non-active branch?  Maybe, but if it can insert an \else in an active
branch, then why not non-active too?  Seems a bit inconsistent.

Anyway, what this seems to point up is that maybe we should've allowed
for a passthrough "void *" argument to the psqlscan callback functions.
There wasn't one in the original design but it's a fairly standard part
of our usual approach to callback functions, so it's hard to see an
objection to adding one now.

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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Wed, Feb 22, 2017 at 5:11 PM, Corey Huinker 
wrote:

> Dunno, that sounds a lot like an "if the only tool I have is a hammer,
>> then this must be a nail" argument.
>
>
> More of a "don't rock the boat more than absolutely necessary", but
> knowing that adding another global struct might be welcomed is good to know.
>
>

After some research, GetVariable is called by psql_get_variable, which is
one of the callback functions passed to psql_scan_create(). So passing a
state variable around probably isn't going to work and PsqlFileState now
looks like the best option.

Re: [HACKERS] Replication vs. float timestamps is a disaster

[Tom Lane]

Andres Freund  writes:
> On 2017-02-22 08:43:28 -0500, Tom Lane wrote:
>> (To be concrete, I'm suggesting dropping --disable-integer-datetimes
>> in HEAD, and just agreeing that in the back branches, use of replication
>> protocol with float-timestamp servers is not supported and we're not
>> going to bother looking for related bugs there.  Given the lack of field
>> complaints, I do not believe anyone cares.)

> I think we should just fix it in the back branches, and drop the float
> timestamp code in 10 or 11.  I.e. 1) and then 4).

I'm not personally interested in touching this issue in the back branches,
but if you want to spend time on it, I surely won't stand in your way.

What I *am* willing to spend time on is removing float-timestamp code
in HEAD.  I've not yet heard anybody speak against doing that (or at
least, nothing I interpreted as a vote against it).  If I've not heard
any complaints by tomorrow, I'll get started on that.

I envision the following work plan:

* Reject --disable-integer-datetimes in configure.  To avoid breaking
existing build scripts unnecessarily, --enable-integer-datetimes will
still be accepted.

* Convert HAVE_INT64_TIMESTAMP to a fixed, always-defined symbol.
(We shouldn't remove it entirely because that would break third-party
code that might be testing it.  Perhaps shove it in pg_config_manual.h.)

* Possibly remove the enableIntTimes field from pg_control as well
as associated logic in places like pg_controldata.  There might be some
argument for keeping this, though ... anyone have an opinion?  pg_upgrade,
at least, would need a bespoke test instead.

* Remove appropriate user documentation.

* Remove all core-code tests for HAVE_INT64_TIMESTAMP, along with the
negative sides of those #if tests.

* Change the places in the replication code that declare timestamp
variables to declare them as TimestampTz not int64, and adjust nearby
comments accordingly.  (This will just be code beautification at this
point, but it still seems like a good idea.)

regards, tom lane


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

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

[Bruce Momjian]

On Fri, Feb  3, 2017 at 05:21:28AM -0500, Stephen Frost wrote:
> > Also googling for pg_wal, I'm finding food for thought like this
> > IBM technote:
> > http://www-01.ibm.com/support/docview.wss?uid=isg3T1015637
> > which recommends to 
> > "Remove all files under /var/lib/pgsql/9.0/data/pg_wal/"
> > and also calls that directory the "write-ahead log directory"
> > which is quite confusing because apparently it's the destination of
> > their archive command.
> 
> It's certainly unfortunate that people have thought that they can create
> arbitrary directories under the PG data directory.  That's never going
> to be safe, witness that we've created new directories under PGDATA in
> the last few releases and I don't see any reason why that would change
> moving forward.  Perhaps we should check for the existance of such a
> directory during pg_upgrade and throw an error, and we should go back
> and do the same for other directories which have been added over
> releases, but I'm not sure I can see an argument for doing much more
> than that.

Actually, pg_upgrade already checks for some odd directories stored
inside of PGDATA:

WARNING:  new data directory should not be inside the
old data directory, e.g. %s\n", old_cluster_pgdata);

WARNING:  user-defined tablespace locations should
not be inside the data directory, e.g. %s\n", old_tablespace_dir);

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

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


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

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> Dunno, that sounds a lot like an "if the only tool I have is a hammer,
> then this must be a nail" argument.


More of a "don't rock the boat more than absolutely necessary", but knowing
that adding another global struct might be welcomed is good to know.


> reasonable interpretation of what it's for.  Inventing a PsqlFileState or
> similar struct might be a good idea to help pull some of that cruft out of
> pset and get it back to having a reasonably clearly defined purpose of
> holding "current settings".
>

+1

command was ignored" warning messages).  I'm failing to follow why
>
GetVariable would need to care.
>

It took me a second to find the post, written by Daniel Verite on Jan 26,
quoting:

> Revised patch

A comment about control flow and variables:
in branches that are not taken, variables are expanded
nonetheless, in a way that can be surprising.
Case in point:

\set var 'ab''cd'
-- select :var;
\if false
  select :var ;
\else
  select 1;
\endif

The 2nd reference to :var has a quoting hazard, but since it's within
an "\if false" branch, at a glance it seems like this script might work.
In fact it barfs with:
  psql:script.sql:0: found EOF before closing \endif(s)

AFAICS what happens is that :var gets injected and starts a
runaway string, so that as far as the parser is concerned
the \else ..\endif block slips into the untaken branch, as a part of
that unfinished string.



So that was the reasoning behind requiring GetVariable to know whether or
not the statement was being ignored.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

Corey Huinker  writes:
> On Wed, Feb 22, 2017 at 4:00 PM, Tom Lane  wrote:
>> One thing I'm wondering is why the "active_branch" bool is in "pset"
>> and not in the conditional stack.  That seems, at best, pretty grotty.
>> _psqlSettings is meant for reasonably persistent state.

> With the if-stack moved to MainLoop(), nearly all the active_branch checks
> could be against a variable that lives in MainLoop(), with two big
> exceptions: GetVariable() needs to know when NOT to expand a variable
> because it's in a false-block, and get_prompt will need to know when it's
> in a false block for printing the '@' prompt hint or equivalent, and pset
> is the only global around I know of to do that.

Dunno, that sounds a lot like an "if the only tool I have is a hammer,
then this must be a nail" argument.  pset should not accrete every single
global variable in psql just because it's there.  Actually, there's a
pretty fair amount of stuff in it already that should not be there by any
reasonable interpretation of what it's for.  Inventing a PsqlFileState or
similar struct might be a good idea to help pull some of that cruft out of
pset and get it back to having a reasonably clearly defined purpose of
holding "current settings".

So I think that if you're intent on this being a global variable, it might
as well be a standalone global variable.  I was wondering more about
whether we shouldn't be passing the condition-stack top pointer around
to places that need to know about conditional execution.  get_prompt would
be one if we decide that the prompt might need to reflect this (a question
that still seems undecided to me --- I think we'd be better off with "this
command was ignored" warning messages).  I'm failing to follow why
GetVariable would need to care.

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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Wed, Feb 22, 2017 at 4:00 PM, Tom Lane  wrote:

> Corey Huinker  writes:
> >> but if you think that it should be somewhere else, please advise Corey
> >> about where to put it.
>
> > Just a reminder that I'm standing by for advice.
>
> Sorry, I'd lost track of this thread.
>

Judging by the volume of the 50-or-so active threads on this list, I
figured you were busy.


> > The issue at hand is whether the if-state should be a part of the
> > PsqlScanState, or if it should be a separate state variable owned by
> > MainLoop() and passed to HandleSlashCommands(), ... or some other
> solution.
>
> My reaction to putting it in PsqlScanState is pretty much "over my dead
> body".  That's just trashing any pretense of an arms-length separation
> between psql and the lexer proper.  We only recently got done sweating
> blood to create that separation, why would we toss it away already?
>

Good to know that history.


>
> If you're concerned about the notational overhead of passing two arguments
> rather than one, my druthers would be to invent a new struct type, perhaps
> named along the lines of PsqlFileState or PsqlInputState, and pass that
> around.  One of its fields would be a PsqlScanState pointer, the rest
> would be for if-state and whatever else we think we need in per-input-file
> state.
>

I wasn't, my reviewer was. I thought about the super-state structure like
you described, and decided I was happy with two state params.


> However, that way is doubling down on the assumption that the if-state is
> exactly one-to-one with input file levels, isn't it?  We might be better
> off to just live with the separate arguments to preserve some flexibility
> in that regard.  The v12 patch doesn't look that awful in terms of what
> it's adding to argument lists.
>

The rationale for tying if-state to file levels is not so much of anything
if-then related, but rather of the mess we'd create for whatever poor soul
decided to undertake \while loops down the road, and the difficulties
they'd have trying to unwind/rewind jump points in file(s)...keeping it
inside one file makes things simpler for the coding and the coder.


> One thing I'm wondering is why the "active_branch" bool is in "pset"
> and not in the conditional stack.  That seems, at best, pretty grotty.
> _psqlSettings is meant for reasonably persistent state.
>

With the if-stack moved to MainLoop(), nearly all the active_branch checks
could be against a variable that lives in MainLoop(), with two big
exceptions: GetVariable() needs to know when NOT to expand a variable
because it's in a false-block, and get_prompt will need to know when it's
in a false block for printing the '@' prompt hint or equivalent, and pset
is the only global around I know of to do that. I can move nearly all the
is-this-branch-active checks to structures inside of MainLoop(), and that
does strike me as cleaner, but there will still have to be that gross bit
where we update pset.active_branch so that the prompt and GetVariable() are
clued in.

Re: [HACKERS] Should we cacheline align PGXACT?

[Alvaro Herrera]

I wonder if this "perf c2c" tool with Linux 4.10 might be useful in
studying this problem.
https://joemario.github.io/blog/2016/09/01/c2c-blog/

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


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

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

Corey Huinker  writes:
>> but if you think that it should be somewhere else, please advise Corey
>> about where to put it.

> Just a reminder that I'm standing by for advice.

Sorry, I'd lost track of this thread.

> The issue at hand is whether the if-state should be a part of the
> PsqlScanState, or if it should be a separate state variable owned by
> MainLoop() and passed to HandleSlashCommands(), ... or some other solution.

My reaction to putting it in PsqlScanState is pretty much "over my dead
body".  That's just trashing any pretense of an arms-length separation
between psql and the lexer proper.  We only recently got done sweating
blood to create that separation, why would we toss it away already?

If you're concerned about the notational overhead of passing two arguments
rather than one, my druthers would be to invent a new struct type, perhaps
named along the lines of PsqlFileState or PsqlInputState, and pass that
around.  One of its fields would be a PsqlScanState pointer, the rest
would be for if-state and whatever else we think we need in per-input-file
state.

However, that way is doubling down on the assumption that the if-state is
exactly one-to-one with input file levels, isn't it?  We might be better
off to just live with the separate arguments to preserve some flexibility
in that regard.  The v12 patch doesn't look that awful in terms of what
it's adding to argument lists.

One thing I'm wondering is why the "active_branch" bool is in "pset"
and not in the conditional stack.  That seems, at best, pretty grotty.
_psqlSettings is meant for reasonably persistent state.

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] Documentation improvements for partitioning

[Peter Geoghegan]

On Tue, Feb 21, 2017 at 6:14 PM, Robert Haas  wrote:
> On Tue, Feb 21, 2017 at 2:03 AM, Bruce Momjian  wrote:
>> I have to admit my reaction was similar to Simon's, meaning that the
>> lack of docs is a problem, and that the limitations are kind of a
>> surprise, and I wonder what other surprises there are.
>
> Did you read my message upthread pointing out that the initial commit
> contained hundreds of lines of documentation?  I agree that it would
> be bad if table partitioning got committed with no documentation, but
> that did not happen.
>
>> I am thinking this is a result of small teams, often from the same
>> company, working on a features in isolation and then making them public.
>> It is often not clear what decisions were made and why.
>
> That also did not happen, or at least certainly not with this patch.
> All of the discussion was public and on the mailing list.

FWIW, I agree that some of what has been claimed about what
contributors failed to do with this patch is exaggerated, and not in a
way that I'd understand as hyperbole that drives home a deeper point.

I'm not the slightest bit surprised at the limitations that this
feature has, even if Bruce and Simon are. The documentation needs
work, and perhaps the feature itself needs a small tweak here or
there. Just not to a particularly notable degree, given the point we
are in in the release cycle.

-- 
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] Temporal query processing with range types

[Peter Eisentraut]

On 2/16/17 07:41, Robert Haas wrote:
> Also, it sounds like all of this is intended to work with ranges that
> are stored in different columns rather than with PostgreSQL's built-in
> range types.

Yeah, that should certainly be changed.

-- 
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] Change in "policy" on dump ordering?

[Peter Eisentraut]

On 2/22/17 10:14, Jim Nasby wrote:
> CREATE MATERIALIZED VIEW tmv AS SELECT * FROM pg_subscription;
> SELECT 0
> 
> IOW, you can create matviews that depend on any other 
> table/view/matview, but right now if the matview includes certain items 
> it will mysteriously end up empty post-restore.

Yes, by that logic matview refresh should always be last.

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

[HACKERS] Note about bug #14563

[Tom Lane]

I looked into the report in bug #14563 about pg_trgm giving wrong
answers for regexp searches.  There is a whole lot of complex mechanism
in trgm_regexp.c for extracting trigram patterns out of regexps, and
so I was afraid that it might be a very subtle bug, but actually the
problem seems to be in an apparently-trivial step.  That's the part in
selectColorTrigrams() where we discard trigrams if there are too many,
fixing the graph by merging the states linked by a discarded trigram.
We are not supposed to merge initial and final states, because if we
did the graph would be broken, and the code that checks for that
looks simple enough ... but it's wrong.  Consider

   C   C
Sinit ---> Sx ---> Sfin

where the two arcs depicted are labeled by the same color trigram.
We will examine each arc and decide that it doesn't link the init
and fin states, and thereby decide that it's okay to remove C.
But removing the first arc merges Sx into Sinit, and removing the
second arc merges Sfin into Sx and thereby into Sinit, and we've
blown it.

I think a reasonable fix for this is to add a "tentative parent" field
into struct TrgmState that is kept NULL except during this specific logic.
In the loop looking for disallowed merges, set "tentative parent" to
indicate a merge we would make if successful, and chase up tentative as
well as real parent links.  Then reset all the tentative links before
moving on, whether or not the merge is allowed.  In the example above,
we'd set Sx's tentative parent to Sinit, then while looking at the second
arc we would chase up from Sx to Sinit and realize we can't merge.

It seems like a good idea, as well, for either mergeStates() or its
sole caller to have an Assert, if not indeed a full-fledged runtime
check, that we aren't merging init and final states.

Also, I notice that the "children" state lists are useless and could just
be dropped.  The only thing they're used for at all is reducing the length
of the parent-link chains, but I see no point in that considering the code
isn't relying on those chains to be length 1.  It's very unlikely that we
save enough cycles by having shorter chains to pay for the overhead of
allocating and maintaining the children lists.

regards, tom lane


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

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

[Mithun Cy]

Hi all thanks,
I have tried to fix all of the comments given above with some more
code cleanups.

On Wed, Feb 22, 2017 at 6:28 AM, Robert Haas  wrote:
> I think it's OK to treat that as something of a corner case.  There's
> nothing to keep you from doing that today: just use pg_buffercache to
> dump a list of blocks on one server, and then pass those blocks to
> pg_prewarm on another server.  The point of this patch, AIUI, is to
> automate a particularly common case of that, which is to dump before
> shutdown and load upon startup.  It doesn't preclude other things that
> people want to do.
>
> I suppose we could have an SQL-callable function that does an
> immediate dump (without starting a worker).  That wouldn't hurt
> anything, and might be useful in a case like the one you mention.

In the latest patch, I have moved the things back as in old ways there
will be one bgworker "auto pg_prewarm" which automatically records
information about blocks which were present in buffer pool before
server shutdown and then prewarm the buffer pool upon server restart
with those blocks. I have reverted back the code which helped us to
launch the stopped "auto pg_prewarm" bgworker. The reason I introduced
a launcher SQL utility function is the running auto pg_prewarm can be
stopped by the user by setting dump_interval to -1. So if the user
wants to restart the stopped auto pg_prewarm(this time dump only to
prewarm on next restart), he can use that utility. The user can launch
the auto pg_prewarm to dump periodically while the server is still
running. If that was not the concern I think I misunderstood the
comments and overdid the design. So as a first patch I will keep the
things simple. Also,
using a separate process for prewarm and dump activity was a bad
design hence reverted back same. The auto pg_prewarm can only be
launched by preloading the library. And I can add additional
utilities, once we can formalize what is really needed out of it.

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


pg_auto_prewarm_05.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] pg_monitor role

[Stephen Frost]

Dave, all,

* Stephen Frost (sfr...@snowman.net) wrote:
> * Dave Page (dp...@pgadmin.org) wrote:
> > What modules should be included?
> 
> On a quick review of all of the modules, excluding those that are just
> testing or examples or which can already be used by non-superusers by
> default, and excluding those which can be used to trivially gain
> superuser access (adminpack and pageinspect), I came up with:
> 
> pg_buffercache
> pg_freespacemap
> pgrowlocks
> pg_stat_statements
> pgstattuple
> pg_visibility
> 
> Reviewing this list, they all seem like things a monitoring user could
> have a use for and none of them allow direct access to table data from
> what I could tell on a quick review.  Obviously, a more detailed review
> of each should be done to make sure I didn't miss something.

Also, not everything in those modules should be allowed to the
pg_monitor role, I don't think.  For example, I don't think pg_monitor
should be given access to pg_truncate_visibility_map(), particularly
since there's zero ACL checks inside of pg_visibility, meaning that
having EXECUTE rights on that function would allow you to truncate the
visibility map of anything in the database, from what I can tell in a
quick review.

The other functions look like they would be 'ok' for the pg_monitor user
to have access to though.  To be clear, I don't think it would make
sense to add ACL checks into those other functions either, unless we
came up with a new type of ACL for just this type of meta-data access.
I'm not really a fan of that either though, because you would then have
to figure out how to give that access to every object in the system,
which isn't something we handle very well today.

Perhaps when we get around to creating default roles that have other
privileges by default (like a 'pg_read_only' role that automatically has
SELECT rights to every table in the database...) we could have a role
like "pg_read_metadata" that automatically had that right everywhere,
but I don't think we need to have that before adding pg_monitor.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] pg_monitor role

[Stephen Frost]

Dave,

* Dave Page (dp...@pgadmin.org) wrote:
> On Wed, Feb 22, 2017 at 4:52 PM, Stephen Frost  wrote:
> >> > > And what about the diagnostic tools such as pageinspect and 
> >> > > pgstattuple?
> >> >
> >> > I think external/contrib modules should not be included. To install
> >> > them you need admin privileges anyway, so you can easily grant
> >> > whatever usage privileges you want at that time.
> >>
> >> I'll start by saying "why not cover contrib"?
> >
> > +1.
> 
> I'm not convinced we should include it, for the reason I gave above.
> However, I don't feel that strongly.
> 
> What modules should be included?

On a quick review of all of the modules, excluding those that are just
testing or examples or which can already be used by non-superusers by
default, and excluding those which can be used to trivially gain
superuser access (adminpack and pageinspect), I came up with:

pg_buffercache
pg_freespacemap
pgrowlocks
pg_stat_statements
pgstattuple
pg_visibility

Reviewing this list, they all seem like things a monitoring user could
have a use for and none of them allow direct access to table data from
what I could tell on a quick review.  Obviously, a more detailed review
of each should be done to make sure I didn't miss something.

One interesting thing that comes up from this list is that there's a
number of things which are "look at something about a row" or "look at
something about a block" (pg_freespacemap, pgrowlocks, pgstattuple,
pg_visibility all fall into those, and to some extent pg_buffercache
too).  I'm tempted to suggest that we have a role which covers that
theme (and is then GRANT'd to pg_monitor).

> >> pgstattuple can be discussed. It doesn't leak anything dangerous. But it
> >> does have views that are quite expensive.
> 
> I don't think expense should be a concern. It's not like a regular
> user cannot run something expensive already, so why stop a user
> specifically setup to monitor something?

I tend to agree with this.

> > I do see two issues to be addressed with such a role:
> >
> > #1- We shouldn't just shove everything into one role.  Where
> > functionality can't be GRANT'd independently of the role, we should have
> > another default role.  For example, "Read all GUCs" is not something
> > that can currently be GRANT'd.  I'm sure there are cases where $admin
> > wants a given role to be able to read all GUCs, but not execute
> > pg_ls_logdir(), for example.  If we start writing code that refers
> > explicitly to pg_monitor then we will end up in an "all-or-nothing" kind
> > of situation (not unlike the superuser case) instead of allowing users a
> > fine-grained set of options.
> 
> I'm fine with having pg_read_all_gucs - it's a trivial change.  I
> wouldn't want us to go too far and end up with separate roles for
> everything under the sun though.

I agree with you there- having too many default roles would lead to
things getting messy, without there really being a need for it.  Users
can always create their own roles for the specific set of capabilities
that they want to provide.  The main thing I want to avoid is having a
situation where a user *can't* create a role that has only a subset of
what "pg_monitor" has because there's some code somewhere that
explicitly allows the "pg_monitor" role to do something.

> > #2- We need to define very carefully, up-front, how we will deal with
> > new privileges/capabilities/features down the road.  A very specific
> > default role like 'pg_read_all_gucs' is quite clear about what's allowed
> > by it and I don't think we'd get any push-back from adding new GUCs that
> > such a default role could read, but some new view pg_stat_X view that
> > would be really useful to monitoring tools might also allow more access
> > than the pg_monitor has or that some admins would be comfortable with-
> > how do we handle such a case?  I see a few options:
> >
> >   - Define up-front that pg_monitor has rights on all pg_stat_X views,
> > which then requires we provide a definition and clarity on what
> > "pg_stat_X" *is* and provides.  We can then later add such views and
> > GRANT access to them to pg_monitor.
> >
> >   - Create new versions of pg_monitor in the future that cover ever
> > increasing sets of privileges ("pg_monitor_with_pg_stat_X" or
> > "pg_monitor_v11" for PG11 or something).
> 
> I prefer the first option. In my experience, users don't much care
> about the rights their monitoring user has, as long as it's not full
> superuser. The only case where I think there are legitimate concerns
> are where you can read arbitrary data (I do not consider query strings
> to be in that class for the record). That said, if we ever do add
> something like that then there's nothing stopping us from explicitly
> documenting that it's excluded from pg_monitor for that reason, and if
> desired the user can grant on it as needed.
> 
> Using a scheme like that would also mean that the user 

Re: [HACKERS] Replication vs. float timestamps is a disaster

[David Fetter]

On Mon, Feb 20, 2017 at 11:58:12AM +0100, Petr Jelinek wrote:
> On 20/02/17 08:03, Andres Freund wrote:
> > On 2017-02-19 10:49:29 -0500, Tom Lane wrote:
> >> Robert Haas  writes:
> >>> On Sun, Feb 19, 2017 at 3:31 AM, Tom Lane  wrote:
>  Thoughts?  Should we double down on trying to make this work according
>  to the "all integer timestamps" protocol specs, or cut our losses and
>  change the specs?
> >>
> >>> I vote for doubling down.  It's bad enough that we have so many
> >>> internal details that depend on this setting; letting that cascade
> >>> into the wire protocol seems like it's just letting the chaos spread
> >>> farther and wider.
> >>
> >> How do you figure that it's not embedded in the wire protocol already?
> >> Not only the replicated data for a timestamp column, but also the
> >> client-visible binary I/O format, depend on this.  I think having some
> >> parts of the protocol use a different timestamp format than other parts
> >> is simply weird, and as this exercise has shown, it's bug-prone as all
> >> get out.
> > 
> > I don't think it's that closely tied together atm. Things like
> > pg_basebackup, pg_receivexlog etc should work, without having to match
> > timestamp storage.  Logical replication, unless your output plugin dumps
> > data in binary / "raw" output, also works just fine across the timestamp
> > divide.
> > 
> > It doesn't sound that hard to add a SystemToIntTimestamp() function,
> > given it only needs to do something if float timestamps are enabled?
> > 
> 
> It's definitely not hard, we already have
> IntegerTimestampToTimestampTz() which does the opposite conversion anyway.
> 
> That being said, I did wonder myself if we should just deprecate float
> timestamps as well.

+1 for deprecating them.  If we need a timestamp(tz) with a wider
range, we are getting options we didn't have before for implementing
it.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


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

[Dave Page]

Hi

On Wed, Feb 22, 2017 at 4:52 PM, Stephen Frost  wrote:
>> > > What about granting to the role to read other statistic views such as
>> > > pg_stat_replication and pg_stat_wal_receiver? Since these informations
>> > > can only be seen by superuser the for example monitoring and
>> > > clustering tool seems to have the same concern.
>> >
>> > Yes, good point.
>>
>> I think basically pg_stat_* should be readable by this role.
>
> Agreed.  I would explicitly point out that we do *not* want to include
> 'pg_statistic' in this as that would include actual data from the
> tables.

Right.

>> > > And what about the diagnostic tools such as pageinspect and pgstattuple?
>> >
>> > I think external/contrib modules should not be included. To install
>> > them you need admin privileges anyway, so you can easily grant
>> > whatever usage privileges you want at that time.
>>
>> I'll start by saying "why not cover contrib"?
>
> +1.

I'm not convinced we should include it, for the reason I gave above.
However, I don't feel that strongly.

What modules should be included?

>> Then I'll say *absolutely* not pageinspect. That is a diagnostics tool and
>> not a monitoring tool. And also, if you give me pageinspect I will happily
>> open up your pg_authid and hack your database. This needs to be superuser
>> only.
>
> Agreed.

+1

>> pgstattuple can be discussed. It doesn't leak anything dangerous. But it
>> does have views that are quite expensive.

I don't think expense should be a concern. It's not like a regular
user cannot run something expensive already, so why stop a user
specifically setup to monitor something?

> For my 2c, I think pgstattuple should be included.  It wouldn't be
> difficult to just have a GRANT at the end of the extension creation
> script to provide the appropriate rights to pg_monitor (or whatever).
>
>> There's also pg_stat_statements, which seems lik eit should be included?
>> Any security issues with that one would be the same as with
>> pg_stat_activity.
>
> Agreed.

OK.

> I do see two issues to be addressed with such a role:
>
> #1- We shouldn't just shove everything into one role.  Where
> functionality can't be GRANT'd independently of the role, we should have
> another default role.  For example, "Read all GUCs" is not something
> that can currently be GRANT'd.  I'm sure there are cases where $admin
> wants a given role to be able to read all GUCs, but not execute
> pg_ls_logdir(), for example.  If we start writing code that refers
> explicitly to pg_monitor then we will end up in an "all-or-nothing" kind
> of situation (not unlike the superuser case) instead of allowing users a
> fine-grained set of options.

I'm fine with having pg_read_all_gucs - it's a trivial change.  I
wouldn't want us to go too far and end up with separate roles for
everything under the sun though.

> That isn't to say that we shouldn't have a pg_monitor role, I'd really
> like to have one, actually, but that role should only have rights which
> can be GRANT'd to it (either by GRANT'ing other default roles to it, or
> by GRANT'ing regular object-level ACLs to it).  What I'm getting at is
> that we should have a 'pg_read_all_gucs' default role for the right and
> then GRANT that role to pg_monitor.

OK.

> #2- We need to define very carefully, up-front, how we will deal with
> new privileges/capabilities/features down the road.  A very specific
> default role like 'pg_read_all_gucs' is quite clear about what's allowed
> by it and I don't think we'd get any push-back from adding new GUCs that
> such a default role could read, but some new view pg_stat_X view that
> would be really useful to monitoring tools might also allow more access
> than the pg_monitor has or that some admins would be comfortable with-
> how do we handle such a case?  I see a few options:
>
>   - Define up-front that pg_monitor has rights on all pg_stat_X views,
> which then requires we provide a definition and clarity on what
> "pg_stat_X" *is* and provides.  We can then later add such views and
> GRANT access to them to pg_monitor.
>
>   - Create new versions of pg_monitor in the future that cover ever
> increasing sets of privileges ("pg_monitor_with_pg_stat_X" or
> "pg_monitor_v11" for PG11 or something).

I prefer the first option. In my experience, users don't much care
about the rights their monitoring user has, as long as it's not full
superuser. The only case where I think there are legitimate concerns
are where you can read arbitrary data (I do not consider query strings
to be in that class for the record). That said, if we ever do add
something like that then there's nothing stopping us from explicitly
documenting that it's excluded from pg_monitor for that reason, and if
desired the user can grant on it as needed.

Using a scheme like that would also mean that the user is more likely
to need to manually update the role their monitoring system uses
following an upgrade.

>   

Re: [HACKERS] Make subquery alias optional in FROM clause

[Bernd Helmle]

On Wed, 2017-02-22 at 08:13 -0700, David G. Johnston wrote:
> I'll contribute to the popular demand aspect but given that the error
> is
> good and the fix is very simple its not exactly a strong desire.

In one project i've recently seen, for some reasons, they need to
maintain an application twice, one for Oracle and the other for
Postgres for years. To be honest, subqueries aren't the only problem,
but having this solved in the backend itself would help to decrease the
amount of maintenance efforts in such projects.

I thought that this would be another thing to make the migration pains
more less, without being too invasive, given that there were already
some thoughts about relaxing alias usage.



-- 
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] Make subquery alias optional in FROM clause

[Nico Williams]

On Wed, Feb 22, 2017 at 10:08:38AM -0500, Tom Lane wrote:
> Bernd Helmle  writes:
> >> From time to time, especially during migration projects from Oracle to
> > PostgreSQL, i'm faced with people questioning why the alias in the FROM
> > clause for subqueries in PostgreSQL is mandatory. The default answer
> > here is, the SQL standard requires it.
> 
> Indeed.  When I wrote the comment you're referring to, quite a few years
> ago now, I thought that popular demand might force us to allow omitted
> aliases.  But the demand never materialized.  At this point it seems
> clear to me that there isn't really good reason to exceed the spec here.
> It just encourages people to write unportable SQL code.

I suspect most users, like me, just roll their eyes, grumble, and put up
with it rather than complain.  It's a pain point, but tolerable enough
that no one bothers to demand a change.  Now that it's been done though,
allow me to add my voice in favor of it!

> > The patch generates an auto-alias for subqueries in the format
> > *SUBQUERY_* for subqueries and *VALUES_* for values
> > expressions.  is the range table index it gets during
> > transformRangeSubselect().
> 
> This is not a solution, because it does nothing to avoid conflicts with
> table names elsewhere in the FROM clause.  If we were going to relax this
> --- which, I repeat, I'm against --- we'd have to come up with something
> that would thumb through the whole query and make sure what it was
> generating didn't already appear somewhere else.  Or else not generate
> a name at all, in which case there simply wouldn't be a way to refer to
> the subquery by name; I'm not sure what that might break though.

On alias conflict... backtrack and retry with a new set of sub-query
names.  For generating the alias names all you need is a gensym-style
counter.  But yes, even this is tricky because you'd have to check that
the conflicting alias name is one of the gensym'ed ones.

Nico
-- 


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

[Stephen Frost]

All,

* Magnus Hagander (mag...@hagander.net) wrote:
> On Wed, Feb 22, 2017 at 1:47 PM, Dave Page  wrote:
> > On Tue, Feb 21, 2017 at 5:40 PM, Masahiko Sawada 
> > wrote:
> > > On Mon, Feb 20, 2017 at 8:48 PM, Dave Page  wrote:
> > >> Further to the patch I just submitted
> > >> (https://www.postgresql.org/message-id/CA%2BOCxow-X%
> > 3DD2fWdKy%2BHP%2BvQ1LtrgbsYQ%3DCshzZBqyFT5jOYrFw%40mail.gmail.com)
> > >> I'd like to propose the addition of a default role, pg_monitor.
> > >>
> > >> The intent is to make it easy for users to setup a role for fully
> > >> monitoring their servers, without requiring superuser level privileges
> > >> which is a problem for many users working within strict security
> > >> policies.
> > >>
> > >> At present, functions or system config info that divulge any
> > >> installation path related info typically require superuser privileges.
> > >> This makes monitoring for unexpected changes in configuration or
> > >> filesystem level monitoring (e.g. checking for large numbers of WAL
> > >> files or log file info) impossible for non-privileged roles.
> > >>
> > >> A similar example is the restriction on the pg_stat_activity.query
> > >> column, which prevents non-superusers seeing any query strings other
> > >> than their own.
> > >>
> > >> Using ACLs is a problem for a number of reasons:
> > >>
> > >> - Users often don't like their database schemas to be modified
> > >> (cluttered with GRANTs).
> > >> - ACL modifications would potentially have to be made in every
> > >> database in a cluster.
> > >> - Using a pre-defined role minimises the setup that different tools
> > >> would have to require.
> > >> - Not all functionality has an ACL (e.g. SHOW)
> > >>
> > >> Other DBMSs solve this problem in a similar way.
> > >>
> > >> Initially I would propose that permission be granted to the role to:
> > >>
> > >> - Execute pg_ls_logdir() and pg_ls_waldir()
> > >> - Read pg_stat_activity, including the query column for all queries.
> > >> - Allow "SELECT pg_tablespace_size('pg_global')"
> > >> - Read all GUCs
> > >>
> > >
> > > Thank you for working on this.
> >
> > You're welcome.
> >
> > > What about granting to the role to read other statistic views such as
> > > pg_stat_replication and pg_stat_wal_receiver? Since these informations
> > > can only be seen by superuser the for example monitoring and
> > > clustering tool seems to have the same concern.
> >
> > Yes, good point.
> 
> I think basically pg_stat_* should be readable by this role.

Agreed.  I would explicitly point out that we do *not* want to include
'pg_statistic' in this as that would include actual data from the
tables.

> > > And what about the diagnostic tools such as pageinspect and pgstattuple?
> >
> > I think external/contrib modules should not be included. To install
> > them you need admin privileges anyway, so you can easily grant
> > whatever usage privileges you want at that time.
> 
> I'll start by saying "why not cover contrib"?

+1.

> Then I'll say *absolutely* not pageinspect. That is a diagnostics tool and
> not a monitoring tool. And also, if you give me pageinspect I will happily
> open up your pg_authid and hack your database. This needs to be superuser
> only.

Agreed.

> pgstattuple can be discussed. It doesn't leak anything dangerous. But it
> does have views that are quite expensive.

For my 2c, I think pgstattuple should be included.  It wouldn't be
difficult to just have a GRANT at the end of the extension creation
script to provide the appropriate rights to pg_monitor (or whatever).

> There's also pg_stat_statements, which seems lik eit should be included?
> Any security issues with that one would be the same as with
> pg_stat_activity.

Agreed.

I do see two issues to be addressed with such a role:

#1- We shouldn't just shove everything into one role.  Where
functionality can't be GRANT'd independently of the role, we should have
another default role.  For example, "Read all GUCs" is not something
that can currently be GRANT'd.  I'm sure there are cases where $admin
wants a given role to be able to read all GUCs, but not execute
pg_ls_logdir(), for example.  If we start writing code that refers
explicitly to pg_monitor then we will end up in an "all-or-nothing" kind
of situation (not unlike the superuser case) instead of allowing users a
fine-grained set of options.

That isn't to say that we shouldn't have a pg_monitor role, I'd really
like to have one, actually, but that role should only have rights which
can be GRANT'd to it (either by GRANT'ing other default roles to it, or
by GRANT'ing regular object-level ACLs to it).  What I'm getting at is
that we should have a 'pg_read_all_gucs' default role for the right and
then GRANT that role to pg_monitor.

#2- We need to define very carefully, up-front, how we will deal with
new privileges/capabilities/features down the road.  A very specific
default role like 'pg_read_all_gucs' is quite 

Re: [HACKERS] Enabling parallelism for queries coming from SQL or other PL functions

[Dilip Kumar]

On Wed, Feb 22, 2017 at 10:25 AM, Rafia Sabih
 wrote:
> Hello everybody,
>
> In the current version, queries in SQL or PL functions can not
> leverage parallelism. To relax this restriction I prepared a patch,
> the approach used in the patch is explained next,

Some initial comments.

--
if (numberTuples || dest->mydest == DestIntoRel)
use_parallel_mode = false;

if (use_parallel_mode)
EnterParallelMode();
+ else if (estate->es_plannedstmt->parallelModeNeeded &&
+ (dest->mydest == DestSPI || dest->mydest == DestSQLFunction))
+ {
+ use_parallel_mode = true;
+ EnterParallelMode();
+ }

I think we can simplify this, can we replace above code with something
like this?

if (dest->mydest == DestIntoRel ||
numberTuples && (dest->mydest != DestSPI || dest->mydest !=
DestSQLFunction))
use_parallel_mode = false;

-

+ {
+ /* Allow parallelism if the function is not trigger type. */
+ if (estate->func->fn_is_trigger == PLPGSQL_NOT_TRIGGER)
+ exec_res = SPI_execute(querystr, estate->readonly_func,
CURSOR_OPT_PARALLEL_OK);
+ else
+ exec_res = SPI_execute(querystr, estate->readonly_func, 0);
+ }

The last parameter of SPI_execute is tuple count, not cursorOption,
you need to fix this. Also, this is crossing the 80 line boundary.

---
Any specific reason for not changing SPI_execute_with_args, EXECUTE
with USING will take this path.


-- 
Regards,
Dilip Kumar
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] Make subquery alias optional in FROM clause

[Bernd Helmle]

On Wed, 2017-02-22 at 10:08 -0500, Tom Lane wrote:
> 
> Indeed.  When I wrote the comment you're referring to, quite a few
> years
> ago now, I thought that popular demand might force us to allow
> omitted
> aliases.  But the demand never materialized.  At this point it seems
> clear to me that there isn't really good reason to exceed the spec
> here.
> It just encourages people to write unportable SQL code.
> 

Years ago i didn't hear anything about it either. But in the last few
months i've heard such a demand several times, so i thought we should
give it another try.

> > The patch generates an auto-alias for subqueries in the format
> > *SUBQUERY_* for subqueries and *VALUES_* for values
> > expressions.  is the range table index it gets during
> > transformRangeSubselect().
> 
> This is not a solution, because it does nothing to avoid conflicts
> with
> table names elsewhere in the FROM clause.  If we were going to relax
> this
> --- which, I repeat, I'm against --- we'd have to come up with
> something
> that would thumb through the whole query and make sure what it was
> generating didn't already appear somewhere else.  

I've thought about this already. One thing that came into my mind was
to maintain a lookup list of aliasnames during the transform phase and
throw an ereport as soon as the generated string has any duplicate. Not
sure about the details, but i was worried about the performance impact
in this area...

> Or else not generate
> a name at all, in which case there simply wouldn't be a way to refer
> to
> the subquery by name; I'm not sure what that might break though.
> 

Hmm, maybe that's an option. Though, i think parts of the code aren't
prepared to deal with empty (or even NULL) aliases. That's likely much
more invasive.




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

[Magnus Hagander]

On Wed, Feb 22, 2017 at 1:47 PM, Dave Page  wrote:

>
> On Tue, Feb 21, 2017 at 5:40 PM, Masahiko Sawada 
> wrote:
> > On Mon, Feb 20, 2017 at 8:48 PM, Dave Page  wrote:
> >> Further to the patch I just submitted
> >> (https://www.postgresql.org/message-id/CA%2BOCxow-X%
> 3DD2fWdKy%2BHP%2BvQ1LtrgbsYQ%3DCshzZBqyFT5jOYrFw%40mail.gmail.com)
> >> I'd like to propose the addition of a default role, pg_monitor.
> >>
> >> The intent is to make it easy for users to setup a role for fully
> >> monitoring their servers, without requiring superuser level privileges
> >> which is a problem for many users working within strict security
> >> policies.
> >>
> >> At present, functions or system config info that divulge any
> >> installation path related info typically require superuser privileges.
> >> This makes monitoring for unexpected changes in configuration or
> >> filesystem level monitoring (e.g. checking for large numbers of WAL
> >> files or log file info) impossible for non-privileged roles.
> >>
> >> A similar example is the restriction on the pg_stat_activity.query
> >> column, which prevents non-superusers seeing any query strings other
> >> than their own.
> >>
> >> Using ACLs is a problem for a number of reasons:
> >>
> >> - Users often don't like their database schemas to be modified
> >> (cluttered with GRANTs).
> >> - ACL modifications would potentially have to be made in every
> >> database in a cluster.
> >> - Using a pre-defined role minimises the setup that different tools
> >> would have to require.
> >> - Not all functionality has an ACL (e.g. SHOW)
> >>
> >> Other DBMSs solve this problem in a similar way.
> >>
> >> Initially I would propose that permission be granted to the role to:
> >>
> >> - Execute pg_ls_logdir() and pg_ls_waldir()
> >> - Read pg_stat_activity, including the query column for all queries.
> >> - Allow "SELECT pg_tablespace_size('pg_global')"
> >> - Read all GUCs
> >>
> >
> > Thank you for working on this.
>
> You're welcome.
>
> > What about granting to the role to read other statistic views such as
> > pg_stat_replication and pg_stat_wal_receiver? Since these informations
> > can only be seen by superuser the for example monitoring and
> > clustering tool seems to have the same concern.
>
> Yes, good point.
>

I think basically pg_stat_* should be readable by this role.



> > And what about the diagnostic tools such as pageinspect and pgstattuple?
>
> I think external/contrib modules should not be included. To install
> them you need admin privileges anyway, so you can easily grant
> whatever usage privileges you want at that time.
>

I'll start by saying "why not cover contrib"?

Then I'll say *absolutely* not pageinspect. That is a diagnostics tool and
not a monitoring tool. And also, if you give me pageinspect I will happily
open up your pg_authid and hack your database. This needs to be superuser
only.

pgstattuple can be discussed. It doesn't leak anything dangerous. But it
does have views that are quite expensive.

There's also pg_stat_statements, which seems lik eit should be included?
Any security issues with that one would be the same as with
pg_stat_activity.

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

Re: [HACKERS] Allow pg_dumpall to work without pg_authid

[David Fetter]

On Wed, Feb 22, 2017 at 06:33:10PM +1100, Robins Tharakan wrote:
> Stephen,
> 
> On 20 February 2017 at 08:50, Stephen Frost  wrote:
> 
> > The other changes to use pg_roles instead of pg_authid when rolpassword
> > isn't being used look like they should just be changed to use pg_roles
> > instead of using one or the other.  That should be an independent patch
> > from the one which adds the option we are discussing.
> >
> 
> Sure. Attached are 2 patches, of which 1 patch just replaces
> ​pg_authid with pg_roles in pg_dumpall. The only exceptions
> there are buildShSecLabels() &
> pg_catalog.binary_upgrade_set_next_pg_authid_oid() which I thought
> should still use pg_authid.

Thanks for doing this!

That pg_dumpall didn't work with RDS Postgres (and possibly others)
was a pretty large wart.

In future, could you please leave patches uncompressed so they're
easier to see in the archives?

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
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] tablesample with partitioned tables

[David Fetter]

On Wed, Feb 22, 2017 at 04:51:46PM +0900, Amit Langote wrote:
> Attached patch fixes an oversight that tablesample cannot be used with
> partitioned tables:
> 
> create table p (a int) partition by list (a);
> select * from p tablesample bernoulli (50);
> ERROR:  TABLESAMPLE clause can only be applied to tables and materialized
> views

Thanks!

Should the error message change somehow to reflect that partitioned
tables are included?  Is complete transparency of partitioned tables
the goal, and reasonable in this context?

Also, is there a good reason apart from tuits not to expand
TABLESAMPLE to the rest of our SQL-visible relation structures?  I'm
guessing this could have something to do with the volatility they
might have, whether in views that call volatile functions or in
foreign tables that might not make the right guarantees...

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
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] Replication vs. float timestamps is a disaster

[Tom Lane]

Andrew Dunstan  writes:
> On 02/22/2017 10:21 AM, Jim Nasby wrote:
>> Only in the catalog though, not the datums, right? I would think you
>> could just change the oid in the catalog the same as you would for a
>> table column.

> No, in the datums.

Yeah, I don't see any way that we could fob off float timestamps to an
extension that would be completely transparent at the pg_upgrade level.
And even a partial solution would be an enormous amount of fundamentally
dead-end work.

There has never been any project policy that promises everybody will be
able to pg_upgrade until the end of time.  I think it's not unreasonable
for us to say that anyone still using float timestamps has to go through
a dump and reload to get to v10.  The original discussion about getting
rid of them was ten years ago come May; that seems long enough.

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] Replication vs. float timestamps is a disaster

[Andrew Dunstan]

On 02/22/2017 10:21 AM, Jim Nasby wrote:
> On 2/22/17 9:12 AM, Andres Freund wrote:
>>> That would allow an in-place upgrade of
>>> a really large cluster. A user would still need to modify their code
>>> to use
>>> the new type.
>>>
>>> Put another way: add ability for pg_upgrade to change the type of a
>>> field.
>>> There might be other uses for that as well.
>> Type oids are unfortunately embedded into composite and array type data
>> - we can do such changes for columns themselves, but it doesn't work if
>> there's any array/composite members containing the to-be-changed type
>> that are used as columns.
>
> Only in the catalog though, not the datums, right? I would think you
> could just change the oid in the catalog the same as you would for a
> table column.

No, in the datums.

cheers

andrew

-- 
Andrew Dunstanhttps://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] Replication vs. float timestamps is a disaster

[Jim Nasby]

On 2/22/17 9:12 AM, Andres Freund wrote:

That would allow an in-place upgrade of
a really large cluster. A user would still need to modify their code to use
the new type.

Put another way: add ability for pg_upgrade to change the type of a field.
There might be other uses for that as well.

Type oids are unfortunately embedded into composite and array type data
- we can do such changes for columns themselves, but it doesn't work if
there's any array/composite members containing the to-be-changed type
that are used as columns.


Only in the catalog though, not the datums, right? I would think you 
could just change the oid in the catalog the same as you would for a 
table column.

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


--
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>  but if you think that it should be somewhere else, please advise Corey
> about where to put it.


Just a reminder that I'm standing by for advice.

The issue at hand is whether the if-state should be a part of the
PsqlScanState, or if it should be a separate state variable owned by
MainLoop() and passed to HandleSlashCommands(), ... or some other solution.

Re: [HACKERS] mat views stats

[Jim Nasby]

On 2/22/17 7:56 AM, Peter Eisentraut wrote:

What behavior would we like by default?  Refreshing a materialized view
is a pretty expensive operation, so I think scheduling an analyze quite
aggressively right afterwards is often what you want.

I think sending a stats message with the number of inserted rows could
make sense.


+1 on both counts. And if sane analyze behavior does depend on the stats 
changes then there's no real advantage to a separate patch.

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


--
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] Make subquery alias optional in FROM clause

[David G. Johnston]

On Wed, Feb 22, 2017 at 8:08 AM, Tom Lane  wrote:

> Or else not generate
> a name at all, in which case there simply wouldn't be a way to refer to
> the subquery by name; I'm not sure what that might break though.
>

​Yeah, usually when I want this I don't end up needing refer by name:

First I write:

SELECT * 
FROM 

The decide I need to do some result filtering

SELECT * FROM (

) --ooops, forgot the alias
WHERE ...

Its for interactive use - and in fact I don't think I'd want to leave my
production queries without names.

David J.

Re: [HACKERS] GRANT EXECUTE ON FUNCTION foo() TO bar();

[Jim Nasby]

On 2/22/17 2:51 AM, Pavel Stehule wrote:

The solution based on rights is elegant, but in this moment I cannot to
see all possible impacts on performance - because it means new check for
any call of any function. Maybe checking call stack can be good enough -
I have not idea how often use case it it.


I think the simple solution to that is not to use proacl for this 
purpose but to add an oidvector to pg_proc that is a list of allowed 
callers. If the vector is kept sorted then it's a simple binary search.


BTW, I agree that this feature would be useful, as would PRIVATE, but 
they're two separate features.

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


--
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] Change in "policy" on dump ordering?

[Jim Nasby]

On 2/22/17 8:00 AM, Peter Eisentraut wrote:

Actually, I think matviews really need to be the absolute last thing.
What if you had a matview that referenced publications or subscriptions?
I'm guessing that would be broken right now.

I'm not sure what you have in mind here.  Publications and subscriptions
don't interact with materialized views, so the relative order doesn't
really matter.


CREATE MATERIALIZED VIEW tmv AS SELECT * FROM pg_subscription;
SELECT 0

IOW, you can create matviews that depend on any other 
table/view/matview, but right now if the matview includes certain items 
it will mysteriously end up empty post-restore.

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


--
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] Make subquery alias optional in FROM clause

[David G. Johnston]

On Wed, Feb 22, 2017 at 8:08 AM, Tom Lane  wrote:

> Bernd Helmle  writes:
> >> From time to time, especially during migration projects from Oracle to
> > PostgreSQL, i'm faced with people questioning why the alias in the FROM
> > clause for subqueries in PostgreSQL is mandatory. The default answer
> > here is, the SQL standard requires it.
>
> Indeed.  When I wrote the comment you're referring to, quite a few years
> ago now, I thought that popular demand might force us to allow omitted
> aliases.  But the demand never materialized.  At this point it seems
> clear to me that there isn't really good reason to exceed the spec here.
> It just encourages people to write unportable SQL code.
>

​I'll contribute to the popular demand aspect but given that the error is
good and the fix is very simple its not exactly a strong desire.

My code is already unportable since I choose to use "::" for casting - and
I'm sure quite a few other PostgreSQL-specific things - so the portability
aspect to the argument is already thin for me and moreso given other DBMSes
already relax the requirement.

David J.​

Re: [HACKERS] Replication vs. float timestamps is a disaster

[Andres Freund]

On 2017-02-22 09:06:38 -0600, Jim Nasby wrote:
> On 2/22/17 7:56 AM, Andres Freund wrote:
> > It sounded more like Jim suggested a full blown SQL type, given that he
> > replied to my concern about the possible need for a deprecation period
> > due to pg_upgrade concerns.  To be useful for that, we'd need a good
> > chunk of magic, so all existing uses of timestamp[tz] are replaced with
> > floattimestamp[tz], duplicate some code, add implicit casts, and accept
> > that composites/arrays won't be fixed.  That sounds like a fair amount
> > of work to me, and we'd still have no way to remove the code without
> > causing pain.
> 
> Right, but I was thinking more in line with just providing the type (as an
> extension, perhaps not even in core) and making it possible for pg_upgrade
> to switch fields over to that type.

Isn't the above what you'd need to do that?


> That would allow an in-place upgrade of
> a really large cluster. A user would still need to modify their code to use
> the new type.
> 
> Put another way: add ability for pg_upgrade to change the type of a field.
> There might be other uses for that as well.

Type oids are unfortunately embedded into composite and array type data
- we can do such changes for columns themselves, but it doesn't work if
there's any array/composite members containing the to-be-changed type
that are used as columns.

- 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] Replication vs. float timestamps is a disaster

[Jim Nasby]

On 2/22/17 7:56 AM, Andres Freund wrote:

On 2017-02-22 08:43:28 -0500, Tom Lane wrote:

Andres Freund  writes:

On 2017-02-22 00:10:35 -0600, Jim Nasby wrote:

I wounder if a separate "floatstamp" data type might fit the bill there. It
might not be completely seamless, but it would be binary compatible.

I don't really see what'd that solve.

Seems to me this is a different name for what I already tried in
<27694.1487456...@sss.pgh.pa.us>.  It would be much better than doing
nothing, IMO, but it would still leave lots of opportunities for mistakes.

It sounded more like Jim suggested a full blown SQL type, given that he
replied to my concern about the possible need for a deprecation period
due to pg_upgrade concerns.  To be useful for that, we'd need a good
chunk of magic, so all existing uses of timestamp[tz] are replaced with
floattimestamp[tz], duplicate some code, add implicit casts, and accept
that composites/arrays won't be fixed.  That sounds like a fair amount
of work to me, and we'd still have no way to remove the code without
causing pain.


Right, but I was thinking more in line with just providing the type (as 
an extension, perhaps not even in core) and making it possible for 
pg_upgrade to switch fields over to that type. That would allow an 
in-place upgrade of a really large cluster. A user would still need to 
modify their code to use the new type.


Put another way: add ability for pg_upgrade to change the type of a 
field. There might be other uses for that as well.

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


--
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] Make subquery alias optional in FROM clause

[Tom Lane]

Bernd Helmle  writes:
>> From time to time, especially during migration projects from Oracle to
> PostgreSQL, i'm faced with people questioning why the alias in the FROM
> clause for subqueries in PostgreSQL is mandatory. The default answer
> here is, the SQL standard requires it.

Indeed.  When I wrote the comment you're referring to, quite a few years
ago now, I thought that popular demand might force us to allow omitted
aliases.  But the demand never materialized.  At this point it seems
clear to me that there isn't really good reason to exceed the spec here.
It just encourages people to write unportable SQL code.

> The patch generates an auto-alias for subqueries in the format
> *SUBQUERY_* for subqueries and *VALUES_* for values
> expressions.  is the range table index it gets during
> transformRangeSubselect().

This is not a solution, because it does nothing to avoid conflicts with
table names elsewhere in the FROM clause.  If we were going to relax this
--- which, I repeat, I'm against --- we'd have to come up with something
that would thumb through the whole query and make sure what it was
generating didn't already appear somewhere else.  Or else not generate
a name at all, in which case there simply wouldn't be a way to refer to
the subquery by name; I'm not sure what that might break though.

regards, tom lane


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

Re: [HACKERS] [PATCH] Suppress Clang 3.9 warnings

[Fabien COELHO]

Hello Aleksander,


```
xloginsert.c:742:18: warning: implicit conversion from 'int' to 'char'
changes value from 253 to -3 [-Wconstant-conversion]
```


There is a bunch of these in "xlog.c" as well, and the same code is used 
in "pg_resetwal.c".



Patch that fixes these warnings is attached to this email.


My 0.02€:

I'm not at ease at putting the thing bluntly under the carpet with a cast.

Why not update the target type to "unsigned char" instead, so that no cast 
is needed and the value compatibility is checked by the compiler? I guess 
there would be some more changes (question is how much), but it would be 
cleaner.


--
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] remove deprecated COMMENT ON RULE syntax

[Tom Lane]

Peter Eisentraut  writes:
> There is support for COMMENT ON RULE  without specifying a table
> name, for upgrading from pre-7.3 instances.  I think it might be time
> for that workaround to go.

No objection here.

regards, tom lane


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

[HACKERS] Make subquery alias optional in FROM clause

[Bernd Helmle]

>From time to time, especially during migration projects from Oracle to
PostgreSQL, i'm faced with people questioning why the alias in the FROM
clause for subqueries in PostgreSQL is mandatory. The default answer
here is, the SQL standard requires it.

This also is exactly the comment in our parser about this topic:

/*
 * The SQL spec does not permit a subselect
 * () without an alias clause,
 * so we don't either.  This avoids the problem
 * of needing to invent a unique refname for it.
 * That could be surmounted if there's sufficient
 * popular demand, but for now let's just implement
 * the spec and see if anyone complains.
 * However, it does seem like a good idea to emit
 * an error message that's better than "syntax error".
 */

So i thought i'm the one standing up for voting to relax this and
making the alias optional.

The main problem, as mentioned in the parser's comment, is to invent a
machinery to create an unique alias for each of the subquery/values
expression in the from clause. I pondered a little about it and came to
the attached result.

The patch generates an auto-alias for subqueries in the format
*SUBQUERY_* for subqueries and *VALUES_* for values
expressions.  is the range table index it gets during
transformRangeSubselect().

Doc patch and simple regression tests included.
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index 211e4c3..f2d21aa 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -51,7 +51,7 @@ SELECT [ ALL | DISTINCT [ ON ( expressiontable_name [ * ] [ [ AS ] alias [ ( column_alias [, ...] ) ] ]
 [ TABLESAMPLE sampling_method ( argument [, ...] ) [ REPEATABLE ( seed ) ] ]
-[ LATERAL ] ( select ) [ AS ] alias [ ( column_alias [, ...] ) ]
+[ LATERAL ] ( select ) [ [ AS ] alias [ ( column_alias [, ...] ) ] ]
 with_query_name [ [ AS ] alias [ ( column_alias [, ...] ) ] ]
 [ LATERAL ] function_name ( [ argument [, ...] ] )
 [ WITH ORDINALITY ] [ [ AS ] alias [ ( column_alias [, ...] ) ] ]
@@ -408,8 +408,7 @@ TABLE [ ONLY ] table_name [ * ]
 output were created as a temporary table for the duration of
 this single SELECT command.  Note that the
 sub-SELECT must be surrounded by
-parentheses, and an alias must be
-provided for it.  A
+parentheses.  An optional alias can be used to name the subquery.  A
  command
 can also be used here.

@@ -1891,6 +1890,31 @@ SELECT distributors.* WHERE distributors.name = 'Westward';
   
 
   
+   Optional subquery alias names in FROM clauses
+
+   
+PostgreSQL allows one to omit
+alias names for subqueries used in FROM-clauses, which
+are required in the SQL standard. Thus, the following SQL is valid in PostgreSQL:
+
+SELECT * FROM (VALUES(1), (2), (3));
+ column1 
+-
+   1
+   2
+   3
+(3 rows)
+
+SELECT * FROM (SELECT 1, 2, 3);
+ ?column? | ?column? | ?column? 
+--+--+--
+1 |2 |3
+(1 row)
+
+   
+  
+
+  
ONLY and Inheritance
 

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 6c6d21b..865b3ce 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -11302,30 +11302,13 @@ table_ref:	relation_expr opt_alias_clause
 	/*
 	 * The SQL spec does not permit a subselect
 	 * () without an alias clause,
-	 * so we don't either.  This avoids the problem
-	 * of needing to invent a unique refname for it.
-	 * That could be surmounted if there's sufficient
-	 * popular demand, but for now let's just implement
-	 * the spec and see if anyone complains.
-	 * However, it does seem like a good idea to emit
-	 * an error message that's better than "syntax error".
+	 * but PostgreSQL isn't that strict here. We
+	 * provide an unique, auto-generated alias
+	 * name instead, which will be done through
+	 * the transform/analyze phase later. See
+	 * parse_clause.c, transformRangeSubselect() for
+	 * details.
 	 */
-	if ($2 == NULL)
-	{
-		if (IsA($1, SelectStmt) &&
-			((SelectStmt *) $1)->valuesLists)
-			ereport(ERROR,
-	(errcode(ERRCODE_SYNTAX_ERROR),
-	 errmsg("VALUES in FROM must have an alias"),
-	 errhint("For example, FROM (VALUES ...) [AS] foo."),
-	 parser_errposition(@1)));
-		else
-			ereport(ERROR,
-	(errcode(ERRCODE_SYNTAX_ERROR),
-	 errmsg("subquery in FROM must have an alias"),
-	 errhint("For example, FROM (SELECT ...) [AS] foo."),
-	 parser_errposition(@1)));
-	}
 	$$ = (Node *) n;
 }
 			| LATERAL_P select_with_parens opt_alias_clause
@@ -11335,22 +11318,6 @@ table_ref:	relation_expr opt_alias_clause
 	n->subquery = $2;
 	n->alias = $3;
 	/* same comment as above */
-	if ($3 == NULL)
-	{
-		if (IsA($2, SelectStmt) &&
-			((SelectStmt *) 

Re: [HACKERS] Replication vs. float timestamps is a disaster

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> While I'm generally not one to vote for dropping backwards-compatibility
>> features, I have to say that I find #4 the most attractive of these
>> options.  It would result in getting rid of boatloads of under-tested
>> code, whereas #2 would really just add more, and #3 at best maintains
>> the status quo complexity-wise.

> +1.

BTW, I did a bit of digging in the archives, and found a couple of
previous discussions around this:

https://www.postgresql.org/message-id/flat/1178476313.18303.164.camel%40goldbach
https://www.postgresql.org/message-id/flat/1287334597.8516.372.camel%40jdavis

as well as numerous discussions of specific bugs associated with float
timestamps, eg this isn't even the first time we've hit a float-timestamp
replication bug:

https://www.postgresql.org/message-id/flat/CAHGQGwFbqTDBrw95jek6_RvYG2%3DE-6o0HOpbeEcP6oWHJTLkUw%40mail.gmail.com

In one or another of those threads, I opined that we'd have to keep float
timestamps available for as long as we were supporting pg_upgrade from
8.3.  However, I think that that argument hasn't really stood the test of
time, because of the lack of packagers shipping builds with float
timestamps turned on.  There may be some number of installations that
still have float timestamps active, but it's got to be a tiny number.
And the list of reasons not to like float timestamps is very long.

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] "may be unused" warnings for gcc

[Peter Eisentraut]

On 2/21/17 22:17, Andres Freund wrote:
> I've not run comparisons this year, but late last year I was seeing > 5%
> < 10% benefits - that seems plenty enough to care.

You mean the 5-minute benchmarks on my laptop are not representative? ;-)

Here is a patch that I had lying around that clears the compiler
warnings under -O3 for me.  It seems that they are a subset of what you
are seeing.  Plausibly, as compilers are doing more analysis in larger
scopes, we can expect to see more of these.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 2a73d3ac095435ecbe3aefff7bcecc292675e167 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 22 Feb 2017 09:23:40 -0500
Subject: [PATCH] Silence compiler warnings from gcc -O3

---
 src/backend/commands/dbcommands.c | 13 +++--
 src/backend/utils/adt/varlena.c   |  6 ++
 src/backend/utils/misc/guc.c  |  2 +-
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 1ebacbc24f..2a23f634c5 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -27,6 +27,7 @@
 #include "access/genam.h"
 #include "access/heapam.h"
 #include "access/htup_details.h"
+#include "access/multixact.h"
 #include "access/xact.h"
 #include "access/xloginsert.h"
 #include "access/xlogutils.h"
@@ -103,14 +104,14 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
 	Relation	rel;
 	Oid			src_dboid;
 	Oid			src_owner;
-	int			src_encoding;
-	char	   *src_collate;
-	char	   *src_ctype;
+	int			src_encoding = -1;
+	char	   *src_collate = NULL;
+	char	   *src_ctype = NULL;
 	bool		src_istemplate;
 	bool		src_allowconn;
-	Oid			src_lastsysoid;
-	TransactionId src_frozenxid;
-	MultiXactId src_minmxid;
+	Oid			src_lastsysoid = InvalidOid;
+	TransactionId src_frozenxid = InvalidTransactionId;
+	MultiXactId src_minmxid = InvalidMultiXactId;
 	Oid			src_deftablespace;
 	volatile Oid dst_deftablespace;
 	Relation	pg_database_rel;
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 28b5745ba8..7da7535b3b 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -1129,6 +1129,8 @@ text_position_setup(text *t1, text *t2, TextPositionState *state)
 		state->use_wchar = false;
 		state->str1 = VARDATA_ANY(t1);
 		state->str2 = VARDATA_ANY(t2);
+		state->wstr1 = 0;
+		state->wstr2 = 0;
 		state->len1 = len1;
 		state->len2 = len2;
 	}
@@ -1144,6 +1146,8 @@ text_position_setup(text *t1, text *t2, TextPositionState *state)
 		len2 = pg_mb2wchar_with_len(VARDATA_ANY(t2), p2, len2);
 
 		state->use_wchar = true;
+		state->str1 = 0;
+		state->str2 = 0;
 		state->wstr1 = p1;
 		state->wstr2 = p2;
 		state->len1 = len1;
@@ -1227,6 +1231,8 @@ text_position_setup(text *t1, text *t2, TextPositionState *state)
 state->skiptable[wstr2[i] & skiptablemask] = last - i;
 		}
 	}
+	else
+		state->skiptablemask = 255;
 }
 
 static int
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 24771389c8..107e1f2957 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -9275,7 +9275,7 @@ RestoreGUCState(void *gucstate)
 	char	   *varname,
 			   *varvalue,
 			   *varsourcefile;
-	int			varsourceline;
+	int			varsourceline = -1;
 	GucSource	varsource;
 	GucContext	varscontext;
 	char	   *srcptr = (char *) gucstate;
-- 
2.11.1


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

[HACKERS] remove deprecated COMMENT ON RULE syntax

[Peter Eisentraut]

There is support for COMMENT ON RULE  without specifying a table
name, for upgrading from pre-7.3 instances.  I think it might be time
for that workaround to go.

Patch attached.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From d15411c389659ac789199e46edaa2de43768e600 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 22 Feb 2017 08:45:14 -0500
Subject: [PATCH] Remove deprecated COMMENT ON RULE syntax

This was only used for allowing upgrades from pre-7.3 instances, which
was a long time ago.
---
 src/backend/catalog/objectaddress.c| 126 -
 src/backend/parser/gram.y  |  10 --
 src/backend/rewrite/rewriteSupport.c   |  55 -
 src/include/rewrite/rewriteSupport.h   |   2 -
 .../test_ddl_deparse/expected/comment_on.out   |   2 +-
 .../modules/test_ddl_deparse/sql/comment_on.sql|   2 +-
 src/test/regress/expected/object_address.out   |   4 +-
 7 files changed, 53 insertions(+), 148 deletions(-)

diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 9029477d68..cc636e2e3e 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -1325,6 +1325,8 @@ get_object_address_relobject(ObjectType objtype, List *objname,
 	Relation	relation = NULL;
 	int			nnames;
 	const char *depname;
+	List	   *relname;
+	Oid			reloid;
 
 	/* Extract name of dependent object. */
 	depname = strVal(llast(objname));
@@ -1332,88 +1334,58 @@ get_object_address_relobject(ObjectType objtype, List *objname,
 	/* Separate relation name from dependent object name. */
 	nnames = list_length(objname);
 	if (nnames < 2)
-	{
-		Oid			reloid;
-
-		/*
-		 * For compatibility with very old releases, we sometimes allow users
-		 * to attempt to specify a rule without mentioning the relation name.
-		 * If there's only rule by that name in the entire database, this will
-		 * work.  But objects other than rules don't get this special
-		 * treatment.
-		 */
-		if (objtype != OBJECT_RULE)
-			elog(ERROR, "must specify relation and object name");
-		address.classId = RewriteRelationId;
-		address.objectId =
-			get_rewrite_oid_without_relid(depname, , missing_ok);
-		address.objectSubId = 0;
-
-		/*
-		 * Caller is expecting to get back the relation, even though we didn't
-		 * end up using it to find the rule.
-		 */
-		if (OidIsValid(address.objectId))
-			relation = heap_open(reloid, AccessShareLock);
-	}
-	else
-	{
-		List	   *relname;
-		Oid			reloid;
+		ereport(ERROR,
+(errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("must specify relation and object name")));
 
-		/* Extract relation name and open relation. */
-		relname = list_truncate(list_copy(objname), nnames - 1);
-		relation = heap_openrv_extended(makeRangeVarFromNameList(relname),
-		AccessShareLock,
-		missing_ok);
+	/* Extract relation name and open relation. */
+	relname = list_truncate(list_copy(objname), nnames - 1);
+	relation = heap_openrv_extended(makeRangeVarFromNameList(relname),
+	AccessShareLock,
+	missing_ok);
 
-		reloid = relation ? RelationGetRelid(relation) : InvalidOid;
+	reloid = relation ? RelationGetRelid(relation) : InvalidOid;
 
-		switch (objtype)
-		{
-			case OBJECT_RULE:
-address.classId = RewriteRelationId;
-address.objectId = relation ?
-	get_rewrite_oid(reloid, depname, missing_ok) : InvalidOid;
-address.objectSubId = 0;
-break;
-			case OBJECT_TRIGGER:
-address.classId = TriggerRelationId;
-address.objectId = relation ?
-	get_trigger_oid(reloid, depname, missing_ok) : InvalidOid;
-address.objectSubId = 0;
-break;
-			case OBJECT_TABCONSTRAINT:
-address.classId = ConstraintRelationId;
-address.objectId = relation ?
-	get_relation_constraint_oid(reloid, depname, missing_ok) :
-	InvalidOid;
-address.objectSubId = 0;
-break;
-			case OBJECT_POLICY:
-address.classId = PolicyRelationId;
-address.objectId = relation ?
-	get_relation_policy_oid(reloid, depname, missing_ok) :
-	InvalidOid;
-address.objectSubId = 0;
-break;
-			default:
-elog(ERROR, "unrecognized objtype: %d", (int) objtype);
-/* placate compiler, which doesn't know elog won't return */
-address.classId = InvalidOid;
-address.objectId = InvalidOid;
-address.objectSubId = 0;
-		}
+	switch (objtype)
+	{
+		case OBJECT_RULE:
+			address.classId = RewriteRelationId;
+			address.objectId = relation ?
+get_rewrite_oid(reloid, depname, missing_ok) : InvalidOid;
+			address.objectSubId = 0;
+			break;
+		case OBJECT_TRIGGER:
+			address.classId = TriggerRelationId;
+			address.objectId = relation ?
+get_trigger_oid(reloid, depname, missing_ok) : InvalidOid;
+			address.objectSubId = 0;
+			break;
+		case OBJECT_TABCONSTRAINT:
+			address.classId = ConstraintRelationId;
+			

Re: [HACKERS] make async slave to wait for lsn to be replayed

[Thom Brown]

On 23 January 2017 at 11:56, Ivan Kartyshov  wrote:
> Thank you for reviews and suggested improvements.
> I rewrote patch to make it more stable.
>
> Changes
> ===
> I've made a few changes:
> 1) WAITLSN now doesn`t depend on snapshot
> 2) Check current replayed LSN rather than in xact_redo_commit
> 3) Add syntax WAITLSN_INFINITE '0/693FF800' - for infinite wait and
> WAITLSN_NO_WAIT '0/693FF800' for check if LSN was replayed as you
> advised.
> 4) Reduce the count of loops with GUCs (WalRcvForceReply() which in 9.5
> doesn`t exist).
> 5) Optimize loop that set latches.
> 6) Add two GUCs that helps us to configure influence on StartupXLOG:
> count_waitlsn (denominator to check not each LSN)
> interval_waitlsn (Interval in milliseconds to additional LSN check)
>
> Feedback
> 
> On 09/15/2016 05:41 AM, Thomas Munro wrote:
>>
>> You hold a spinlock in one arbitrary slot, but that
>> doesn't seem sufficient: another backend may also read it, compute a
>> new value and then write it, while holding a different spin lock.  Or
>> am I missing something?
>
>
> We acquire an individual spinlock on each member of array, so you cannot
> compute new value and write it concurrently.
>
> Tested
> ==
> We have been tested it on different servers and OS`s, in different cases and
> workloads. New version is nearly as fast as vanilla on primary and bring
> tiny influence on standby performance.
>
> Hardware:
> 144 Intel Cores with HT
> 3TB RAM
> all data on ramdisk
> primary + hotstandby  on the same node.
>
> A dataset was created with "pgbench -i -s 1000" command. For each round of
> test we pause replay on standby, make 100 transaction on primary with
> pgbench, start replay on standby and measure replication gap disappearing
> time under different standby workload. The workload was "WAITLSN
> ('Very/FarLSN', 1000ms timeout)" followed by "select abalance from
> pgbench_accounts there aid = random_aid;"
> For vanilla 1000ms timeout was enforced on pgbench side by -R option.
> GUC waitlsn parameters was adopted for 1000ms timeout on standby with 35000
> tps rate on primary.
> interval_waitlsn = 500 (ms)
> count_waitlsn = 3
>
> On 200 clients, slave caching up master as vanilla without significant
> delay.
> On 500 clients, slave caching up master 3% slower then vanilla.
> On 1000 clients, 12% slower.
> On 5000 clients, 3 time slower because it far above our hardware ability.
>
> How to use it
> ==
> WAITLSN ‘LSN’ [, timeout in ms];
> WAITLSN_INFINITE ‘LSN’;
> WAITLSN_NO_WAIT ‘LSN’;
>
> #Wait until LSN 0/303EC60 will be replayed, or 10 second passed.
> WAITLSN ‘0/303EC60’, 1;
>
> #Or same without timeout.
> WAITLSN ‘0/303EC60’;
> orfile:///home/vis/Downloads/waitlsn_10dev_v2.patch
> WAITLSN_INFINITE '0/693FF800';
>
> #To check if LSN is replayed can be used.
> WAITLSN_NO_WAIT '0/693FF800';
>
> Notice: WAITLSN will release on PostmasterDeath or Interruption events
> if they come earlier then target LSN or timeout.
>
>
> Thank you for reading, will be glad to get your feedback.

Could you please rebase your patch as it no longer applies cleanly.

Thanks

Thom


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

Re: [HACKERS] Hash support for grouping sets

[Thom Brown]

On 6 January 2017 at 03:48, Andrew Gierth  wrote:
> Herewith a patch for doing grouping sets via hashing or mixed hashing
> and sorting.
>
> The principal objective is to pick whatever combination of grouping sets
> has an estimated size that fits in work_mem, and minimizes the number of
> sorting passes we need to do over the data, and hash those.  (Yes, this
> is a knapsack problem.)
>
> This is achieved by adding another strategy to the Agg node; AGG_MIXED
> means that both hashing and (sorted or plain) grouping happens.  In
> addition, support for multiple hash tables in AGG_HASHED mode is added.
>
> Sample plans look like this:
>
> explain select two,ten from onek group by cube(two,ten);
>   QUERY PLAN
> --
>  MixedAggregate  (cost=0.00..89.33 rows=33 width=8)
>Hash Key: two, ten
>Hash Key: two
>Hash Key: ten
>Group Key: ()
>->  Seq Scan on onek  (cost=0.00..79.00 rows=1000 width=8)
>
> explain select two,ten from onek group by two, cube(ten,twenty);
>   QUERY PLAN
> ---
>  HashAggregate  (cost=86.50..100.62 rows=162 width=12)
>Hash Key: two, ten, twenty
>Hash Key: two, ten
>Hash Key: two
>Hash Key: two, twenty
>->  Seq Scan on onek  (cost=0.00..79.00 rows=1000 width=12)
>
> set work_mem='64kB';
> explain select count(*) from tenk1
>   group by grouping sets (unique1,thousand,hundred,ten,two);
>QUERY PLAN
> 
>  MixedAggregate  (cost=1535.39..3023.89 rows=2 width=28)
>Hash Key: two
>Hash Key: ten
>Hash Key: hundred
>Group Key: unique1
>Sort Key: thousand
>  Group Key: thousand
>->  Sort  (cost=1535.39..1560.39 rows=1 width=20)
>  Sort Key: unique1
>  ->  Seq Scan on tenk1  (cost=0.00..458.00 rows=1 width=20)
> (10 rows)
>
> Columns with unhashable or unsortable data types are handled
> appropriately, though obviously every individual grouping set must end
> up containing compatible column types.
>
> There is one current weakness which I don't see a good solution for: the
> planner code still has to pick a single value for group_pathkeys before
> planning the input path.  This means that we sometimes can't choose a
> minimal set of sorts, because we may have chosen a sort order for a
> grouping set that should have been hashed, for example:
>
> explain select count(*) from tenk1
>   group by grouping sets (two,unique1,thousand,hundred,ten);
>QUERY PLAN
> 
>  MixedAggregate  (cost=1535.39..4126.28 rows=2 width=28)
>Hash Key: ten
>Hash Key: hundred
>Group Key: two
>Sort Key: unique1
>  Group Key: unique1
>Sort Key: thousand
>  Group Key: thousand
>->  Sort  (cost=1535.39..1560.39 rows=1 width=20)
>  Sort Key: two
>  ->  Seq Scan on tenk1  (cost=0.00..458.00 rows=1 width=20)
>
> (3 sorts, vs. 2 in the previous example that listed the same grouping
> sets in different order)
>
> Current status of the work: probably still needs cleanup, maybe some
> better regression tests, and Andres has expressed some concern over the
> performance impact of additional complexity in advance_aggregates; it
> would be useful if this could be tested.  But it should be reviewable
> as-is.

This doesn't apply cleanly to latest master.  Could you please post a
rebased patch?

Thanks

Thom


-- 
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] Change in "policy" on dump ordering?

[Peter Eisentraut]

On 2/22/17 00:55, Jim Nasby wrote:
> Originally, no, but reviewing the list again I'm kindof wondering about 
> DO_DEFAULT_ACL, especially since the acl code in pg_dump looks at 
> defaults as part of what removes the need to explicitly dump 
> permissions. I'm also wondering if DO_POLICY could potentially affect 
> matviews?

I'm not sure about the details of these, but I know that there are
reasons why the permissions stuff is pretty late in the dump in general.

> Actually, I think matviews really need to be the absolute last thing. 
> What if you had a matview that referenced publications or subscriptions? 
> I'm guessing that would be broken right now.

I'm not sure what you have in mind here.  Publications and subscriptions
don't interact with materialized views, so the relative order doesn't
really matter.

> Not really; it just makes reference to needing to be in-sync with 
> pg_dump.c. My concern is that clearly people went to lengths in the past 
> to put everything possible before DO_PRE_DATA_BOUNDARY (ie, text search 
> and FDW) but most recently added stuff has gone after 
> DO_POST_DATA_BOUNDARY, even though there's no reason it couldn't be 
> pre-data. That's certainly a change, and I suspect it's not intentional

I think the recent additions actually were intentional, although one
could debate the intentions. ;-)

-- 
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] Replication vs. float timestamps is a disaster

[Stephen Frost]

Tom, all,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> While I'm generally not one to vote for dropping backwards-compatibility
> features, I have to say that I find #4 the most attractive of these
> options.  It would result in getting rid of boatloads of under-tested
> code, whereas #2 would really just add more, and #3 at best maintains
> the status quo complexity-wise.

+1.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] mat views stats

[Peter Eisentraut]

On 2/22/17 06:31, Jim Mlodgenski wrote:
> Matviews already show up in the pg_stat_*_tables and the patch does
> leverage the existing pg_stat_*_tables underlying structure, but it
> creates more meaningful pg_stat_*_matviews leaving out things like
> insert and update counts.  

But fields like seq_scans and last_analyze are then redundant between
the *_tables view and the *_matviews view.  Maybe it would make more
sense to introduce a new view like you propose and not show them in
*_tables anymore?

> I was originally thinking 2 patches, but I couldn't think of a way to
> trigger the analyze reliably without adding a refresh count or sending
> bogus stats. We can certainly send a stats message containing the number
> of rows inserted by the refresh, but are we going to also send the
> number of deletes as well? Consider a matview that has month to date
> data. At the end of the month, there will be about 30n live tuples. The
> next day on the new month, there will be n inserts with the stats
> thinking there are 30n live tuples which is below the analyze scale
> factor.  We want to analyze the matview on the first of the day of the
> new month, but it wouldn't be triggered for a few days. We can have
> REFRESH also track live tuples, but it was quickly becoming a slippery
> slope of changing behavior for a back patch. Maybe that's OK and we can
> go down that road.

For those not reading the patch, it introduces a new reloption
autovacuum_analyze_refresh_threshold that determines when to autoanalyze
a materialized view.

What behavior would we like by default?  Refreshing a materialized view
is a pretty expensive operation, so I think scheduling an analyze quite
aggressively right afterwards is often what you want.

I think sending a stats message with the number of inserted rows could
make sense.

-- 
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] Replication vs. float timestamps is a disaster

[Andres Freund]

On 2017-02-22 08:43:28 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > On 2017-02-22 00:10:35 -0600, Jim Nasby wrote:
> >> I wounder if a separate "floatstamp" data type might fit the bill there. It
> >> might not be completely seamless, but it would be binary compatible.
> 
> > I don't really see what'd that solve.
> 
> Seems to me this is a different name for what I already tried in
> <27694.1487456...@sss.pgh.pa.us>.  It would be much better than doing
> nothing, IMO, but it would still leave lots of opportunities for mistakes.

It sounded more like Jim suggested a full blown SQL type, given that he
replied to my concern about the possible need for a deprecation period
due to pg_upgrade concerns.  To be useful for that, we'd need a good
chunk of magic, so all existing uses of timestamp[tz] are replaced with
floattimestamp[tz], duplicate some code, add implicit casts, and accept
that composites/arrays won't be fixed.  That sounds like a fair amount
of work to me, and we'd still have no way to remove the code without
causing pain.


> 1. Just patch the already-identified float-vs-int-timestamp bugs in as
> localized a fashion as possible, and hope that there aren't any more and
> that we never introduce any more.  I find this hope foolish :-(,
> especially in view of the fact that what started this discussion is a
> newly-introduced bug of this ilk.

Well, the newly introduced bug was just a copy of the existing code.
I'm far less negative about this than you - we're not dealing with a
whole lot of code here. I don't get why it'd be foolish to verify the
limited amount of code dealing with replication protocol timestamp.

> 4. Give up on float timestamps altogether.
> 
> While I'm generally not one to vote for dropping backwards-compatibility
> features, I have to say that I find #4 the most attractive of these
> options.  It would result in getting rid of boatloads of under-tested
> code, whereas #2 would really just add more, and #3 at best maintains
> the status quo complexity-wise.

> (To be concrete, I'm suggesting dropping --disable-integer-datetimes
> in HEAD, and just agreeing that in the back branches, use of replication
> protocol with float-timestamp servers is not supported and we're not
> going to bother looking for related bugs there.  Given the lack of field
> complaints, I do not believe anyone cares.)

I think we should just fix it in the back branches, and drop the float
timestamp code in 10 or 11.  I.e. 1) and then 4).

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] Logical replication existing data copy

[Erik Rijkers]

On 2017-02-22 13:03, Petr Jelinek wrote:


0001-Skip-unnecessary-snapshot-builds.patch
0002-Don-t-use-on-disk-snapshots-for-snapshot-export-in-l.patch
0003-Fix-xl_running_xacts-usage-in-snapshot-builder.patch
0001-Use-asynchronous-connect-API-in-libpqwalreceiver.patch
0002-Fix-after-trigger-execution-in-logical-replication.patch
0003-Add-RENAME-support-for-PUBLICATIONs-and-SUBSCRIPTION.patch
0001-Logical-replication-support-for-initial-data-copy-v5.patch


It works well now, or at least my particular test case seems now solved.

thanks,

Erik Rijkers


--
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] Replication vs. float timestamps is a disaster

[Tom Lane]

Andres Freund  writes:
> On 2017-02-22 00:10:35 -0600, Jim Nasby wrote:
>> I wounder if a separate "floatstamp" data type might fit the bill there. It
>> might not be completely seamless, but it would be binary compatible.

> I don't really see what'd that solve.

Seems to me this is a different name for what I already tried in
<27694.1487456...@sss.pgh.pa.us>.  It would be much better than doing
nothing, IMO, but it would still leave lots of opportunities for mistakes.

To review the bidding a bit, it seems to me we've got four possible ways
of dealing with this issue:

1. Just patch the already-identified float-vs-int-timestamp bugs in as
localized a fashion as possible, and hope that there aren't any more and
that we never introduce any more.  I find this hope foolish :-(,
especially in view of the fact that what started this discussion is a
newly-introduced bug of this ilk.

2. Introduce some new notation, perhaps <27694.1487456...@sss.pgh.pa.us>
plus new "sendtimestamp" and "recvtimestamp" functions, to try to provide
some compiler-assisted support for getting it right.

3. Give up on "protocol timestamps are always integer style".

4. Give up on float timestamps altogether.

While I'm generally not one to vote for dropping backwards-compatibility
features, I have to say that I find #4 the most attractive of these
options.  It would result in getting rid of boatloads of under-tested
code, whereas #2 would really just add more, and #3 at best maintains
the status quo complexity-wise.

I think it was clear from the day we switched to integer timestamps as the
default that the days of float timestamps were numbered, and that we were
only going to continue to support them as long as it wasn't costing a lot
of effort.  We have now reached a point at which it's clear that
continuing to support them will have direct and significant costs.
I say it's time to pull the plug.

(To be concrete, I'm suggesting dropping --disable-integer-datetimes
in HEAD, and just agreeing that in the back branches, use of replication
protocol with float-timestamp servers is not supported and we're not
going to bother looking for related bugs there.  Given the lack of field
complaints, I do not believe anyone cares.)

regards, tom lane


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

[HACKERS] Cleanup: avoid direct use of ip_posid/ip_blkid

[Pavan Deolasee]

Hello All,

I would like to propose the attached patch which removes all direct
references to ip_posid and ip_blkid members of ItemPointerData struct and
instead use ItemPointerGetOffsetNumber and ItemPointerGetBlockNumber macros
respectively to access these members.

My motivation to do this is to try to free-up some bits from ip_posid
field, given that it can only be maximum 13-bits long. But even without
that, it seems like a good cleanup to me.

One reason why these macros are not always used is because they typically
do assert-validation to ensure ip_posid has a valid value. There are a few
places in code, especially in GIN and also when we are dealing with
user-supplied TIDs when we might get a TID with invalid ip_posid. I've
handled that by defining and using separate macros which skip the
validation. This doesn't seem any worse than what we are already doing.

make check-world with --enable-tap-tests passes.

Comments/suggestions?

Thanks,
Pavan

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


remove_ip_posid_blkid_ref_v3.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] GRANT EXECUTE ON FUNCTION foo() TO bar();

[Joel Jacobson]

On Wed, Feb 22, 2017 at 2:18 PM, Tom Lane  wrote:
> I think this is really *not* a good idea.  The entire permissions model
> is built around granting permissions to roles, by other roles.

My bad. I shouldn't have proposed the idea on how to achieve/implement the idea.

I should instead just have presented the idea without suggesting to
use the permissions model.

Do you think it's a bad idea in general? Or is it just the idea of
using the permissions model for the purpose that is a bad idea?

If it's a good idea apart from that, then maybe we can figure out some other
more feasible way to control what functions can call what other functions?

> It's not that hard, if you have needs like this, to make an owning role
> for each such function.  You might end up with a lot of single-purpose
> roles, but they could be grouped under one or a few group roles for most
> purposes beyond the individual tailored grants.

I think that approach is not very user-friendly, but maybe it can be
made more convenient if adding syntactic sugar to allow doing it all
in a single command?

Or maybe there is some other way to implement it without the permissions model.


-- 
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] GRANT EXECUTE ON FUNCTION foo() TO bar();

[Tom Lane]

Joel Jacobson  writes:
> Currently, it's only possible to grant/revoke execute on functions to roles.

> I think it would be useful in many situations, both for documentation 
> purposes,
> but also for increased security, to in a precise way control what
> other function(s) are allowed to execute a specific function.

I think this is really *not* a good idea.  The entire permissions model
is built around granting permissions to roles, by other roles.  Allowing
non-role objects to hold permissions would be a complicated mess and
probably create security bugs.  Confusing function OIDs with role OIDs
is a likely example.  Another problem is that roles are installation-wide
while functions are not, and all the ACL catalog infrastructure is
designed for the permissions-holding entities to be installation-wide.
No doubt that could be dealt with, but it would be more complexity and
another fertile source of bugs.  Complexity in security-related concepts
is not a good thing.

It's not that hard, if you have needs like this, to make an owning role
for each such function.  You might end up with a lot of single-purpose
roles, but they could be grouped under one or a few group roles for most
purposes beyond the individual tailored grants.

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] Partitioned tables and relfilenode

[Ashutosh Bapat]

>
>> I am wondering whether we should deal with inh flat reset in a
>> slightly different way. Let expand_inherited_rtentry() mark inh =
>> false for the partitioned tables without any partitions and deal with
>> those at the time of estimating size by marking those as dummy. That
>> might be better than multiple changes here. I will try to cook up a
>> patch soon for that.
>
> Are thinking something along the lines of the attached rewritten patch
> 0003?  I also tend to think that's probably a cleaner patch.  Thanks for
> the idea.

Yes. Thanks for working on it.

>
>> Also we should add tests to make sure the scans on partitioned tables
>> without any partitions do not get into problems. PFA patch which adds
>> those tests.
>
> I added the test case you suggest, but kept just the first one.

The second one was testing multi-level partitioning case, which
deserves a testcase by its own.

Some more comments

The comment below seems too verbose. All it can say is "A partitioned table
without any partitions results in a dummy relation." I don't think we need to
be explicit about rte->inh. But it's more of personal preference. We will leave
it to the committer, if you don't agree.
+   /*
+* An empty partitioned table, i.e., one without any leaf
+* partitions, as signaled by rte->inh being set to false
+* by the prep phase (see expand_inherited_rtentry).
+*/

We don't need this comment as well. Rather than repeating what's been said at
the top of the function, it should just refer to it like "nominal relation has
been set above for partitioned tables. For other parent relations, we'll use
the first child ...".
+*
+* If the parent is a partitioned table, we do not have a separate RTE
+* representing it as a member of the inheritance set, because we do
+* not create a scan plan for it.  As mentioned at the top of this
+* function, we make the parent RTE itself the nominal relation.
 */

Following condition is not very readable. It's not evident that it's of the
form (A && B) || C, at a glance it looks like it's A && (B || C).
+   if ((rte->relkind != RELKIND_PARTITIONED_TABLE &&
+list_length(appinfos) < 2) || list_length(appinfos) < 1)

Instead you may rearrage it as
min_child_rels = (rte->relkind == RELKIND_PARTITIONED_TABLE ? 1 : 2);
if (list_length(appinfos) < min_child_rels)

-- 
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] pg_monitor role

[Dave Page]

Hi

On Tue, Feb 21, 2017 at 5:40 PM, Masahiko Sawada  wrote:
> On Mon, Feb 20, 2017 at 8:48 PM, Dave Page  wrote:
>> Further to the patch I just submitted
>> (https://www.postgresql.org/message-id/CA%2BOCxow-X%3DD2fWdKy%2BHP%2BvQ1LtrgbsYQ%3DCshzZBqyFT5jOYrFw%40mail.gmail.com)
>> I'd like to propose the addition of a default role, pg_monitor.
>>
>> The intent is to make it easy for users to setup a role for fully
>> monitoring their servers, without requiring superuser level privileges
>> which is a problem for many users working within strict security
>> policies.
>>
>> At present, functions or system config info that divulge any
>> installation path related info typically require superuser privileges.
>> This makes monitoring for unexpected changes in configuration or
>> filesystem level monitoring (e.g. checking for large numbers of WAL
>> files or log file info) impossible for non-privileged roles.
>>
>> A similar example is the restriction on the pg_stat_activity.query
>> column, which prevents non-superusers seeing any query strings other
>> than their own.
>>
>> Using ACLs is a problem for a number of reasons:
>>
>> - Users often don't like their database schemas to be modified
>> (cluttered with GRANTs).
>> - ACL modifications would potentially have to be made in every
>> database in a cluster.
>> - Using a pre-defined role minimises the setup that different tools
>> would have to require.
>> - Not all functionality has an ACL (e.g. SHOW)
>>
>> Other DBMSs solve this problem in a similar way.
>>
>> Initially I would propose that permission be granted to the role to:
>>
>> - Execute pg_ls_logdir() and pg_ls_waldir()
>> - Read pg_stat_activity, including the query column for all queries.
>> - Allow "SELECT pg_tablespace_size('pg_global')"
>> - Read all GUCs
>>
>
> Thank you for working on this.

You're welcome.

> What about granting to the role to read other statistic views such as
> pg_stat_replication and pg_stat_wal_receiver? Since these informations
> can only be seen by superuser the for example monitoring and
> clustering tool seems to have the same concern.

Yes, good point.

> And what about the diagnostic tools such as pageinspect and pgstattuple?

I think external/contrib modules should not be included. To install
them you need admin privileges anyway, so you can easily grant
whatever usage privileges you want at that time.

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: 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] dropping partitioned tables without CASCADE

[Ashutosh Bapat]

On Wed, Feb 22, 2017 at 11:11 AM, Amit Langote
 wrote:
> On 2017/02/22 13:46, Ashutosh Bapat wrote:
>> Looks good to me. In the attached patch I have added a comment
>> explaining the reason to make partition tables "Auto" dependent upon
>> the corresponding partitioned tables.
>
> Good call.
>
> +   /*
> +* Unlike inheritance children, partition tables are expected to be 
> dropped
> +* when the parent partitioned table gets dropped.
> +*/
>
> Hmm.  Partitions *are* inheritance children, so we perhaps don't need the
> part before the comma.  Also, adding "automatically" somewhere in there
> would be nice.
>
> Or, one could just write: /* add an auto dependency for partitions */

I changed it in the attached patch to
+/*
+ * Partition tables are expected to be dropped when the parent partitioned
+ * table gets dropped.
+ */

>
>> In the tests we are firing commands to drop partitioned table, but are
>> not checking whether those tables or the partitions are getting
>> dropped or not. Except for drop_if_exists.sql, I did not find that we
>> really check this. Should we try a query on pg_class to ensure that
>> the tables get really dropped?
>
> I don't see why this patch should do it, if dependency.sql itself does
> not?  I mean dropping AUTO dependent objects is one of the contracts of
> dependency.c, so perhaps it would make sense to query pg_class in
> dependency.sql to check if AUTO dependencies work correctly.

Hmm, I agree.

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


0001-Allow-dropping-partitioned-table-without-CASCADE_v3.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] [patch] reorder tablespaces in basebackup tar stream for backup_label

[Bernd Helmle]

Am Dienstag, den 21.02.2017, 11:17 +0100 schrieb Michael Banck:
> However, third party tools using the BASE_BACKUP command might want
> to
> extract the backup_label, e.g. in order to figure out the START WAL
> LOCATION. If they make a big tarball for the whole cluster
> potentially
> including all external tablespaces, then the backup_label file is
> somewhere in the middle of it and it takes a long time for tar to
> extract it.
> 
> So I am proposing the attached patch, which sends the base tablespace
> first, and then all the other external tablespaces afterwards, thus
> having base_backup be the first file in the tar in all cases. Does
> anybody see a problem with that?
> 
> 
> Michael
> 
> [1] Chapter 52.3 of the documentation says "one or more CopyResponse
> results will be sent, one for the main data directory and one for
> each
> additional tablespace other than pg_default and pg_global.", which
> makes
> it sound like the main data directory is first, but in my testing,
> this
> is not the case.

The comment in the code says explicitely to add the base directory to
the end of the list, not sure if that is out of a certain reason.

I'd say this is an oversight in the implementation. I'm currently
working on a tool using the streaming protocol directly and i've
understood it exactly the way, that the default tablespace is the first
one in the stream.

So +1 for the patch.


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