date:20150710


On 07/10/2015 02:06 AM, Tom Lane wrote:

Andres Freund and...@anarazel.de writes:

On 2015-07-06 11:49:54 -0400, Tom Lane wrote:

Rather than reverting cab9a0656c36739f, which would re-introduce a
different performance problem, perhaps we could have COPY create a new
relfilenode when it does this.  That should be safe if the table was
previously empty.



I'm not convinced that cab9a0656c36739f needs to survive in that
form. To me only allowing one COPY to benefit from the wal_level =
minimal optimization has a significantly higher cost than
cab9a0656c36739f.


What evidence have you got to base that value judgement on?

cab9a0656c36739f was based on an actual user complaint, so we have good
evidence that there are people out there who care about the cost of
truncating a table many times in one transaction.


Yeah, if we specifically made that case cheap, in response to a 
complaint, it would be a regression to make it expensive again. We might 
get away with it in a major version, but would hate to backpatch that.



My tentative guess is that the best course is to
a) Make heap_truncate_one_rel() create a new relfeilnode. That fixes the
truncation replay issue.
b) Force new pages to be used when using the heap_sync mode in
COPY. That avoids the INIT danger you found. It seems rather
reasonable to avoid using pages that have already been the target of
WAL logging here in general.


And what reason is there to think that this would fix all the problems?
We know of those two, but we've not exactly looked hard for other cases.


Hmm. Perhaps that could be made to work, but it feels pretty fragile. 
For example, you could have an insert trigger on the table that inserts 
additional rows to the same table, and those inserts would be intermixed 
with the rows inserted by COPY. You'll have to avoid that somehow. 
Full-page images in general are a problem. If a checkpoint happens, and 
a trigger modifies the page we're COPYing to in any way, you have the 
same issue. Even reading a page can cause a full-page image of it to be 
written: If you update a hint bit on the page while reading it, and 
checksums are enabled, and a checkpoint happened since the page was last 
updated, bang. I don't think that's a problem in this case because there 
are no hint bits to be set on pages that we're COPYing to, but it's a 
whole new subtle assumption.


I think we should
1. reliably and explicitly keep track of whether we've WAL-logged any 
TRUNCATE, INSERT/UPDATE+INIT, or any other full-page-logging operations 
on the relation, and

2. make sure we never skip WAL-logging again if we have.

Let's add a flag, rd_skip_wal_safe, to RelationData that's initially set 
when a new relfilenode is created, i.e. whenever rd_createSubid or 
rd_newRelfilenodeSubid is set. Whenever a TRUNCATE or a full-page image 
(including INSERT/UPDATE+INIT) is WAL-logged, clear the flag. In copy.c, 
only skip WAL-logging if the flag is still set. To deal with the case 
that the flag gets cleared in the middle of COPY, also check the flag 
whenever we're about to skip WAL-logging in heap_insert, and if it's 
been cleared, ignore the HEAP_INSERT_SKIP_WAL option and WAL-log anyway.


Compared to the status quo, that disables the WAL-skipping optimization 
in the scenario where you CREATE, INSERT, then COPY to a table in the 
same transaction. I think that's acceptable.


(Alternatively, to handle the case that the flag gets cleared in the 
middle of COPY, add another flag to RelationData indicating that a 
WAL-skipping COPY is in-progress, and refrain from WAL-logging any 
FPW-writing operations on the table when it's set (or any operations 
whatsoever). That'd be more efficient, but it's such a rare corner case 
that it hardly matters.)


- Heikki



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

Re: [HACKERS] WAL logging problem in 9.4.3?

On 2015-07-09 19:06:11 -0400, Tom Lane wrote:
 What evidence have you got to base that value judgement on?

 cab9a0656c36739f was based on an actual user complaint, so we have good
 evidence that there are people out there who care about the cost of
 truncating a table many times in one transaction.  On the other hand,
 I know of no evidence that anyone's depending on multiple sequential
 COPYs, nor intermixed COPY and INSERT, to be fast.  The original argument
 for having this COPY optimization at all was to make restoring pg_dump
 scripts in a single transaction fast; and that use-case doesn't care
 about anything but a single COPY into a virgin table.

Well, you'll hardly have heard complaints about COPY, given that we
behaved like currently for a long while.

I definitely know of ETL like processes that have relied on subsequent
COPYs into truncates relations being cheaper. Can't remember the same
for intermixed COPY and INSERT, but it'd not surprise me if somebody
mixed COPY and UPDATEs rather freely for ETL.

 I think you're worrying about exactly the wrong case.

  My tentative guess is that the best course is to
  a) Make heap_truncate_one_rel() create a new relfeilnode. That fixes the
 truncation replay issue.
  b) Force new pages to be used when using the heap_sync mode in
 COPY. That avoids the INIT danger you found. It seems rather
 reasonable to avoid using pages that have already been the target of
 WAL logging here in general.

 And what reason is there to think that this would fix all the
 problems?

Yea, that's the big problem.

 Again, the only known field usage for the COPY optimization is the pg_dump
 scenario; were that not so, we'd have noticed the problem long since.
 So I don't have any faith that this is a well-tested area.

You need to crash in the right moment. I don't think that's that
frequently exercised...


-- 
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] configure can't detect proper pthread flags


On 07/08/2015 08:50 PM, Tom Lane wrote:

Heikki Linnakangas hlinn...@iki.fi writes:

The only scenario where you might now get warnings if we switch to
upstream version, and didn't before, is if one of the flags makes
pthreads to work, but also creates compiler warnings, while another flag
later in the list would make it work without warnings. That's not
totally inconceivable, but I doubt it happens. In any case, there's
nothing PostgreSQL-specific about that, so if that happens, I'd like to
find out so that we can complain to the upstream.


Actually, it looks like the modern version of ax_pthread.m4 adds -Werror
while testing, so that this should not be an issue anyway (at least on
compilers that accept -Werror).


To conclude this thread: I replaced our custom pthread-checking autoconf 
macro with the latest upstream version, in PostgreSQL master. That 
should fix your original problem with the uClibc, OpenSSL, and pthreads 
combination, in PostgreSQL master (i.e. the future 9.6 version).


As a workaround for current versions, you can give 
PTHREAD_CFLAGS=-pthread (or whatever the right flag for that platform 
is) explicitly on the configure command line. That way the autoconf 
script will only test if that option works, skipping the autodetection 
logic and the warnings-test, so it will pass.


- Heikki



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

[HACKERS] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

2015-07-10 Thread Etsuro Fujita

To save cycles, I modified create_foreignscan_plan so that it detects
whether any system columns are requested if scanning a base relation.
Also, I revised other code there a little bit.

For ExecInitForeignScan, I simplified the code there to determine the
scan tuple type, whith seems to me complex beyound necessity.  Maybe
that might be nitpicking, though.

Best regards,
Etsuro Fujita
*** a/src/backend/executor/nodeForeignscan.c
--- b/src/backend/executor/nodeForeignscan.c
***
*** 159,182  ExecInitForeignScan(ForeignScan *node, EState *estate, int eflags)
  	}
  
  	/*
! 	 * Determine the scan tuple type.  If the FDW provided a targetlist
! 	 * describing the scan tuples, use that; else use base relation's rowtype.
  	 */
! 	if (node-fdw_scan_tlist != NIL || currentRelation == NULL)
  	{
  		TupleDesc	scan_tupdesc;
  
  		scan_tupdesc = ExecTypeFromTL(node-fdw_scan_tlist, false);
  		ExecAssignScanType(scanstate-ss, scan_tupdesc);
  		/* Node's targetlist will contain Vars with varno = INDEX_VAR */
  		tlistvarno = INDEX_VAR;
  	}
- 	else
- 	{
- 		ExecAssignScanType(scanstate-ss, RelationGetDescr(currentRelation));
- 		/* Node's targetlist will contain Vars with varno = scanrelid */
- 		tlistvarno = scanrelid;
- 	}
  
  	/*
  	 * Initialize result tuple type and projection info.
--- 159,183 
  	}
  
  	/*
! 	 * Determine the scan tuple type.
  	 */
! 	if (scanrelid  0)
! 	{
! 		/* Use base relation's rowtype */
! 		ExecAssignScanType(scanstate-ss, RelationGetDescr(currentRelation));
! 		/* Node's targetlist will contain Vars with varno = scanrelid */
! 		tlistvarno = scanrelid;
! 	}
! 	else
  	{
  		TupleDesc	scan_tupdesc;
  
+ 		/* Use targetlist provided by the FDW */
  		scan_tupdesc = ExecTypeFromTL(node-fdw_scan_tlist, false);
  		ExecAssignScanType(scanstate-ss, scan_tupdesc);
  		/* Node's targetlist will contain Vars with varno = INDEX_VAR */
  		tlistvarno = INDEX_VAR;
  	}
  
  	/*
  	 * Initialize result tuple type and projection info.
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
***
*** 2050,2058  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
  	RelOptInfo *rel = best_path-path.parent;
  	Index		scan_relid = rel-relid;
  	Oid			rel_oid = InvalidOid;
- 	Bitmapset  *attrs_used = NULL;
- 	ListCell   *lc;
- 	int			i;
  
  	Assert(rel-fdwroutine != NULL);
  
--- 2050,2055 
***
*** 2112,2147  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
  	}
  
  	/*
! 	 * Detect whether any system columns are requested from rel.  This is a
! 	 * bit of a kluge and might go away someday, so we intentionally leave it
! 	 * out of the API presented to FDWs.
! 	 *
! 	 * First, examine all the attributes needed for joins or final output.
! 	 * Note: we must look at reltargetlist, not the attr_needed data, because
! 	 * attr_needed isn't computed for inheritance child rels.
  	 */
! 	pull_varattnos((Node *) rel-reltargetlist, rel-relid, attrs_used);
! 
! 	/* Add all the attributes used by restriction clauses. */
! 	foreach(lc, rel-baserestrictinfo)
  	{
! 		RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
  
! 		pull_varattnos((Node *) rinfo-clause, rel-relid, attrs_used);
! 	}
  
! 	/* Now, are any system columns requested from rel? */
! 	scan_plan-fsSystemCol = false;
! 	for (i = FirstLowInvalidHeapAttributeNumber + 1; i  0; i++)
! 	{
! 		if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
  		{
! 			scan_plan-fsSystemCol = true;
! 			break;
  		}
- 	}
  
! 	bms_free(attrs_used);
  
  	return scan_plan;
  }
--- 2109,2153 
  	}
  
  	/*
! 	 * If we're scanning a base relation, detect whether any system columns
! 	 * are requested from the rel.  (Irrelevant if scanning a join relation.)
! 	 * This is a bit of a kluge and might go away someday, so we intentionally
! 	 * leave it out of the API presented to FDWs.
  	 */
! 	scan_plan-fsSystemCol = false;
! 	if (scan_relid  0)
  	{
! 		Bitmapset  *attrs_used = NULL;
! 		ListCell   *lc;
! 		int			i;
  
! 		/*
! 		 * First, examine all the attributes needed for joins or final output.
! 		 * Note: we must look at reltargetlist, not the attr_needed data,
! 		 * because attr_needed isn't computed for inheritance child rels.
! 		 */
! 		pull_varattnos((Node *) rel-reltargetlist, scan_relid, attrs_used);
  
! 		/* Add all the attributes used by restriction clauses. */
! 		foreach(lc, rel-baserestrictinfo)
  		{
! 			RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
! 
! 			pull_varattnos((Node *) rinfo-clause, scan_relid, attrs_used);
  		}
  
! 		/* Now, are any system columns requested from rel? */
! 		for (i = FirstLowInvalidHeapAttributeNumber + 1; i  0; i++)
! 		{
! 			if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
! 			{
! scan_plan-fsSystemCol = true;
! break;
! 			}
! 		}
! 
! 		bms_free(attrs_used);
! 	}
  
  	return scan_plan;
  }

-- 
Sent via pgsql-hackers mailing

Re: [HACKERS] Freeze avoidance of very large table.

2015-07-10 Thread Simon Riggs

On 10 July 2015 at 09:49, Sawada Masahiko sawada.m...@gmail.com wrote:


  It is a minor thing, but if there is no other use for this fourth
  bit-space, it seems a shame to waste it when there is some use for
 it.  I
  haven't looked at the code around this area to know how hard it would be
 to
  implement the setting and clearing of the bit.

 I think so too, we would be able to use unused fourth status of bits
 efficiently.
 Should I include these improvement into this patch?
 This topic should be discussed on another thread after this feature is
 committed, I think.


The impossible state acts as a diagnostic check for us to ensure the bitmap
is not itself corrupt.

-1 for using it for another purpose.

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

Re: [HACKERS] WAL logging problem in 9.4.3?

On 2015-07-10 19:23:28 +0900, Fujii Masao wrote:
 Maybe I'm missing something. But I start wondering why TRUNCATE
 and INSERT (or even all the operations on the table created at
 the current transaction) need to be WAL-logged while COPY can be
 optimized. If no WAL records are generated on that table, the problem
 we're talking about seems not to occur. Also this seems safe and
 doesn't degrade the performance of data loading. Thought?

Skipping WAL logging means that you need to scan through the whole
shrared buffers to write out dirty buffers and fsync the segments. A
single insert wal record is a couple orders of magnitudes cheaper than
that.  Essentially doing this juts for COPY is a heuristic.


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

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

2015-07-10 Thread Beena Emerson

Hello,

Tue, Jul 7, 2015 at 02:56 AM, Josh Berkus wrote:
pro-JSON:

* standard syntax which is recognizable to sysadmins and devops.
* can use JSON/JSONB functions with ALTER SYSTEM SET to easily make
additions/deletions from the synch rep config.
* can add group labels (see below)

Adding group labels do have a lot of values but as Amit has pointed out,
with little modification, they can be included in GUC as well. It will not
make it any more complex.

On Tue, Jul 7, 2015 at 2:19 PM, Michael Paquier wrote:

Something like pg_syncinfo/ coupled with a LW lock, we already do
something similar for replication slots with pg_replslot/.

I was trying to figure out how the JSON metadata can be used.
It would have to be set using a given set of functions. Right?
I am sorry this question is very basic.

The functions could be something like:
1. pg_add_synch_set(set_name NAME, quorum INT, is_priority bool, set_members
VARIADIC)

This will be used to add a sync set. The set_members can be individual
elements of another set name. The parameter is_priority is used to decide
whether the set is priority (true) set or quorum (false). This function call
will create a folder pg_syncinfo/groups/$NAME and store the json blob?

The root group would be automatically sset by finding the group which is not
included in other groups? or can be set by another function?

2. pg_modify_sync_set(set_name NAME, quorum INT, is_priority bool,
set_members VARIADIC)

This will update the pg_syncinfo/groups/$NAME to store the new values.

3. pg_drop_synch_set(set_name NAME)

This will update the pg_syncinfo/groups/$NAME folder. Also all the groups
which included this would be updated?

4. pg_show_synch_set()

this will display the current sync setting in json format.

Am I missing something?

Is JSON being preferred because it would be ALTER SYSTEM friendly and in a
format already known to users?

In a real-life scenario, at most how many groups and nesting would be
expected?

-
Beena Emerson

--
View this message in context:
http://postgresql.nabble.com/Support-for-N-synchronous-standby-servers-take-2-tp5849384p5857516.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.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] Fillfactor for GIN indexes

2015-07-10 Thread Michael Paquier

On Fri, Jul 10, 2015 at 7:13 PM, Alexander Korotkov aekorot...@gmail.com
wrote:

 On Fri, Jul 10, 2015 at 4:54 AM, Michael Paquier 
 michael.paqu...@gmail.com wrote:



 On Thu, Jul 9, 2015 at 10:33 PM, Alexander Korotkov wrote:
  [...]

 + /* Caclculate max data size on page according to fillfactor */
 s/Caclculate/Calculate

 When creating a simple gin index, I am seeing that GinGetMaxDataSize gets
 called with ginEntryInsert:
   * frame #0: 0x00010a49d72e
 postgres`GinGetMaxDataSize(index=0x000114168378, isBuild='\x01') + 14
 at gindatapage.c:436
 frame #1: 0x00010a49ecbe
 postgres`createPostingTree(index=0x000114168378,
 items=0x000114a9c038, nitems=35772, buildStats=0x7fff55787350) +
 302 at gindatapage.c:1754
 frame #2: 0x00010a493423
 postgres`buildFreshLeafTuple(ginstate=0x7fff55784d90, attnum=1,
 key=2105441, category='\0', items=0x000114a9c038, nitem=35772,
 buildStats=0x7fff55787350) + 339 at gininsert.c:158
 frame #3: 0x00010a492f84
 postgres`ginEntryInsert(ginstate=0x7fff55784d90, attnum=1, key=2105441,
 category='\0', items=0x000114a9c038, nitem=35772,
 buildStats=0x7fff55787350) + 916 at gininsert.c:228

 And as far as I can see GinGetMaxDataSize uses isBuild:
 + int
 + GinGetMaxDataSize(Relation index, bool isBuild)
 + {
 + int fillfactor, result;
 +
 + /* Grab option value which affects only index build */
 + if (!isBuild)
 However isBuild is not set in this code path, see
 http://www.postgresql.org/message-id/cab7npqsc4vq9mhkqm_yvafcteho-iuy8skbxydnmgnai1xn...@mail.gmail.com
 where I reported the problem. So this code is somewhat broken, no? I am
 attaching to this email the patch in question that should be applied on top
 of Alexander's latest version.


 I didn't get what is problem. Was this stacktrace during index build? If
 so, GinGetMaxDataSize really should get isBuild='\x01'. It is set by
 following line

 maxdatasize = GinGetMaxDataSize(index, buildStats ? true: false);


Yeah, I just find confusing that we actually rely on the fact that
buildStats is NULL or not here instead of passing directly the isBuild flag
of GinBtree for example. But that's a point of detail..
-- 
Michael

Re: [HACKERS] WAL logging problem in 9.4.3?

On 2015-07-10 13:38:50 +0300, Heikki Linnakangas wrote:
 In the long-term, I'd like to refactor this whole thing so that we never
 WAL-log any operations on a relation that's created in the same transaction
 (when wal_level=minimal). Instead, at COMMIT, we'd fsync() the relation, or
 if it's smaller than some threshold, WAL-log the contents of the whole file
 at that point. That would move all that
 more-difficult-than-it-seems-at-first-glance logic from COPY and indexam's
 to a central location, and it would allow the same optimization for all
 operations, not just COPY. But that probably isn't feasible to backpatch.

I don't think that's really realistic until we have a buffer manager
that lets you efficiently scan for all pages of a relation :(


-- 
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] Re: Removing SSL renegotiation (Was: Should we back-patch SSL renegotiation fixes?)

On 2015-07-01 23:32:23 -0400, Noah Misch wrote:
 We'd need to be triply confident that we know better than the DBA before
 removing flexibility in back branches.
 +1 for just changing the default.

I think we do. But I also think that I pretty clearly lost this
argument, so let's just change the default.

Is anybody willing to work on this?


-- 
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] Fillfactor for GIN indexes


On 07/10/2015 01:13 PM, Alexander Korotkov wrote:

On Fri, Jul 10, 2015 at 4:54 AM, Michael Paquier michael.paqu...@gmail.com
wrote:

+ #define GIN_MIN_FILLFACTOR20
+ #define GIN_DEFAULT_FILLFACTOR90
I am still worrying about using a default fillfactor at 90, as we did a
lot of promotion about compression of GIN indexes in 9.4. Feel free to
ignore this comment if you think that 90 is a good default. The difference
is visibly in order of MBs even for large indexes still, this is changing
the default of 9.4 and 9.5.


On the other side, it's unclear why should we have fillfactor distinct from
btree..


Good question. One argument is because the items on a GIN posting page 
are much smaller (2-3 bytes typically) than index tuples in a B-tree 
page (32 bytes or more). By a rough calculation, in the 10% free space 
you leave in an index page, which is about 800 bytes, you can insert at 
most 25 or so tuples. In the same amount of free space in a GIN page, 
you can fit hundreds items. The fillfactor is particularly useful to 
avoid splitting a lot pages after creating a new index, when you do a 
few random updates.


Another argument is that for the typical use cases of GIN, tuples are 
not updated as often as with B-tree indexes.


A third argument is that before this patch, we always packed the index 
as full as possible (i.e. fillfactor=100), and we want to avoid 
changing the default so much.


I'm not sure any of those arguments are very strong, but my gut feeling 
is that 90 might indeed be too small. Perhaps set the default to 95..


I'm curious why the minimum in the patch is 20, while the minimum for 
b-tree is 10, though. I don't see any particularly good reason for that.


- Heikki



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

Re: [HACKERS] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

2015-07-10 Thread David Rowley

On 10 July 2015 at 21:40, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote:

 To save cycles, I modified create_foreignscan_plan so that it detects
 whether any system columns are requested if scanning a base relation.
 Also, I revised other code there a little bit.

 For ExecInitForeignScan, I simplified the code there to determine the
 scan tuple type, whith seems to me complex beyound necessity.  Maybe
 that might be nitpicking, though.


I just glanced at this and noticed that the method for determining if
there's any system columns could be made a bit nicer.

/* Now, are any system columns requested from rel? */
scan_plan-fsSystemCol = false;
for (i = FirstLowInvalidHeapAttributeNumber + 1; i  0; i++)
{
if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
{
scan_plan-fsSystemCol = true;
break;
}
}

I think could just be written as:
 /* Now, are any system columns requested from rel? */
if (!bms_is_empty(attrs_used) 
bms_next_member(attrs_used, -1)  -FirstLowInvalidHeapAttributeNumber)
scan_plan-fsSystemCol = true;
else
scan_plan-fsSystemCol = false;

I know you didn't change this, but just thought I'd mention it while
there's an opportunity to fix it.

Regards

David Rowley

--
 David Rowley   http://www.2ndQuadrant.com/
http://www.2ndquadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services

Re: [HACKERS] WIP: Enhanced ALTER OPERATOR


On 07/06/2015 07:21 PM, Uriy Zhuravlev wrote:

Hello hackers.

This is the fifth version of the patch (the fourth was unsuccessful :)).
I added documentation and was held a small refactoring.


I edited the formatting of the syntax page a bit, and came up with this:

ALTER OPERATOR name ( { left_type | NONE } , { right_type | NONE } )
SET ( {  RESTRICT = { res_proc | NULL }
   | JOIN = { join_proc | NULL }
 } [, ... ] )

A couple of minor issues with the syntax itself:

* I think it'd be better to use NONE instead of NULL above, to remove 
the estimator. It seems inconsistent when we've used NONE to mean 
missing earlier in the same statement.


* The way you check for the NULL in OperatorUpd is wrong. It cannot 
distinguish between a quoted null, meaning a function called null, 
and a NULL, meaning no function. (You can argue that you're just asking 
for trouble if you name any function null, but we're careful to deal 
with that correctly everywhere else.) You don't have the information 
about quoting once you leave the parser, so you'll have to distinguish 
those in the grammar.


Attached is a new version of your patch, rebased over current master, 
and the docs formatting fixes.


- Heikki

diff --git a/doc/src/sgml/ref/alter_operator.sgml b/doc/src/sgml/ref/alter_operator.sgml
index 8a7af50..b8c353f 100644
--- a/doc/src/sgml/ref/alter_operator.sgml
+++ b/doc/src/sgml/ref/alter_operator.sgml
@@ -26,6 +26,11 @@ ALTER OPERATOR replaceablename/replaceable ( { replaceableleft_type/repla
 
 ALTER OPERATOR replaceablename/replaceable ( { replaceableleft_type/replaceable | NONE } , { replaceableright_type/replaceable | NONE } )
 SET SCHEMA replaceablenew_schema/replaceable
+
+ALTER OPERATOR replaceablename/replaceable ( { replaceableleft_type/replaceable | NONE } , { replaceableright_type/replaceable | NONE } )
+SET ( {  RESTRICT = { replaceable class=parameterres_proc/replaceable | NONE }
+   | JOIN = { replaceable class=parameterjoin_proc/replaceable | NONE }
+ } [, ... ] )
 /synopsis
  /refsynopsisdiv
 
@@ -34,8 +39,7 @@ ALTER OPERATOR replaceablename/replaceable ( { replaceableleft_type/repla
 
   para
commandALTER OPERATOR/command changes the definition of
-   an operator.  The only currently available functionality is to change the
-   owner of the operator.
+   an operator.
   /para
 
   para
@@ -98,6 +102,25 @@ ALTER OPERATOR replaceablename/replaceable ( { replaceableleft_type/repla
  /para
 /listitem
/varlistentry
+
+   varlistentry
+ termreplaceable class=parameterres_proc/replaceable/term
+ listitem
+   para
+ The restriction selectivity estimator function for this operator; write NONE to remove existing selectivity estimator.
+   /para
+  /listitem
+   /varlistentry
+
+   varlistentry
+ termreplaceable class=parameterjoin_proc/replaceable/term
+ listitem
+   para
+ The join selectivity estimator function for this operator; write NONE to remove existing selectivity estimator.
+   /para
+ /listitem
+   /varlistentry
+
   /variablelist
  /refsect1
 
@@ -109,6 +132,13 @@ ALTER OPERATOR replaceablename/replaceable ( { replaceableleft_type/repla
 programlisting
 ALTER OPERATOR @@ (text, text) OWNER TO joe;
 /programlisting/para
+
+  para
+Change the restriction and join selectivity estimator function of a custom operator literala  b/literal for type typeint[]/type:
+programlisting
+ALTER OPERATOR  (_int4, _int4) SET (RESTRICT = _int_contsel, JOIN = _int_contjoinsel);
+/programlisting/para
+
  /refsect1
 
  refsect1
diff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c
index 072f530..b981a44 100644
--- a/src/backend/catalog/pg_operator.c
+++ b/src/backend/catalog/pg_operator.c
@@ -28,8 +28,10 @@
 #include catalog/pg_operator.h
 #include catalog/pg_proc.h
 #include catalog/pg_type.h
+#include commands/defrem.h
 #include miscadmin.h
 #include parser/parse_oper.h
+#include parser/parse_func.h
 #include utils/acl.h
 #include utils/builtins.h
 #include utils/lsyscache.h
@@ -53,7 +55,7 @@ static Oid OperatorShellMake(const char *operatorName,
   Oid leftTypeId,
   Oid rightTypeId);
 
-static void OperatorUpd(Oid baseId, Oid commId, Oid negId);
+static void ShellOperatorUpd(Oid baseId, Oid commId, Oid negId);
 
 static Oid get_other_operator(List *otherOp,
    Oid otherLeftTypeId, Oid otherRightTypeId,
@@ -563,7 +565,7 @@ OperatorCreate(const char *operatorName,
 		commutatorId = operatorObjectId;
 
 	if (OidIsValid(commutatorId) || OidIsValid(negatorId))
-		OperatorUpd(operatorObjectId, commutatorId, negatorId);
+		ShellOperatorUpd(operatorObjectId, commutatorId, negatorId);
 
 	return address;
 }
@@ -633,7 +635,7 @@ get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
 }
 
 /*
- * OperatorUpd
+ * ShellOperatorUpd
  *
  *	For a given operator, look up its negator and commutator operators.
  *	If they are defined, but their

Re: [HACKERS] Fillfactor for GIN indexes

2015-07-10 Thread Alexander Korotkov

On Fri, Jul 10, 2015 at 4:54 AM, Michael Paquier michael.paqu...@gmail.com
wrote:



 On Thu, Jul 9, 2015 at 10:33 PM, Alexander Korotkov wrote:
  [...]

 + /* Caclculate max data size on page according to fillfactor */
 s/Caclculate/Calculate

 When creating a simple gin index, I am seeing that GinGetMaxDataSize gets
 called with ginEntryInsert:
   * frame #0: 0x00010a49d72e
 postgres`GinGetMaxDataSize(index=0x000114168378, isBuild='\x01') + 14
 at gindatapage.c:436
 frame #1: 0x00010a49ecbe
 postgres`createPostingTree(index=0x000114168378,
 items=0x000114a9c038, nitems=35772, buildStats=0x7fff55787350) +
 302 at gindatapage.c:1754
 frame #2: 0x00010a493423
 postgres`buildFreshLeafTuple(ginstate=0x7fff55784d90, attnum=1,
 key=2105441, category='\0', items=0x000114a9c038, nitem=35772,
 buildStats=0x7fff55787350) + 339 at gininsert.c:158
 frame #3: 0x00010a492f84
 postgres`ginEntryInsert(ginstate=0x7fff55784d90, attnum=1, key=2105441,
 category='\0', items=0x000114a9c038, nitem=35772,
 buildStats=0x7fff55787350) + 916 at gininsert.c:228

 And as far as I can see GinGetMaxDataSize uses isBuild:
 + int
 + GinGetMaxDataSize(Relation index, bool isBuild)
 + {
 + int fillfactor, result;
 +
 + /* Grab option value which affects only index build */
 + if (!isBuild)
 However isBuild is not set in this code path, see
 http://www.postgresql.org/message-id/cab7npqsc4vq9mhkqm_yvafcteho-iuy8skbxydnmgnai1xn...@mail.gmail.com
 where I reported the problem. So this code is somewhat broken, no? I am
 attaching to this email the patch in question that should be applied on top
 of Alexander's latest version.


I didn't get what is problem. Was this stacktrace during index build? If
so, GinGetMaxDataSize really should get isBuild='\x01'. It is set by
following line

maxdatasize = GinGetMaxDataSize(index, buildStats ? true: false);


 + #define GIN_MIN_FILLFACTOR20
 + #define GIN_DEFAULT_FILLFACTOR90
 I am still worrying about using a default fillfactor at 90, as we did a
 lot of promotion about compression of GIN indexes in 9.4. Feel free to
 ignore this comment if you think that 90 is a good default. The difference
 is visibly in order of MBs even for large indexes still, this is changing
 the default of 9.4 and 9.5.


On the other side, it's unclear why should we have fillfactor distinct from
btree..

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

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

Hi,

pg_dump dumps security labels on databases. Which makes sense. The
problem is that they're dumped including the database name.

Which means that if you dump a database and restore it into a
differently named one you'll either get a failure because the database
does not exist, or worse you'll update the label of the wrong database.

So I think we need CURRENT_DATABASE (or similar) support for security
labels on databases.

I won't have time to do anything about this anytime soon, but I think we
should fix that at some point.  Shall I put this on the todo? Or do we
want to create an 'open items' page that's not major version specific?

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

forgotten attachment

Regards

Pavel

2015-07-10 14:34 GMT+02:00 Pavel Stehule pavel.steh...@gmail.com:

 Hi

 I am sending review of this patch:

 1. I reread a previous discussion and almost all are for this patch (me
 too)

 2. I have to fix a typo in hstore_io.c function (update attached), other
 (patching, regress tests) without problems

 My objections:

 1. comments - missing comment for some basic API, basic fields like
 key_scalar and similar
 2. why you did indirect call via JsonOutContext?

 What is benefit

 dst.value(dst, (Datum) 0, JSONTYPE_NULL, InvalidOid, InvalidOid, false);

 instead

 json_out_value(dst, )

 ? Is it necessary?

 3. if it should be used everywhere, then in EXPLAIN statement too.

 Regards

 Pavel


 2015-07-10 6:31 GMT+02:00 Pavel Stehule pavel.steh...@gmail.com:



 2015-07-03 12:27 GMT+02:00 Heikki Linnakangas hlinn...@iki.fi:

 On 05/27/2015 09:51 PM, Andrew Dunstan wrote:


 On 05/27/2015 02:37 PM, Robert Haas wrote:

 On Tue, May 26, 2015 at 2:50 AM, Shulgin, Oleksandr
 oleksandr.shul...@zalando.de wrote:

 Is it reasonable to add this patch to CommitFest now?

 It's always reasonable to add a patch to the CommitFest if you would
 like for it to be reviewed and avoid having it get forgotten about.
 There seems to be some disagreement about whether we want this, but
 don't let that stop you from adding it to the next CommitFest.


 I'm not dead set against it either. When I have time I will take a
 closer look.


 Andrew, will you have the time to review this? Please add yourself as
 reviewer in the commitfest app if you do.

 My 2 cents is that I agree with your initial reaction: This is a lot of
 infrastructure and generalizing things, for little benefit. Let's change
 the current code where we generate JSON to be consistent with whitespace,
 and call it a day.


 I am  thinking so it is not bad idea. This code can enforce uniform
 format, and it can check if produced value is correct. It can be used in
 our code, it can be used by extension's developers.

 This patch is not small, but really new lines are not too much.

 I'll do review today.

 Regards

 Pavel




 - Heikki


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




diff --git a/contrib/hstore/hstore_io.c b/contrib/hstore/hstore_io.c
new file mode 100644
index 7d89867..0ca223f
*** a/contrib/hstore/hstore_io.c
--- b/contrib/hstore/hstore_io.c
*** hstore_to_json_loose(PG_FUNCTION_ARGS)
*** 1241,1286 
  	int			count = HS_COUNT(in);
  	char	   *base = STRPTR(in);
  	HEntry	   *entries = ARRPTR(in);
! 	StringInfoData tmp,
! dst;
  
  	if (count == 0)
  		PG_RETURN_TEXT_P(cstring_to_text_with_len({}, 2));
  
  	initStringInfo(tmp);
! 	initStringInfo(dst);
! 
! 	appendStringInfoChar(dst, '{');
  
  	for (i = 0; i  count; i++)
  	{
  		resetStringInfo(tmp);
  		appendBinaryStringInfo(tmp, HS_KEY(entries, base, i), HS_KEYLEN(entries, i));
! 		escape_json(dst, tmp.data);
! 		appendStringInfoString(dst, : );
  		if (HS_VALISNULL(entries, i))
! 			appendStringInfoString(dst, null);
  		/* guess that values of 't' or 'f' are booleans */
  		else if (HS_VALLEN(entries, i) == 1  *(HS_VAL(entries, base, i)) == 't')
! 			appendStringInfoString(dst, true);
  		else if (HS_VALLEN(entries, i) == 1  *(HS_VAL(entries, base, i)) == 'f')
! 			appendStringInfoString(dst, false);
  		else
  		{
  			resetStringInfo(tmp);
  			appendBinaryStringInfo(tmp, HS_VAL(entries, base, i), HS_VALLEN(entries, i));
  			if (IsValidJsonNumber(tmp.data, tmp.len))
! appendBinaryStringInfo(dst, tmp.data, tmp.len);
  			else
! escape_json(dst, tmp.data);
  		}
- 
- 		if (i + 1 != count)
- 			appendStringInfoString(dst, , );
  	}
- 	appendStringInfoChar(dst, '}');
  
! 	PG_RETURN_TEXT_P(cstring_to_text(dst.data));
  }
  
  PG_FUNCTION_INFO_V1(hstore_to_json);
--- 1241,1289 
  	int			count = HS_COUNT(in);
  	char	   *base = STRPTR(in);
  	HEntry	   *entries = ARRPTR(in);
! 	StringInfoData	tmp;
! 	JsonOutContext	dst;
  
  	if (count == 0)
  		PG_RETURN_TEXT_P(cstring_to_text_with_len({}, 2));
  
  	initStringInfo(tmp);
! 	json_out_init_context(dst, JSON_OUT_USE_SPACES);
! 	dst.object_start(dst);
  
  	for (i = 0; i  count; i++)
  	{
  		resetStringInfo(tmp);
  		appendBinaryStringInfo(tmp, HS_KEY(entries, base, i), HS_KEYLEN(entries, i));
! 		json_out_cstring(dst, tmp.data, true);
! 
  		if (HS_VALISNULL(entries, i))
! 			dst.value(dst, (Datum) 0, JSONTYPE_NULL, InvalidOid, InvalidOid, false);
! 
  		/* guess that values of 't' or 'f' are booleans */
  		else if (HS_VALLEN(entries, i) == 1  *(HS_VAL(entries, base, i)) == 't')
! 			dst.value(dst, BoolGetDatum(true), JSONTYPE_BOOL,
! 	  InvalidOid, InvalidOid, false);
! 
  		else if (HS_VALLEN(entries, i) == 1  *(HS_VAL(entries, base, i)) == 'f')
! 			dst.value(dst, BoolGetDatum(false), JSONTYPE_BOOL,
! 	  InvalidOid, InvalidOid, false);
  		else

Re: [HACKERS] WAL logging problem in 9.4.3?

2015-07-10 Thread Fujii Masao

On Fri, Jul 10, 2015 at 2:27 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Fujii Masao masao.fu...@gmail.com writes:
 On Tue, Jul 7, 2015 at 12:49 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 One idea I had was to allow the COPY optimization only if the heap file is
 physically zero-length at the time the COPY starts.

 This seems not helpful for the case where TRUNCATE is executed
 before COPY. No?

 Huh?  The heap file would be zero length in that case.

 So, if COPY is executed multiple times at the same transaction,
 only first COPY can be optimized?

 This is true, and I don't think we should care, especially not if we're
 going to take risks of incorrect behavior in order to optimize that
 third-order case.  The fact that we're dealing with this bug at all should
 remind us that this stuff is harder than it looks.  I want a simple,
 reliable, back-patchable fix, and I do not believe that what you are
 suggesting would be any of those.

Maybe I'm missing something. But I start wondering why TRUNCATE
and INSERT (or even all the operations on the table created at
the current transaction) need to be WAL-logged while COPY can be
optimized. If no WAL records are generated on that table, the problem
we're talking about seems not to occur. Also this seems safe and
doesn't degrade the performance of data loading. Thought?

Regards,

-- 
Fujii Masao


-- 
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] creating extension including dependencies


On 07/09/2015 07:05 PM, Petr Jelinek wrote:

On 2015-07-07 15:41, Andres Freund wrote:

On 2015-07-07 22:36:29 +0900, Fujii Masao wrote:

On Mon, Jun 15, 2015 at 7:50 AM, Petr Jelinek p...@2ndquadrant.com wrote:

Hi,

I am getting tired installing manually required extensions manually. I was
wondering if we might want to add option to CREATE SEQUENCE that would allow
automatic creation of the extensions required by the extension that is being
installed by the user.


I'm wondering how much helpful this feature is. Because, even if we can save
some steps for CREATE EXTENSION by using the feature, we still need to
manually find out, download and install all the extensions that the target
extension depends on. So isn't it better to implement the tool like yum, i.e.,
which performs all those steps almost automatically, rather than the proposed
feature? Maybe it's outside PostgreSQL core.


That doesn't seem to make much sense to me. Something like yum can't
install everything in all relevant databases. Sure, yum will be used to
install dependencies between extensions on the filesystem level.

At the minimum I'd like to see that CREATE EXTENSION foo; would install
install extension 'bar' if foo dependended on 'bar' if CASCADE is
specified. Right now we always error out saying that the dependency on
'bar' is not fullfilled - not particularly helpful.


That's what the proposed patch does (with slightly different syntax but
syntax is something that can be changed easily).


This seems quite reasonable, but I have to ask: How many extensions are 
there out there that depend on another extension? Off the top of my 
head, I can't think of any..


- Heikki


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

Re: [HACKERS] more RLS oversights

2015-07-10 Thread Joe Conway

On 07/03/2015 10:03 AM, Noah Misch wrote:
 (1) CreatePolicy() and AlterPolicy() omit to call assign_expr_collations() on
 the node trees.  Test case:
 
 begin;
 set row_security = force;
 create table t (c) as values ('bar'::text);
 create policy p on t using (c  ('foo'::text COLLATE C));
 alter table t enable row level security;
 table pg_policy;  -- note :inputcollid 0
 select * from t;  -- ERROR:  could not determine which collation ...
 rollback;

The attached fixes this issue for me, but I am unsure whether we really
need/want the regression test. Given the recent push to increase test
coverage maybe so. Any objections?

Joe


diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 11efc9f..7232983 100644
*** a/src/backend/commands/policy.c
--- b/src/backend/commands/policy.c
*** CreatePolicy(CreatePolicyStmt *stmt)
*** 538,543 
--- 538,547 
  		   EXPR_KIND_WHERE,
  		   POLICY);
  
+ 	/* Fix up collation information */
+ 	assign_expr_collations(qual_pstate, qual);
+ 	assign_expr_collations(with_check_pstate, with_check_qual);
+ 
  	/* Open pg_policy catalog */
  	pg_policy_rel = heap_open(PolicyRelationId, RowExclusiveLock);
  
*** AlterPolicy(AlterPolicyStmt *stmt)
*** 681,686 
--- 685,693 
  	EXPR_KIND_WHERE,
  	POLICY);
  
+ 		/* Fix up collation information */
+ 		assign_expr_collations(qual_pstate, qual);
+ 
  		qual_parse_rtable = qual_pstate-p_rtable;
  		free_parsestate(qual_pstate);
  	}
*** AlterPolicy(AlterPolicyStmt *stmt)
*** 701,706 
--- 708,716 
  			   EXPR_KIND_WHERE,
  			   POLICY);
  
+ 		/* Fix up collation information */
+ 		assign_expr_collations(with_check_pstate, with_check_qual);
+ 
  		with_check_parse_rtable = with_check_pstate-p_rtable;
  		free_parsestate(with_check_pstate);
  	}
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
index 4073c1b..eabfd93 100644
*** a/src/test/regress/expected/rowsecurity.out
--- b/src/test/regress/expected/rowsecurity.out
*** ERROR:  permission denied for relation c
*** 2730,2735 
--- 2730,2756 
  RESET SESSION AUTHORIZATION;
  DROP TABLE copy_t;
  --
+ -- Collation support
+ --
+ BEGIN;
+ SET row_security = force;
+ CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
+ CREATE POLICY coll_p ON coll_t USING (c  ('foo'::text COLLATE C));
+ ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
+ SELECT (string_to_array(polqual, ':'))[7] AS inputcollid FROM pg_policy WHERE polrelid = 'coll_t'::regclass;
+inputcollid
+ --
+  inputcollid 950 
+ (1 row)
+ 
+ SELECT * FROM coll_t;
+   c  
+ -
+  bar
+ (1 row)
+ 
+ ROLLBACK;
+ --
  -- Clean up objects
  --
  RESET SESSION AUTHORIZATION;
diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
index fdd9b89..782824a 100644
*** a/src/test/regress/sql/rowsecurity.sql
--- b/src/test/regress/sql/rowsecurity.sql
*** RESET SESSION AUTHORIZATION;
*** 1088,1093 
--- 1088,1105 
  DROP TABLE copy_t;
  
  --
+ -- Collation support
+ --
+ BEGIN;
+ SET row_security = force;
+ CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
+ CREATE POLICY coll_p ON coll_t USING (c  ('foo'::text COLLATE C));
+ ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
+ SELECT (string_to_array(polqual, ':'))[7] AS inputcollid FROM pg_policy WHERE polrelid = 'coll_t'::regclass;
+ SELECT * FROM coll_t;
+ ROLLBACK;
+ 
+ --
  -- Clean up objects
  --
  RESET SESSION AUTHORIZATION;

-- 
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_upgrade + Extensions

2015-07-10 Thread Andrew Dunstan

On Fri, Jul 10, 2015 at 5:05 PM, David E. Wheeler da...@justatheory.com
wrote:

 On Jul 10, 2015, at 11:32 AM, Smitha Pamujula 
 smitha.pamuj...@iovation.com wrote:


  Your installation references loadable libraries that are missing from the
  new installation.  You can add these libraries to the new installation,
  or remove the functions using them from the old installation.  A list of
  problem libraries is in the file:
  loadable_libraries.txt
 
  Failure, exiting
  [postgres@pdxdvrptsrd04 ~]$ cat loadable_libraries.txt
  Could not load library json_build
  ERROR:  could not access file json_build: No such file or directory

 So you drop the json_build extension before upgrading, but pg_upgrade
 still complains that it’s missing? That seems odd.




Are you sure the extension was uninstalled from every database in the
cluster? This seems likely to occur when you forgot to uninstall it from
some database (e.g. template1)

cheers

andrew

Re: [HACKERS] more RLS oversights

2015-07-10 Thread Noah Misch

On Fri, Jul 10, 2015 at 03:08:53PM -0700, Joe Conway wrote:
 On 07/03/2015 10:03 AM, Noah Misch wrote:
  (1) CreatePolicy() and AlterPolicy() omit to call assign_expr_collations() 
  on
  the node trees.

 The attached fixes this issue for me, but I am unsure whether we really
 need/want the regression test. Given the recent push to increase test
 coverage maybe so.

I wouldn't remove the test from your 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] Freeze avoidance of very large table.

2015-07-10 Thread Jim Nasby


On 7/10/15 4:46 AM, Simon Riggs wrote:

On 10 July 2015 at 09:49, Sawada Masahiko sawada.m...@gmail.com
mailto:sawada.m...@gmail.com wrote:


 It is a minor thing, but if there is no other use for this fourth
 bit-space, it seems a shame to waste it when there is some use for it.  
I
 haven't looked at the code around this area to know how hard it would be 
to
 implement the setting and clearing of the bit.

I think so too, we would be able to use unused fourth status of bits
efficiently.
Should I include these improvement into this patch?
This topic should be discussed on another thread after this feature is
committed, I think.


The impossible state acts as a diagnostic check for us to ensure the
bitmap is not itself corrupt.

-1 for using it for another purpose.


AFAICS empty page is only interesting for vacuum truncate, which is a 
very short-term thing. It would be better to find a way to handle that 
differently.


In any case, that should definitely be a separate discussion from this 
patch.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Data in Trouble? Get it in Treble! http://BlueTreble.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] polymorphic types - enforce casting to most common type automatically

2015-07-10 23:19 GMT+02:00 Pavel Stehule pavel.steh...@gmail.com:

 Hi

 2015-07-10 18:43 GMT+02:00 Tom Lane t...@sss.pgh.pa.us:

 Pavel Stehule pavel.steh...@gmail.com writes:
  now a functions with more than one polymorphic arguments are relative
  fragile due missing casting to most common type. Some our functions
 like
  coalesce can do it, so it is surprising for our users.

  our custom polymorphic function foo(anyelement, anyelement) working
 well for
  foo(10,20) or foo(10.1, 20.1), but not for foo(10, 20.1)

  I am thinking, so we can add a searching most common type stage without
  breaking to backing compatibility.

  What do you think about it?

 I see nobody's replied to this, still, so ...

 I think this is simply a bad idea, for a couple of reasons:

 1. It will reduce predictability of type resolution.


 I don't think - same mechanism we use - it doesn't introduce some new.



 2. It will greatly increase the risk of getting ambiguous function call
 failures, because of adding more possible ways to match the same call.
 (The argument that we'd not break backwards compatibility is thus bogus.)


 Maybe I not described well my idea.

 This can generate new conflicts only when new behave will be different
 than old behave. And different old behave is not possible - it fails on
 error now. So there is possible, with this patch, some queries can fail on
 conflict, but this code fails on function doesn't exists now. So if there
 is some possibility of breaking compatibility, then one error can be
 replaced by different error. It is known best practice to don't mix
 polymorphic parameters and function overloading.

 Why I need it - the motivation, why I returned to this topic is issue
 https://github.com/orafce/orafce/issues/17 and some questions about same
 topic on stackoverflow.

 There is workaround with any type - but I have to repeat lot of work
 what core analyzer can do, and the code in extension is longer. And I have
 to write extension in C.


It worse - workaround with any isn't good enough for implementation NVL
function - because I cannot to change result type inside function.

I know, so you dislike parser/analyzer hook, but is there any other
possibility? Some hint in pg_class?

Regards

Pavel





 Worth noting for onlookers is that the submitted patch seems to be using
 UNION-style rules to determine a common type for anyelement arguments,
 not just counting the most common type among the arguments as you might
 think from the subject.  But that doesn't make things any better.


 it is related to only polymorphic types.


 An example of what would presumably happen if we adopted this sort of rule
 (I've not checked whether the patch as written does this, but it would
 logically follow) is that appending a float to an integer array would
 cause the whole array to be silently promoted to float, with attendant
 possible loss of precision for existing array elements.


 it is based on select_common_type() - so it is use only available implicit
 casts.


 That does not
 seem to me to satisfy the principle of least astonishment.  Related,
 even more astonishing behaviors could ensue from type promotion in
 anyrange situations, eg range_contains_elem(anyrange,anyelement).
 So I think it's just as well that we make people write a cast to show
 what they mean in such cases.


 The polymorphic parameters create much bigger space - if somebody needs to
 less variability, then he doesn't use polymorphic params.

 I understand to some situation, when we prefer strict work with
 polymorphic parameters - theoretically we can introduce new option that
 enforce it.


 In fact, if you discount cases involving anyarray and anyrange, we do not
 have *any* built-in functions for which this patch would do anything,
 except for the three-argument forms of lead() and lag(), where I think it
 would be rather astonishing to let the default-value argument control the
 result type, anyway.  This leaves me feeling dubious both about the actual
 scope of the use-case for such a change, and about whether use the UNION
 rules would be a sensible heuristic even if we wanted to do something.
 There seem to be too many cases where it's not a great idea to put all the
 arguments on exactly equal footing for deciding what common type to
 choose.


 Very common problem of polymorphic parameters is mix of numeric and
 integers as parameters. It is one known gotcha - and I am trying to solve
 it.

 Regards

 Pavel



 So I'm inclined to mark this patch as Rejected.

 regards, tom lane

Re: [HACKERS] [PATCH] Generalized JSON output functions

2015-07-10 16:16 GMT+02:00 Shulgin, Oleksandr oleksandr.shul...@zalando.de
:

 On Fri, Jul 10, 2015 at 4:04 PM, Pavel Stehule pavel.steh...@gmail.com
 wrote:



 2. why you did indirect call via JsonOutContext?

 What is benefit

 dst.value(dst, (Datum) 0, JSONTYPE_NULL, InvalidOid, InvalidOid,
 false);

 instead

 json_out_value(dst, )


 For consistency.  Even though we initialize the output context
 ourselves, there might be some code introduced between
 json_out_init_context() and dst.value() calls that replaces some of the
 callbacks, and then there would be a difference.


 with this consistency? I didn't see this style everywhere in Postgres?
 Isn't it premature optimization?


 Well, one could call it premature pessimization due to dynamic call
 overhead.

 IMO, the fact that json_out_init_context() sets the value callback to
 json_out_value is an implementation detail, the other parts of code should
 not rely on.  And for the Explain output, there definitely going to be
 *some* code between context initialization and output callbacks: these are
 done in a number of different functions.


Again - it is necessary? Postgres still use modular code, not OOP code. I
can understand the using of this technique, when I need a possibility to
change behave. But these function are used for printing JSON, not printing
any others.

Pavel



 --
 Alex

Re: [HACKERS] LANGUAGE sql functions don't use the custom plan logic

Andres Freund and...@anarazel.de writes:
 I recently was forcfully reminded that sql functions don't use the
 plancache.c infrastructure. I was sort of aware of that, but I didn't
 think far enough ahead that that also implies we'll not use custom plans
 for statements with parameters when it'd be helpful.

There's a whole lot of ways in which the current implementation of
SQL-language functions leaves things to be desired.  What did you
run into exactly?

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] LANGUAGE sql functions don't use the custom plan logic

On 2015-07-10 09:52:30 -0400, Tom Lane wrote:
 There's a whole lot of ways in which the current implementation of
 SQL-language functions leaves things to be desired.

Yea.

 What did you run into exactly?

A generic plan was used on a partitioned table.  Normally the function
could be inlined so that wasn't noticeable, but in one case it
wasn't... It's confusing if all invocations of the same function are
reasonably fast, but one, with the numerically same values passed down,
isn't.

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] Freeze avoidance of very large table.

2015-07-10 Thread Sawada Masahiko

On Fri, Jul 10, 2015 at 10:43 PM, Fujii Masao masao.fu...@gmail.com wrote:
 On Fri, Jul 10, 2015 at 2:41 PM, Sawada Masahiko sawada.m...@gmail.com 
 wrote:
 On Fri, Jul 10, 2015 at 3:05 AM, Sawada Masahiko sawada.m...@gmail.com 
 wrote:

 Also something for pg_upgrade is also not yet.

 TODO
 - Test case for this feature
 - pg_upgrade support.


 I had forgotten to change the fork name of visibility map to vfm.
 Attached latest v7 patch.
 Please review it.

 The compilation failed on my machine...

 gcc -Wall -Wmissing-prototypes -Wpointer-arith
 -Wdeclaration-after-statement -Wendif-labels
 -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
 -fwrapv -g -O0 -I../../../../src/include -D_GNU_SOURCE   -c -o
 visibilitymap.o visibilitymap.c
 make[4]: *** No rule to make target `heapfuncs.o', needed by
 `objfiles.txt'.  Stop.
 make[4]: *** Waiting for unfinished jobs
 ( echo src/backend/access/index/genam.o
 src/backend/access/index/indexam.o ) objfiles.txt
 make[4]: Leaving directory `/home/postgres/pgsql/git/src/backend/access/index'
 gcc -Wall -Wmissing-prototypes -Wpointer-arith
 -Wdeclaration-after-statement -Wendif-labels
 -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
 -fwrapv -g -O0 -I../../../src/include -D_GNU_SOURCE   -c -o
 tablespace.o tablespace.c
 gcc -Wall -Wmissing-prototypes -Wpointer-arith
 -Wdeclaration-after-statement -Wendif-labels
 -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
 -fwrapv -g -O0 -I../../../src/include -D_GNU_SOURCE   -c -o
 instrument.o instrument.c
 make[4]: Leaving directory `/home/postgres/pgsql/git/src/backend/access/heap'
 make[3]: *** [heap-recursive] Error 2
 make[3]: Leaving directory `/home/postgres/pgsql/git/src/backend/access'
 make[2]: *** [access-recursive] Error 2
 make[2]: *** Waiting for unfinished jobs


Oops, I had forgotten to add new file heapfuncs.c.
Latest patch is attached.

Regards,

--
Sawada Masahiko


000_add_frozen_bit_into_visibilitymap_v8.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] creating extension including dependencies

Andres Freund and...@anarazel.de writes:
 On July 10, 2015 4:16:59 PM GMT+02:00, Tom Lane t...@sss.pgh.pa.us wrote:
 Would that propagate down through multiple levels of CASCADE?  (Although
 I'm not sure it would be sensible for a non-relocatable extension to
 depend on a relocatable one, so maybe the need doesn't arise in
 practice.)

 I'd day so. I was thinking it'd adding a flag that allows to pass a
 schema to a non relocatable extension. That'd then be passed down. I
 agree that it's unlikely to be used often.

Yeah, I was visualizing it slightly differently, as a separate
internal-only option schema_if_needed, but it works out to the
same thing either way.

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] LANGUAGE sql functions don't use the custom plan logic

Hi,

I recently was forcfully reminded that sql functions don't use the
plancache.c infrastructure. I was sort of aware of that, but I didn't
think far enough ahead that that also implies we'll not use custom plans
for statements with parameters when it'd be helpful.

That's imo quite the trap. Should we document it somewhere?

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

Hi

I am sending review of this patch:

1. I reread a previous discussion and almost all are for this patch (me too)

2. I have to fix a typo in hstore_io.c function (update attached), other
(patching, regress tests) without problems

My objections:

1. comments - missing comment for some basic API, basic fields like
key_scalar and similar
2. why you did indirect call via JsonOutContext?

What is benefit

dst.value(dst, (Datum) 0, JSONTYPE_NULL, InvalidOid, InvalidOid, false);

instead

json_out_value(dst, )

? Is it necessary?

3. if it should be used everywhere, then in EXPLAIN statement too.

Regards

Pavel


2015-07-10 6:31 GMT+02:00 Pavel Stehule pavel.steh...@gmail.com:



 2015-07-03 12:27 GMT+02:00 Heikki Linnakangas hlinn...@iki.fi:

 On 05/27/2015 09:51 PM, Andrew Dunstan wrote:


 On 05/27/2015 02:37 PM, Robert Haas wrote:

 On Tue, May 26, 2015 at 2:50 AM, Shulgin, Oleksandr
 oleksandr.shul...@zalando.de wrote:

 Is it reasonable to add this patch to CommitFest now?

 It's always reasonable to add a patch to the CommitFest if you would
 like for it to be reviewed and avoid having it get forgotten about.
 There seems to be some disagreement about whether we want this, but
 don't let that stop you from adding it to the next CommitFest.


 I'm not dead set against it either. When I have time I will take a
 closer look.


 Andrew, will you have the time to review this? Please add yourself as
 reviewer in the commitfest app if you do.

 My 2 cents is that I agree with your initial reaction: This is a lot of
 infrastructure and generalizing things, for little benefit. Let's change
 the current code where we generate JSON to be consistent with whitespace,
 and call it a day.


 I am  thinking so it is not bad idea. This code can enforce uniform
 format, and it can check if produced value is correct. It can be used in
 our code, it can be used by extension's developers.

 This patch is not small, but really new lines are not too much.

 I'll do review today.

 Regards

 Pavel




 - Heikki


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

Re: ctidscan as an example of custom-scan (Re: [HACKERS] [v9.5] Custom Plan API)

On 07/02/2015 08:21 AM, Kouhei Kaigai wrote:
 Folks,
 
 Moved this patch to next CF 2015-02 because of lack of review(ers).

 Do we still need this patch as contrib module?
 It was originally required it as example of custom-scan interface last
 summer, however, here was no strong requirement after that, then, it
 was bumped to v9.6 development cycle.
 
 If somebody still needs it, I'll rebase and adjust the patch towards
 the latest custom-scan interface. However, I cannot be motivated for
 the feature nobody wants.

Robert, can you weigh in on this? Do we currently have anything in the
tree that tests the Custom Scan interface? If not, would this be helpful
for that purpose?

- Heikki



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

Re: [HACKERS] Freeze avoidance of very large table.

2015-07-10 Thread Fujii Masao

On Fri, Jul 10, 2015 at 2:41 PM, Sawada Masahiko sawada.m...@gmail.com wrote:
 On Fri, Jul 10, 2015 at 3:05 AM, Sawada Masahiko sawada.m...@gmail.com 
 wrote:

 Also something for pg_upgrade is also not yet.

 TODO
 - Test case for this feature
 - pg_upgrade support.


 I had forgotten to change the fork name of visibility map to vfm.
 Attached latest v7 patch.
 Please review it.

The compilation failed on my machine...

gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -g -O0 -I../../../../src/include -D_GNU_SOURCE   -c -o
visibilitymap.o visibilitymap.c
make[4]: *** No rule to make target `heapfuncs.o', needed by
`objfiles.txt'.  Stop.
make[4]: *** Waiting for unfinished jobs
( echo src/backend/access/index/genam.o
src/backend/access/index/indexam.o ) objfiles.txt
make[4]: Leaving directory `/home/postgres/pgsql/git/src/backend/access/index'
gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -g -O0 -I../../../src/include -D_GNU_SOURCE   -c -o
tablespace.o tablespace.c
gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -g -O0 -I../../../src/include -D_GNU_SOURCE   -c -o
instrument.o instrument.c
make[4]: Leaving directory `/home/postgres/pgsql/git/src/backend/access/heap'
make[3]: *** [heap-recursive] Error 2
make[3]: Leaving directory `/home/postgres/pgsql/git/src/backend/access'
make[2]: *** [access-recursive] Error 2
make[2]: *** Waiting for unfinished jobs

Regards,

-- 
Fujii Masao


-- 
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] creating extension including dependencies

Michael Paquier michael.paqu...@gmail.com writes:
 On Fri, Jul 10, 2015 at 10:09 PM, Heikki Linnakangas hlinn...@iki.fi
 This seems quite reasonable, but I have to ask: How many extensions are
 there out there that depend on another extension? Off the top of my head, I
 can't think of any..

 With transforms there are such dependencies, and there are 3 in contrib/:
 hstore_plperl, hstore_plpython and ltree_plpython.

It's reasonable to expect that such cases will become more common as the
extension community matures.  It wasn't something we especially had to
worry about in the initial implementation of extensions, but it seems
totally worthwhile to me to add it now.

FWIW, I agree with using CASCADE to signal a request to create required
extension(s) automatically.

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] creating extension including dependencies

On July 10, 2015 4:16:59 PM GMT+02:00, Tom Lane t...@sss.pgh.pa.us wrote:
Andres Freund and...@anarazel.de writes:
 I think we should copy the SCHEMA option here and document that we
use
 the same schema. But it needs to be done in a way that doesn't error
out
 if the extension is not relocatable...

Would that propagate down through multiple levels of CASCADE? 
(Although
I'm not sure it would be sensible for a non-relocatable extension to
depend on a relocatable one, so maybe the need doesn't arise in
practice.)

I'd day so. I was thinking it'd adding a flag that allows to pass a schema to a 
non relocatable extension. That'd then be passed down. I agree that it's 
unlikely to be used often.

Andres

--- 
Please excuse brevity and formatting - I am writing this on my mobile phone.


-- 
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] creating extension including dependencies

On 2015-06-15 00:50:08 +0200, Petr Jelinek wrote:
 + /* Create and execute new CREATE EXTENSION 
 statement. */
 + ces = makeNode(CreateExtensionStmt);
 + ces-extname = curreq;
 + ces-if_not_exists = false;
 + parents = lappend(list_copy(recursive_parents), 
 stmt-extname);
 + ces-options = 
 list_make1(makeDefElem(recursive,
 + 
   (Node *) parents));
 + CreateExtension(ces);

I think we should copy the SCHEMA option here and document that we use
the same schema. But it needs to be done in a way that doesn't error out
if the extension is not relocatable...


-- 
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] creating extension including dependencies

2015-07-10 Thread Vladimir Borodin


 10 июля 2015 г., в 16:09, Heikki Linnakangas hlinn...@iki.fi написал(а):
 
 On 07/09/2015 07:05 PM, Petr Jelinek wrote:
 On 2015-07-07 15:41, Andres Freund wrote:
 On 2015-07-07 22:36:29 +0900, Fujii Masao wrote:
 On Mon, Jun 15, 2015 at 7:50 AM, Petr Jelinek p...@2ndquadrant.com wrote:
 Hi,
 
 I am getting tired installing manually required extensions manually. I was
 wondering if we might want to add option to CREATE SEQUENCE that would 
 allow
 automatic creation of the extensions required by the extension that is 
 being
 installed by the user.
 
 I'm wondering how much helpful this feature is. Because, even if we can 
 save
 some steps for CREATE EXTENSION by using the feature, we still need to
 manually find out, download and install all the extensions that the target
 extension depends on. So isn't it better to implement the tool like yum, 
 i.e.,
 which performs all those steps almost automatically, rather than the 
 proposed
 feature? Maybe it's outside PostgreSQL core.
 
 That doesn't seem to make much sense to me. Something like yum can't
 install everything in all relevant databases. Sure, yum will be used to
 install dependencies between extensions on the filesystem level.
 
 At the minimum I'd like to see that CREATE EXTENSION foo; would install
 install extension 'bar' if foo dependended on 'bar' if CASCADE is
 specified. Right now we always error out saying that the dependency on
 'bar' is not fullfilled - not particularly helpful.
 
 That's what the proposed patch does (with slightly different syntax but
 syntax is something that can be changed easily).
 
 This seems quite reasonable, but I have to ask: How many extensions are there 
 out there that depend on another extension? Off the top of my head, I can't 
 think of any..

pg_stat_kcache depends on pg_stat_statements, for example.

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

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

Re: [HACKERS] WIP: Enhanced ALTER OPERATOR

2015-07-10 Thread Uriy Zhuravlev

Hello.

On Friday 10 July 2015 15:46:35 you wrote:
 * I think it'd be better to use NONE instead of NULL above, to remove
 the estimator. It seems inconsistent when we've used NONE to mean
 missing earlier in the same statement.

Ok, you right. 

 * The way you check for the NULL in OperatorUpd is wrong. It cannot
 distinguish between a quoted null, meaning a function called null,
 and a NULL, meaning no function. 
With none has a similar problem. I need to correct the grammar.

 
 Attached is a new version of your patch, rebased over current master,
 and the docs formatting fixes.

Thanks. I hope to send the new patch on Monday. 

-- 
Uriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


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

Re: [HACKERS] [PATCH] Generalized JSON output functions

2015-07-10 Thread Shulgin, Oleksandr


 2015-07-10 14:34 GMT+02:00 Pavel Stehule pavel.steh...@gmail.com:

 Hi

 I am sending review of this patch:

 1. I reread a previous discussion and almost all are for this patch (me
 too)

 2. I have to fix a typo in hstore_io.c function (update attached), other
 (patching, regress tests) without problems

 My objections:

 1. comments - missing comment for some basic API, basic fields like
 key_scalar and similar


I thought it was pretty obvious from the code, because it's sort of the
only source for docs on the subject right now.  Should we add proper
documentation section, this would have been documented for sure.


 2. why you did indirect call via JsonOutContext?

 What is benefit

 dst.value(dst, (Datum) 0, JSONTYPE_NULL, InvalidOid, InvalidOid, false);

 instead

 json_out_value(dst, )


For consistency.  Even though we initialize the output context ourselves,
there might be some code introduced between json_out_init_context() and
dst.value() calls that replaces some of the callbacks, and then there would
be a difference.


 3. if it should be used everywhere, then in EXPLAIN statement too.


Ahh.. good catch.  I'll have a look on this now.

Thanks for the review!

--
Alex

Re: [HACKERS] [PATCH] Generalized JSON output functions

2015-07-10 15:57 GMT+02:00 Shulgin, Oleksandr oleksandr.shul...@zalando.de
:

 2015-07-10 14:34 GMT+02:00 Pavel Stehule pavel.steh...@gmail.com:

 Hi

 I am sending review of this patch:

 1. I reread a previous discussion and almost all are for this patch (me
 too)

 2. I have to fix a typo in hstore_io.c function (update attached), other
 (patching, regress tests) without problems

 My objections:

 1. comments - missing comment for some basic API, basic fields like
 key_scalar and similar


 I thought it was pretty obvious from the code, because it's sort of the
 only source for docs on the subject right now.  Should we add proper
 documentation section, this would have been documented for sure.


 2. why you did indirect call via JsonOutContext?

 What is benefit

 dst.value(dst, (Datum) 0, JSONTYPE_NULL, InvalidOid, InvalidOid, false);

 instead

 json_out_value(dst, )


 For consistency.  Even though we initialize the output context ourselves,
 there might be some code introduced between json_out_init_context() and
 dst.value() calls that replaces some of the callbacks, and then there would
 be a difference.


with this consistency? I didn't see this style everywhere in Postgres?
Isn't it premature optimization?




 3. if it should be used everywhere, then in EXPLAIN statement too.


 Ahh.. good catch.  I'll have a look on this now.

 Thanks for the review!

 --
 Alex

Re: [HACKERS] [PATCH] Generalized JSON output functions

2015-07-10 Thread Shulgin, Oleksandr

On Fri, Jul 10, 2015 at 4:04 PM, Pavel Stehule pavel.steh...@gmail.com
wrote:



 2. why you did indirect call via JsonOutContext?

 What is benefit

 dst.value(dst, (Datum) 0, JSONTYPE_NULL, InvalidOid, InvalidOid,
 false);

 instead

 json_out_value(dst, )


 For consistency.  Even though we initialize the output context ourselves,
 there might be some code introduced between json_out_init_context() and
 dst.value() calls that replaces some of the callbacks, and then there would
 be a difference.


 with this consistency? I didn't see this style everywhere in Postgres?
 Isn't it premature optimization?


Well, one could call it premature pessimization due to dynamic call
overhead.

IMO, the fact that json_out_init_context() sets the value callback to
json_out_value is an implementation detail, the other parts of code should
not rely on.  And for the Explain output, there definitely going to be
*some* code between context initialization and output callbacks: these are
done in a number of different functions.

--
Alex

Re: [HACKERS] creating extension including dependencies

Andres Freund and...@anarazel.de writes:
 I think we should copy the SCHEMA option here and document that we use
 the same schema. But it needs to be done in a way that doesn't error out
 if the extension is not relocatable...

Would that propagate down through multiple levels of CASCADE?  (Although
I'm not sure it would be sensible for a non-relocatable extension to
depend on a relocatable one, so maybe the need doesn't arise in practice.)

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] pg_upgrade + Ubuntu

2015-07-10 Thread Joshua D. Drake



Hackers,

Simple problem (I think):

9.4 version of pg_upgrade said:

 /usr/lib/postgresql/9.4/bin/pg_ctl -w -l pg_upgrade_server.log -D 
/var/lib/postgresql/9.4/main -o -p 9400 -b -c synchronous_commit=off 
-c fsync=off -c full_page_writes=off  -c listen_addresses='' -c 
unix_socket_permissions=0700 -c 
unix_socket_directories='/var/lib/postgresql' start


postgres cannot access the server configuration file 
/var/lib/postgresql/9.4/main/postgresql.conf: No such file or directory


The issue is Debian/Ubuntu etc... don't have a postgresql.conf in the 
cluster. They keep it separately in /etc/postgresql.


Could we get a flag that allows us to specifically point to where the 
conf filesare?


JD
--
Command Prompt, Inc. - http://www.commandprompt.com/  503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Announcing I'm offended is basically telling the world you can't
control your own emotions, so everyone else should do it for you.


--
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_upgrade + Extensions

David E. Wheeler da...@justatheory.com writes:
 My co-workers tell me that pg_upgrade told them to drop the colnames and
 hostname extensions before upgrading from 9.3 to 9.4.

Really?  I see nothing in the source code that would print any such
advice.

There *is* a check on whether .so libraries used by the source
installation exist in the destination one.  But the preferred way to
deal with that type of complaint is to install the needed libraries
(in the destination's lib/ folder).  You shouldn't have to drop anything
as long as you have a copy of the extension that works for the new PG
version.

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] pg_upgrade + Ubuntu

2015-07-10 Thread Mike Blackwell

Does pg_config show the correct location?  If so, perhaps pg_upgrade could
get the .conf location the same way rather than requiring a command line
option.

__
*Mike Blackwell | Technical Analyst, Distribution Services/Rollout
Management | RR Donnelley*
1750 Wallace Ave | St Charles, IL 60174-3401
Office: 630.313.7818
mike.blackw...@rrd.com
http://www.rrdonnelley.com


http://www.rrdonnelley.com/
* mike.blackw...@rrd.com*

On Fri, Jul 10, 2015 at 12:40 PM, Joshua D. Drake j...@commandprompt.com
wrote:


 Hackers,

 Simple problem (I think):

 9.4 version of pg_upgrade said:

  /usr/lib/postgresql/9.4/bin/pg_ctl -w -l pg_upgrade_server.log -D
 /var/lib/postgresql/9.4/main -o -p 9400 -b -c synchronous_commit=off -c
 fsync=off -c full_page_writes=off  -c listen_addresses='' -c
 unix_socket_permissions=0700 -c
 unix_socket_directories='/var/lib/postgresql' start

 postgres cannot access the server configuration file
 /var/lib/postgresql/9.4/main/postgresql.conf: No such file or directory

 The issue is Debian/Ubuntu etc... don't have a postgresql.conf in the
 cluster. They keep it separately in /etc/postgresql.

 Could we get a flag that allows us to specifically point to where the conf
 filesare?

 JD
 --
 Command Prompt, Inc. - http://www.commandprompt.com/  503-667-4564
 PostgreSQL Centered full stack support, consulting and development.
 Announcing I'm offended is basically telling the world you can't
 control your own emotions, so everyone else should do it for you.


 --
 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_upgrade + Extensions

2015-07-10 Thread Smitha Pamujula

Tom,

I just tested and yes that worked. Once we have the new library for the
hostname, pg_upgrade is not complaining about the hostname extension.

Another thing we found is this. We needed to drop json_build extension
before the upgrade. However the upgrade fails with the following error and
the way to resolve it is to install json_build94 library. Any ideas why
this might be?

/usr/pgsql-9.4/bin/pg_upgrade --check --link
...

Your installation references loadable libraries that are missing from the
new installation.  You can add these libraries to the new installation,
or remove the functions using them from the old installation.  A list of
problem libraries is in the file:
loadable_libraries.txt

Failure, exiting
[postgres@pdxdvrptsrd04 ~]$ cat loadable_libraries.txt
Could not load library json_build
ERROR:  could not access file json_build: No such file or directory

Thanks
Smitha


On Fri, Jul 10, 2015 at 10:59 AM, Tom Lane t...@sss.pgh.pa.us wrote:

 David E. Wheeler da...@justatheory.com writes:
  My co-workers tell me that pg_upgrade told them to drop the colnames and
  hostname extensions before upgrading from 9.3 to 9.4.

 Really?  I see nothing in the source code that would print any such
 advice.

 There *is* a check on whether .so libraries used by the source
 installation exist in the destination one.  But the preferred way to
 deal with that type of complaint is to install the needed libraries
 (in the destination's lib/ folder).  You shouldn't have to drop anything
 as long as you have a copy of the extension that works for the new PG
 version.

 regards, tom lane




-- 
Smitha Pamujula
Database Administrator // The Watch Woman

Direct: 503.943.6764
Mobile: 503.290.6214 // Twitter: iovation
www.iovation.com

[HACKERS] pg_upgrade + Extensions

2015-07-10 Thread David E. Wheeler

Hackers,

My co-workers tell me that pg_upgrade told them to drop the colnames and 
hostname extensions before upgrading from 9.3 to 9.4. Fortunately, Postgres had 
 not recorded any dependencies on functions from these extensions (not sure why 
not, since we do user them, but for the moment grateful), so it wasn’t a big 
deal to drop them and then add them back after finishing the upgrade. But 
frankly I don’t understand why this was necessary. It’s true that they’re C 
extensions with shared libraries, but there are separate .so files for the 9.3 
and 9.4 installs.

Would there be a way to convince pg_upgrade that extensions don’t need to be 
dropped before upgrading?

Thanks,

David



smime.p7s
Description: S/MIME cryptographic signature

Re: [HACKERS] RFC: replace pg_stat_activity.waiting with something more descriptive

2015-07-10 Thread Alexander Korotkov

On Fri, Jun 26, 2015 at 6:39 AM, Robert Haas robertmh...@gmail.com wrote:

 On Thu, Jun 25, 2015 at 9:23 AM, Peter Eisentraut pete...@gmx.net wrote:
  On 6/22/15 1:37 PM, Robert Haas wrote:
  Currently, the only time we report a process as waiting is when it is
  waiting for a heavyweight lock.  I'd like to make that somewhat more
  fine-grained, by reporting the type of heavyweight lock it's awaiting
  (relation, relation extension, transaction, etc.).  Also, I'd like to
  report when we're waiting for a lwlock, and report either the specific
  fixed lwlock for which we are waiting, or else the type of lock (lock
  manager lock, buffer content lock, etc.) for locks of which there is
  more than one.  I'm less sure about this next part, but I think we
  might also want to report ourselves as waiting when we are doing an OS
  read or an OS write, because it's pretty common for people to think
  that a PostgreSQL bug is to blame when in fact it's the operating
  system that isn't servicing our I/O requests very quickly.
 
  Could that also cover waiting on network?

 Possibly.  My approach requires that the number of wait states be kept
 relatively small, ideally fitting in a single byte.  And it also
 requires that we insert pgstat_report_waiting() calls around the thing
 that is notionally blocking.  So, if there are a small number of
 places in the code where we do network I/O, we could stick those calls
 around those places, and this would work just fine.  But if a foreign
 data wrapper, or any other piece of code, does network I/O - or any
 other blocking operation - without calling pgstat_report_waiting(), we
 just won't know about it.


Idea of fitting wait information into single byte and avoid both locking
and atomic operations is attractive.
But how long we can go with it?
Could DBA make some conclusion by single querying of pg_stat_activity or
double querying?
In order to make a conclusion about system load one have to run daemon or
background worker which is continuously sampling current wait events.
Sampling current wait event with high rate also gives some overhead to the
system as well as locking or atomic operations.
Checking if backend is stuck isn't easy as well. If you don't expose how
long last wait event continues it's hard to distinguish getting stuck on
particular lock and high concurrency on that lock type.

I can propose following:

1) Expose more information about current lock to user. For instance, having
duration of current wait event, user can determine if backend is getting
stuck on particular event without sampling.
2) Accumulate per backend statistics about each wait event type: number of
occurrences and total duration. With this statistics user can identify
system bottlenecks again without sampling.

Number #2 will be provided as a separate patch.
Number #1 require different concurrency model. ldus will extract it from
waits monitoring patch shortly.

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

Re: [HACKERS] polymorphic types - enforce casting to most common type automatically

Pavel Stehule pavel.steh...@gmail.com writes:
 now a functions with more than one polymorphic arguments are relative
 fragile due missing casting to most common type. Some our functions like
 coalesce can do it, so it is surprising for our users.

 our custom polymorphic function foo(anyelement, anyelement) working well for
 foo(10,20) or foo(10.1, 20.1), but not for foo(10, 20.1)

 I am thinking, so we can add a searching most common type stage without
 breaking to backing compatibility.

 What do you think about it?

I see nobody's replied to this, still, so ...

I think this is simply a bad idea, for a couple of reasons:

1. It will reduce predictability of type resolution.

2. It will greatly increase the risk of getting ambiguous function call
failures, because of adding more possible ways to match the same call.
(The argument that we'd not break backwards compatibility is thus bogus.)

Worth noting for onlookers is that the submitted patch seems to be using
UNION-style rules to determine a common type for anyelement arguments,
not just counting the most common type among the arguments as you might
think from the subject.  But that doesn't make things any better.

An example of what would presumably happen if we adopted this sort of rule
(I've not checked whether the patch as written does this, but it would
logically follow) is that appending a float to an integer array would
cause the whole array to be silently promoted to float, with attendant
possible loss of precision for existing array elements.  That does not
seem to me to satisfy the principle of least astonishment.  Related,
even more astonishing behaviors could ensue from type promotion in
anyrange situations, eg range_contains_elem(anyrange,anyelement).
So I think it's just as well that we make people write a cast to show
what they mean in such cases.

In fact, if you discount cases involving anyarray and anyrange, we do not
have *any* built-in functions for which this patch would do anything,
except for the three-argument forms of lead() and lag(), where I think it
would be rather astonishing to let the default-value argument control the
result type, anyway.  This leaves me feeling dubious both about the actual
scope of the use-case for such a change, and about whether use the UNION
rules would be a sensible heuristic even if we wanted to do something.
There seem to be too many cases where it's not a great idea to put all the
arguments on exactly equal footing for deciding what common type to
choose.

So I'm inclined to mark this patch as Rejected.

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] pg_upgrade + Ubuntu

2015-07-10 Thread Alvaro Herrera

Joshua D. Drake wrote:
 
 On 07/10/2015 11:01 AM, Mike Blackwell wrote:
 Does pg_config show the correct location?  If so, perhaps pg_upgrade
 could get the .conf location the same way rather than requiring a
 command line option.
 
 Good idea but:
 
 postgres@ly19:~$ pg_config
 You need to install postgresql-server-dev-X.Y for building a server-side
 extension or libpq-dev for building a client-side application.
 
 Which is worse having to install yet another package or having a command
 line option?

It seems to me that this is a Debian packaging issue, not an upstream
issue, isn't it?  If you want to fix the problem in this way, then
surely whatever package contains pg_upgrade should also contain
pg_config.

Why are you not using pg_upgradecluster anyway?  There's a mode to use
pg_upgrade there.

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


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

Re: [HACKERS] pg_upgrade + Ubuntu

2015-07-10 Thread Mike Blackwell

On Fri, Jul 10, 2015 at 2:10 PM, Alvaro Herrera alvhe...@2ndquadrant.com
 wrote:

 Joshua D. Drake wrote:
 
  On 07/10/2015 11:01 AM, Mike Blackwell wrote:
  Does pg_config show the correct location?
  Good idea but:
 
  postgres@ly19:~$ pg_config
  You need to install postgresql-server-dev-X.Y for building a server-side
  extension or libpq-dev for building a client-side application.
 
  Which is worse having to install yet another package or having a command
  line option?

 It seems to me that this is a Debian packaging issue, not an upstream
 issue, isn't it?  If you want to fix the problem in this way, then
 surely whatever package contains pg_upgrade should also contain
 pg_config.


Indeed.  An interesting packaging choice.  I'd think it belongs to an admin
category along with pg_upgrade, pg_dump, etc., rather than a development
package.  Surely it could be useful for admin scripts?

__
*Mike Blackwell | Technical Analyst, Distribution Services/Rollout
Management | RR Donnelley*
1750 Wallace Ave | St Charles, IL 60174-3401
Office: 630.313.7818
mike.blackw...@rrd.com
http://www.rrdonnelley.com

 http://www.rrdonnelley.com/

Re: [HACKERS] pg_upgrade + Ubuntu

2015-07-10 Thread Joshua D. Drake



On 07/10/2015 12:10 PM, Alvaro Herrera wrote:


It seems to me that this is a Debian packaging issue, not an upstream
issue, isn't it?  If you want to fix the problem in this way, then
surely whatever package contains pg_upgrade should also contain
pg_config.

Why are you not using pg_upgradecluster anyway?  There's a mode to use
pg_upgrade there.



Ignorance. I still remember pg_upgradecluster from the pg_dumpall days.

JD


--
Command Prompt, Inc. - http://www.commandprompt.com/  503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Announcing I'm offended is basically telling the world you can't
control your own emotions, so everyone else should do it for you.


--
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: index-only scans with partial indexes

Tomas Vondra tomas.von...@2ndquadrant.com writes:
 currently partial indexes end up not using index only scans in most 
 cases, because check_index_only() is overly conservative, as explained 
 in this comment:
 ...

 I've done a bunch of tests, and I do see small (hardly noticeable) 
 increase in planning time with long list of WHERE clauses, because all 
 those need to be checked against the index predicate. Not sure if this 
 is what's meant by 'quite expensive' in the comment. Moreover, this was 
 more than compensated by the IOS benefits (even with everything in RAM).

 But maybe it's possible to fix that somehow? For example, we're 
 certainly doing those checks elsewhere when deciding which clauses need 
 to be evaluated at run-time, so maybe we could cache that somehow?

The key problem here is that you're doing those proofs vastly earlier than
before, for indexes that might not get used at all in the final plan.
If you do some tests with multiple partial indexes you will probably see
a bigger planning-time penalty.

Perhaps we should bite the bullet and do it anyway, but I'm pretty
suspicious of any claim that the planning cost is minimal.

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] polymorphic types - enforce casting to most common type automatically

Hi

2015-07-10 18:43 GMT+02:00 Tom Lane t...@sss.pgh.pa.us:

 Pavel Stehule pavel.steh...@gmail.com writes:
  now a functions with more than one polymorphic arguments are relative
  fragile due missing casting to most common type. Some our functions
 like
  coalesce can do it, so it is surprising for our users.

  our custom polymorphic function foo(anyelement, anyelement) working well
 for
  foo(10,20) or foo(10.1, 20.1), but not for foo(10, 20.1)

  I am thinking, so we can add a searching most common type stage without
  breaking to backing compatibility.

  What do you think about it?

 I see nobody's replied to this, still, so ...

 I think this is simply a bad idea, for a couple of reasons:

 1. It will reduce predictability of type resolution.


I don't think - same mechanism we use - it doesn't introduce some new.



 2. It will greatly increase the risk of getting ambiguous function call
 failures, because of adding more possible ways to match the same call.
 (The argument that we'd not break backwards compatibility is thus bogus.)


Maybe I not described well my idea.

This can generate new conflicts only when new behave will be different than
old behave. And different old behave is not possible - it fails on error
now. So there is possible, with this patch, some queries can fail on
conflict, but this code fails on function doesn't exists now. So if there
is some possibility of breaking compatibility, then one error can be
replaced by different error. It is known best practice to don't mix
polymorphic parameters and function overloading.

Why I need it - the motivation, why I returned to this topic is issue
https://github.com/orafce/orafce/issues/17 and some questions about same
topic on stackoverflow.

There is workaround with any type - but I have to repeat lot of work what
core analyzer can do, and the code in extension is longer. And I have to
write extension in C.



 Worth noting for onlookers is that the submitted patch seems to be using
 UNION-style rules to determine a common type for anyelement arguments,
 not just counting the most common type among the arguments as you might
 think from the subject.  But that doesn't make things any better.


it is related to only polymorphic types.


 An example of what would presumably happen if we adopted this sort of rule
 (I've not checked whether the patch as written does this, but it would
 logically follow) is that appending a float to an integer array would
 cause the whole array to be silently promoted to float, with attendant
 possible loss of precision for existing array elements.


it is based on select_common_type() - so it is use only available implicit
casts.


 That does not
 seem to me to satisfy the principle of least astonishment.  Related,
 even more astonishing behaviors could ensue from type promotion in
 anyrange situations, eg range_contains_elem(anyrange,anyelement).
 So I think it's just as well that we make people write a cast to show
 what they mean in such cases.


The polymorphic parameters create much bigger space - if somebody needs to
less variability, then he doesn't use polymorphic params.

I understand to some situation, when we prefer strict work with polymorphic
parameters - theoretically we can introduce new option that enforce it.


 In fact, if you discount cases involving anyarray and anyrange, we do not
 have *any* built-in functions for which this patch would do anything,
 except for the three-argument forms of lead() and lag(), where I think it
 would be rather astonishing to let the default-value argument control the
 result type, anyway.  This leaves me feeling dubious both about the actual
 scope of the use-case for such a change, and about whether use the UNION
 rules would be a sensible heuristic even if we wanted to do something.
 There seem to be too many cases where it's not a great idea to put all the
 arguments on exactly equal footing for deciding what common type to
 choose.


Very common problem of polymorphic parameters is mix of numeric and
integers as parameters. It is one known gotcha - and I am trying to solve
it.

Regards

Pavel



 So I'm inclined to mark this patch as Rejected.

 regards, tom lane

Re: [HACKERS] pg_upgrade + Extensions

2015-07-10 Thread David E. Wheeler

On Jul 10, 2015, at 11:32 AM, Smitha Pamujula smitha.pamuj...@iovation.com 
wrote:

 I just tested and yes that worked. Once we have the new library for the 
 hostname, pg_upgrade is not complaining about the hostname extension. 

Great, thank you Smitha -- and Tom for the pointer.

 Your installation references loadable libraries that are missing from the
 new installation.  You can add these libraries to the new installation,
 or remove the functions using them from the old installation.  A list of
 problem libraries is in the file:
 loadable_libraries.txt
 
 Failure, exiting
 [postgres@pdxdvrptsrd04 ~]$ cat loadable_libraries.txt
 Could not load library json_build
 ERROR:  could not access file json_build: No such file or directory

So you drop the json_build extension before upgrading, but pg_upgrade still 
complains that it’s missing? That seems odd.

Best,

David



smime.p7s
Description: S/MIME cryptographic signature

Re: [HACKERS] Additional role attributes superuser review