Re: [HACKERS] Reduce pinning in btree indexes

[Andrew Gierth]

> "Kyotaro" == Kyotaro HORIGUCHI  writes:

 >> You might want to try running the same test, but after patching
 >> ExecSupportsMarkRestore to return false for index scans. This will
 >> cause the planner to insert a Materialize node to handle the
 >> mark/restore.

 Kyotaro> Mmm? The patch bt-nopin-v1.patch seems not contain the change
 Kyotaro> for ExecSupportMarkRestore and the very simple function remain
 Kyotaro> looking to return true for T_Index(Only)Scan after the patch
 Kyotaro> applied.

Right. I'm suggesting you change that, in order to determine what
performance cost, if any, would result from abandoning the idea of doing
mark/restore entirely.

-- 
Andrew (irc:RhodiumToad)


-- 
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] Merge compact/non compact commits, make aborts dynamically sized

[Michael Paquier]

On Wed, Feb 25, 2015 at 8:10 PM, Andres Freund  wrote:
> On 2015-02-24 20:51:42 +0200, Heikki Linnakangas wrote:
>> On 02/20/2015 05:21 PM, Andres Freund wrote:
>> >There's one bit that I'm not so sure about though: To avoid duplication
>> >I've added Parse(Commit/Abort)Record(), but unfortunately that has to be
>> >available both in front and backend code - so it's currently living in
>> >xactdesc.c. I think we can live with that, but it's certainly not
>> >pretty.
>>
>> Yeah, that's ugly. Why does frontend code need that? The old format
>> isn't exactly trivial for frontend code to decode either.
>
> pg_xlogdump outputs subxacts and such; I don't forsee other
> usages. Sure, we could copy the code around, but I think that's worse
> than having it in xactdesc.c. Needs a comment explaining why it's there
> if I haven't added one already.

FWIW, I think they would live better in frontend code for client applications.

That's a nice patch. +1 for merging them. Here are a couple of comments:

+/* Parse the WAL format of a xact abort into a easier to understand format. */
+void
+ParseCommitRecord(uint8 info, xl_xact_commit *xlrec,
xl_xact_parsed_commit *parsed)
I think that you mean here of "an xact commit", not abort.

+ * Emit, but don't insert, a abort record.
s/a abort/an abort/
XactEmitAbortRecord has some problems with tabs replaced by 4 spaces
at a couple of places.

+/* free opcode 0x70 */
+
+#define XLOG_XACT_OPMASK   0x70
There is a contradiction here, 0x70 is not free.

Regards,
-- 
Michael


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

Re: [HACKERS] Reduce pinning in btree indexes

[Kyotaro HORIGUCHI]

Hi,

At Fri, 27 Feb 2015 05:56:18 +, Andrew Gierth  
wrote in <874mq77vuu@news-spur.riddles.org.uk>
> > "Kyotaro" == Kyotaro HORIGUCHI  writes:
> 
>  Kyotaro> ammarkpos/amrestrpos are called in merge joins. By the steps
>  Kyotaro> shown below, I had 1M times of markpos and no restorepos for
>  Kyotaro> 1M result rows, and had 500k times of markpos and the same
>  Kyotaro> number of times of restorepos for 2M rows result by a bit
>  Kyotaro> different configuration. I suppose we can say that they are
>  Kyotaro> the worst case considering maskpos/restrpos. The call counts
>  Kyotaro> can be taken using the attached patch.
> 
> You might want to try running the same test, but after patching
> ExecSupportsMarkRestore to return false for index scans. This will cause
> the planner to insert a Materialize node to handle the mark/restore.

Mmm? The patch bt-nopin-v1.patch seems not contain the change for
ExecSupportMarkRestore and the very simple function remain
looking to return true for T_Index(Only)Scan after the patch
applied.

Explain shows that the plan does not involve materializing step
and addition to it, the counters I put into both btmarkpos() and
btrestrpos() showed that they are actually called during the
execution, like this, for both unpatched and patched.

| LOG:  markpos=100, restrpos=0
| STATEMENT:  EXPLAIN (ANALYZE,BUFFERS) SELECT t1.a, t2.a FROM t1 JOIN t2 on 
(t1.a = t2.a);

To make sure, I looked into btmarkpos and btrestrpos to confirm
that they really does the memcpy() we're talking about, then
recompiled whole the source tree.

> This way, you can get an idea of how much gain (if any) the direct
> support of mark/restore in the scan is actually providing.

Does "the scan" mean T_Material node? But no material in plan and
counters in bt*pos were incremented in fact as mentioned above..

Could you point out any other possible mistake in my thought?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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

Re: [HACKERS] New CF app deployment

[Stefan Kaltenbrunner]

On 02/26/2015 01:59 PM, Michael Paquier wrote:
> On Thu, Feb 26, 2015 at 9:15 PM, Asif Naeem  wrote:
>> This thread seems relevant, Please guide me to how can access older CF pages
>>> The MSVC portion of this fix got completely lost in the void:
>>> https://commitfest.postgresql.org/action/patch_view?id=1330
>>
>> Above link result in the following error i.e.
>>
>>> Not found
>>> The specified URL was not found.
>>
>> Please do let me know if I missed something. Thanks.
> 
> Try commitfest-old instead, that is where the past CF app stores its
> data, like that:
> https://commitfest-old.postgresql.org/action/patch_view?id=1330

hmm maybe we should have some sort of handler the redirects/reverse
proxies to the old commitfest app for this.



Stefan


-- 
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] Providing catalog view to pg_hba.conf file - Patch submission

[Haribabu Kommi]

On Sat, Feb 7, 2015 at 8:26 PM, Pavel Stehule  wrote:
> Hi
>
> I am sending a review of this patch.

Thanks for the review. sorry for the delay.

> 4. Regress tests
>
> test rules... FAILED  -- missing info about new  view

Thanks. Corrected.

> My objections:
>
> 1. data type for "database" field should be array of name or array of text.
>
> When name contains a comma, then this comma is not escaped
>
> currently: {omega,my stupid extremly, name2,my stupid name}
>
> expected: {"my stupid name",omega,"my stupid extremly, name2"}
>
> Same issue I see in "options" field

Changed including the user column also.

> 2. Reload is not enough for content refresh - logout is necessary
>
> I understand, why it is, but it is not very friendly, and can be very
> stressful. It should to work with last reloaded data.

At present the view data is populated from the variable "parsed_hba_lines"
which contains the already parsed lines of pg_hba.conf file.

During the reload, the hba entries are reloaded only in the postmaster process
and not in every backend, because of this reason the reloaded data is
not visible until
the session is disconnected and connected.

Instead of changing the SIGHUP behavior in the backend to load the hba entries
also, whenever the call is made to the view, the pg_hba.conf file is
parsed to show
the latest reloaded data in the view. This way it doesn't impact the
existing behavior
but it may impact the performance because of file load into memory every time.

Updated patch is attached. comments?

Regards,
Hari Babu
Fujitsu Australia


Catalog_view_to_HBA_settings_patch_V5.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] Performance improvement for joins where outer side is unique

[David Rowley]

On 3 February 2015 at 22:23, Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp> wrote:

> Hi, I had a look on this patch. Although I haven't understood
> whole the stuff and all of the related things, I will comment as
> possible.
>
>
Great, thank you for taking the time to look and review the patch.


> Performance:
>
> I looked on the performance gain this patch gives. For several
> on-memory joins, I had gains about 3% for merge join, 5% for hash
> join, and 10% for nestloop (@CentOS6), for simple 1-level joins
> with aggregation similar to what you mentioned in previous
> mail like this.
>
> =# SELECT count(*) FROM t1 JOIN t2 USING (id);
>
>  Of course, the gain would be trivial when returning many tuples,
> or scans go to disk.
>
>
That's true, but joins where rows are filtered after the join condition
will win here too:

For example select * from t1 inner join t2 on t1.id=t2.id where t1.value >
t2.value;

Also queries with a GROUP BY clause. (Since grouping is always performed
after the join :-( )

I haven't measured the loss by additional computation when the
> query is regarded as not a "single join".
>
>
I think this would be hard to measure, but likely if it is measurable then
you'd want to look at just planning time, rather than planning and
execution time.


>
> Explain representation:
>
> I don't like that the 'Unique Join:" occupies their own lines in
> the result of explain, moreover, it doesn't show the meaning
> clearly. What about the representation like the following? Or,
> It might not be necessary to appear there.
>
>Nested Loop ...
>  Output: 
>  -   Unique Jion: Yes
>-> Seq Scan on public.t2 (cost = ...
>  - -> Seq Scan on public.t1 (cost = 
>  + -> Seq Scan on public.t1 (unique) (cost = 
>
>
>
Yeah I'm not too big a fan of this either. I did at one evolution of the
patch I had "Unique Left Join" in the join node's line in the explain
output, but I hated that more and changed it just to be just in the VERBOSE
output, and after I did that I didn't want to change the join node's line
only when in verbose mode.  I do quite like that it's a separate item for
the XML and JSON explain output. That's perhaps quite useful when the
explain output must be processed by software.

I'm totally open for better ideas on names, but I just don't have any at
the moment.


> Coding:
>
> The style looks OK. Could applied on master.
> It looks to work as you expected for some cases.
>
> Random comments follow.
>
> - Looking specialjoin_is_unique_join, you seem to assume that
>   !delay_upper_joins is a sufficient condition for not being
>   "unique join".  The former simplly indicates that "don't
>   commute with upper OJs" and the latter seems to indicate that
>   "the RHS is unique", Could you tell me what kind of
>   relationship is there between them?
>

The rationalisation around that are from the (now changed) version of
join_is_removable(), where the code read:

/*
 * Must be a non-delaying left join to a single baserel, else we aren't
 * going to be able to do anything with it.
 */
if (sjinfo->jointype != JOIN_LEFT ||
sjinfo->delay_upper_joins)
return false;

I have to admit that I didn't go and investigate why delayed upper joins
cannot be removed by left join removal code, I really just assumed that
we're just unable to prove that a join to such a relation won't match more
than one outer side's row. I kept this to maintain that behaviour as I
assumed it was there for a good reason.



>
> - The naming "unique join" seems a bit obscure for me, but I
>   don't have no better idea:( However, the member name
>   "has_unique_join" seems to be better to be "is_unique_join".
>
>
Yeah, I agree with this. I just can't find something short enough that
means "based on the join condition, the inner side of the join will never
produce more than 1 row for any single outer row". Unique join was the best
I came up with. I'm totally open for better ideas.

I agree that is_unique_join is better than has_unique_join. I must have
just copied the has_eclass_joins struct member without thinking too hard
about it. I've now changed this in my local copy of the patch.

- eclassjoin_is_unique_join involves seemingly semi-exhaustive
>   scan on ec members for every element in joinlist. Even if not,
>   it might be thought to be a bit too large for the gain. Could
>   you do the equivelant things by more small code?
>
>
I'd imagine some very complex queries could have many equivalence classes
and members, though I hadn't really thought that this number would be so
great that processing here would suffer much. There's quite a few fast
paths out, like the test to ensure that both rels are mentioned
in ec_relids. Though for a query which is so complex to have a great number
of eclass' and members, likely there would be quite a few relations
involved and planning would be slower anyway. I'm not quite sure how else I
could write this and still find the unique join cases each time. We can't

Re: [HACKERS] Reduce pinning in btree indexes

[Andrew Gierth]

> "Kyotaro" == Kyotaro HORIGUCHI  writes:

 Kyotaro> ammarkpos/amrestrpos are called in merge joins. By the steps
 Kyotaro> shown below, I had 1M times of markpos and no restorepos for
 Kyotaro> 1M result rows, and had 500k times of markpos and the same
 Kyotaro> number of times of restorepos for 2M rows result by a bit
 Kyotaro> different configuration. I suppose we can say that they are
 Kyotaro> the worst case considering maskpos/restrpos. The call counts
 Kyotaro> can be taken using the attached patch.

You might want to try running the same test, but after patching
ExecSupportsMarkRestore to return false for index scans. This will cause
the planner to insert a Materialize node to handle the mark/restore.

This way, you can get an idea of how much gain (if any) the direct
support of mark/restore in the scan is actually providing.

-- 
Andrew (irc:RhodiumToad)


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

Re: [HACKERS] Reduce pinning in btree indexes

[Kyotaro HORIGUCHI]

Hello, I measured the performance of this patch considering
markpos/restorepos. The loss seems to be up to about 10%
unfortunately.

At Thu, 26 Feb 2015 17:49:23 + (UTC), Kevin Grittner  
wrote in <440831854.629116.1424972963735.javamail.ya...@mail.yahoo.com>
> Heikki Linnakangas  wrote:> On 02/15/2015 02:19 AM, 
> Kevin Grittner wrote:
> > Hmm. Did your test case actually exercise mark/restore? The memcpy()s 
> > are not that small. Especially if it's an index-only scan, you're 
> > copying a large portion of the page. Some scans call markpos on every 
> > tuple, so that could add up.
> 
> 
> I have results from the `make world` regression tests and a 48-hour
> customer test.  Unfortunately I don't know how heavily either of those
> exercise this code.  Do you have a suggestion for a way to test whether
> there is a serious regression for something approaching a "worst case"?

ammarkpos/amrestrpos are called in merge joins. By the steps
shown below, I had 1M times of markpos and no restorepos for 1M
result rows, and had 500k times of markpos and the same number of
times of restorepos for 2M rows result by a bit different
configuration. I suppose we can say that they are the worst case
considering maskpos/restrpos. The call counts can be taken using
the attached patch.

Both binaries ware compiled with -O2. shared_buffers=1GB and all
shared pages used in the query were hit when measuring.

The numbers were taken 12 times for each cofiguration and took
averages and stddevs of 10 numbers excluding best and worst.

Case 1. 500k markpos/restorepos for 2M result rows.

Index scan: The patched loses about 2%. (1.98%)
  master:  6166 ms (std dev = 3.2 ms)
  Patched: 6288 ms (std dev = 3.7 ms)

IndesOnly scan: The patches loses about 2%. (2.14%)
  master:  5946 ms (std dev = 5.0 ms)
  Patched: 6073 ms (std dev = 3.3 ms)

The patched version is slower by about 2%. Of course all of it is
not the effect of memcpy but I don't know the distribution.


Case 2. 1M markpos, no restorepos for 1M result rows.

IndesOnly scan: The patches loses about 10%.
  master:  3655 ms (std dev = 2.5 ms)
  Patched: 4038 ms (std dev = 2.6 ms)

The loss is about 10%. The case looks a bit impractical but
unfortunately the number might be unignorable. The distribution
of the loss is unknown, too.


regards,

===
CREATE TABLE t1 (a int);
CREATE TABLE t2 (a int);
delete from t1;
delete from t2;
-- This causes 1M times of markpos and no restorepos
INSERT INTO t1 (SELECT a FROM generate_series(0, 99) a);
INSERT INTO t2 (SELECT a FROM generate_series(0, 99) a);
-- This causes 500k times of markpos and the same number of restorepos
-- INSERT INTO t1 (SELECT a/2 FROM generate_series(0, 99) a);
-- INSERT INTO t2 (SELECT a/2 FROM generate_series(0, 99) a);
CREATE INDEX it1 ON t1 (a);
CREATE INDEX it2 ON t2 (a);
ANALYZE t1;
ANALYZE t1;
SET enable_seqscan to false;
SET enable_material to false;
SET enable_hashjoin to false;
SET enable_nestloop to false;
SET enable_indexonlyscan to false; -- omit this to do indexonly scan

EXPLAIN (ANALYZE) SELECT t1.a, t2.a FROM t1 JOIN t2 on (t1.a = t2.a);


  QUERY PLAN 
--
 Merge Join  (cost=2.83..322381.82 rows=3031231 width=8) (actual 
time=0.013..5193.566 rows=200 loops=1)
   Merge Cond: (t1.a = t2.a)
   ->  Index Scan using it1 on t1  (cost=0.43..65681.87 rows=167 width=4) 
(actual time=0.004..727.557 rows=100
oops=1)
   ->  Index Scan using it2 on t2  (cost=0.43..214642.89 rows=3031231 width=4) 
(actual time=0.004..1478.361 rows=1
 loops=1)
 Planning time: 25.585 ms
 Execution time: 6299.521 ms
(6 rows)

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 3af462b..04d6cec 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -543,15 +543,19 @@ btendscan(PG_FUNCTION_ARGS)
 	PG_RETURN_VOID();
 }
 
+
 /*
  *	btmarkpos() -- save current scan position
  */
+int hoge_markpos_count = 0;
+int hoge_restrpos_count = 0;
 Datum
 btmarkpos(PG_FUNCTION_ARGS)
 {
 	IndexScanDesc scan = (IndexScanDesc) PG_GETARG_POINTER(0);
 	BTScanOpaque so = (BTScanOpaque) scan->opaque;
 
+	hoge_markpos_count++;
 	/* we aren't holding any read locks, but gotta drop the pin */
 	if (BTScanPosIsValid(so->markPos))
 	{
@@ -585,7 +589,7 @@ btrestrpos(PG_FUNCTION_ARGS)
 {
 	IndexScanDesc scan = (IndexScanDesc) PG_GETARG_POINTER(0);
 	BTScanOpaque so = (BTScanOpaque) scan->opaque;
-
+	hoge_restrpos_count++;
 	/* Restore the marked positions of any array keys */
 	if (so->numArrayKeys)
 		_bt_restore_array_keys(scan);
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 33b172b..e7c1b99 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -258,14 +258,19 @@ standard_ExecutorStart(QueryDesc *queryDes

Re: [HACKERS] Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset

[Andrew Gierth]

> "Tom" == Tom Lane  writes:

 Tom> We've discussed doing $SUBJECT off and on for nearly ten years,
 Tom> the oldest thread I could find about it being here:
 Tom> 
http://www.postgresql.org/message-id/flat/1186435268.16321.37.ca...@dell.linuxdev.us.dell.com
 Tom> It's come up again every time we found another leak of dead child
 Tom> contexts, which happened twice last year for example.  And that
 Tom> patch I'm pushing for "expanded" out-of-line objects really needs
 Tom> this to become the default behavior anywhere that we can detoast
 Tom> objects.

So, this is also changing (indirectly) the effect of ReScanExprContext
so that deletes child contexts too. In the grouping sets work I noticed
I had to explicitly add some MemoryContextDeleteChildren after
ReScanExprContext in order to clean up properly; this obviously makes
that redundant.

(that looks like another data point in favour of this change)

I guess the only question is whether anything currently relies on
ReScanExprContext's current behavior.

-- 
Andrew (irc:RhodiumToad)


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

[HACKERS] Add CINE for ALTER TABLE ... ADD COLUMN

[Fabrízio de Royes Mello]

Hi all,

This simple patch add CINE for ALTER TABLE ... ADD COLUMN.

So now we can:

ALTER TABLE foo
ADD COLUMN IF NOT EXISTS c1 integer;

and/or ...

ALTER TABLE foo
ADD COLUMN IF NOT EXISTS c1 integer,
ADD COLUMN c2 integer;

Regards,

-- 
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index b3a4970..aba7ec0 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -36,7 +36,7 @@ ALTER TABLE ALL IN TABLESPACE name
 
 where action is one of:
 
-ADD [ COLUMN ] column_name data_type [ COLLATE collation ] [ column_constraint [ ... ] ]
+ADD [ COLUMN ] [ IF NOT EXISTS ]column_name data_type [ COLLATE collation ] [ column_constraint [ ... ] ]
 DROP [ COLUMN ] [ IF EXISTS ] column_name [ RESTRICT | CASCADE ]
 ALTER [ COLUMN ] column_name [ SET DATA ] TYPE data_type [ COLLATE collation ] [ USING expression ]
 ALTER [ COLUMN ] column_name SET DEFAULT expression
@@ -96,11 +96,12 @@ ALTER TABLE ALL IN TABLESPACE name
 
   

-ADD COLUMN
+ADD COLUMN [ IF NOT EXISTS ]
 
  
   This form adds a new column to the table, using the same syntax as
-  .
+  . If IF NOT EXISTS
+  is specified and the column already exists, no error is thrown.
  
 

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f5d5b63..5ecb438 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -328,8 +328,8 @@ static void ATPrepAddColumn(List **wqueue, Relation rel, bool recurse, bool recu
 AlterTableCmd *cmd, LOCKMODE lockmode);
 static void ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 ColumnDef *colDef, bool isOid,
-bool recurse, bool recursing, LOCKMODE lockmode);
-static void check_for_column_name_collision(Relation rel, const char *colname);
+bool recurse, bool recursing, bool if_not_exists, LOCKMODE lockmode);
+static bool check_for_column_name_collision(Relation rel, const char *colname, bool if_not_exists);
 static void add_column_datatype_dependency(Oid relid, int32 attnum, Oid typid);
 static void add_column_collation_dependency(Oid relid, int32 attnum, Oid collid);
 static void ATPrepAddOids(List **wqueue, Relation rel, bool recurse,
@@ -2283,7 +2283,7 @@ renameatt_internal(Oid myrelid,
 		oldattname)));
 
 	/* new name should not already exist */
-	check_for_column_name_collision(targetrelation, newattname);
+	check_for_column_name_collision(targetrelation, newattname, false);
 
 	/* apply the update */
 	namestrcpy(&(attform->attname), newattname);
@@ -3399,11 +3399,11 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
 		case AT_AddColumnToView:		/* add column via CREATE OR REPLACE
 		 * VIEW */
 			ATExecAddColumn(wqueue, tab, rel, (ColumnDef *) cmd->def,
-			false, false, false, lockmode);
+			false, false, false, cmd->missing_ok, lockmode);
 			break;
 		case AT_AddColumnRecurse:
 			ATExecAddColumn(wqueue, tab, rel, (ColumnDef *) cmd->def,
-			false, true, false, lockmode);
+			false, true, false, cmd->missing_ok, lockmode);
 			break;
 		case AT_ColumnDefault:	/* ALTER COLUMN DEFAULT */
 			ATExecColumnDefault(rel, cmd->name, cmd->def, lockmode);
@@ -3500,13 +3500,13 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
 			/* Use the ADD COLUMN code, unless prep decided to do nothing */
 			if (cmd->def != NULL)
 ATExecAddColumn(wqueue, tab, rel, (ColumnDef *) cmd->def,
-true, false, false, lockmode);
+true, false, false, cmd->missing_ok, lockmode);
 			break;
 		case AT_AddOidsRecurse:	/* SET WITH OIDS */
 			/* Use the ADD COLUMN code, unless prep decided to do nothing */
 			if (cmd->def != NULL)
 ATExecAddColumn(wqueue, tab, rel, (ColumnDef *) cmd->def,
-true, true, false, lockmode);
+true, true, false, cmd->missing_ok, lockmode);
 			break;
 		case AT_DropOids:		/* SET WITHOUT OIDS */
 
@@ -4593,7 +4593,7 @@ ATPrepAddColumn(List **wqueue, Relation rel, bool recurse, bool recursing,
 static void
 ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 ColumnDef *colDef, bool isOid,
-bool recurse, bool recursing, LOCKMODE lockmode)
+bool recurse, bool recursing, bool if_not_exists, LOCKMODE lockmode)
 {
 	Oid			myrelid = RelationGetRelid(rel);
 	Relation	pgclass,
@@ -4687,7 +4687,13 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	relkind = ((Form_pg_class) GETSTRUCT(reltup))->relkind;
 
 	/* new name should not already exist */
-	check_for_column_name_collision(rel, colDef->colname);
+	if (!check_for_column_name_collision(rel, colDef->colname, if_not_exists))
+	{
+		h

Re: [HACKERS] [REVIEW] Re: Compression of full-page-writes

[Michael Paquier]

On Fri, Feb 27, 2015 at 8:01 AM, Michael Paquier
 wrote:
> On Fri, Feb 27, 2015 at 6:54 AM, Rahila Syed  wrote:
>>>Even this patch doesn't work fine. The standby emit the following
>>>error messages.
>>
>> Yes this bug remains unsolved. I am still working on resolving this.
>>
>> Following chunk IDs have been added in the attached patch as suggested
>> upthread.
>> +#define XLR_CHUNK_BLOCK_REFERENCE  0x10
>> +#define XLR_CHUNK_BLOCK_HAS_IMAGE  0x04
>> +#define XLR_CHUNK_BLOCK_HAS_DATA   0x08
>>
>> XLR_CHUNK_BLOCK_REFERENCE denotes chunk ID of block references.
>> XLR_CHUNK_BLOCK_HAS_IMAGE is a replacement of BKPBLOCK_HAS_IMAGE
>> and XLR_CHUNK_BLOCK_HAS DATA a replacement of BKPBLOCK_HAS_DATA.
>
> Before sending a new version, be sure that this get fixed by for
> example building up a master with a standby replaying WAL, and running
> make installcheck-world or similar. If the standby does not complain
> at all, you have good chances to not have bugs. You could also build
> with WAL_DEBUG to check record consistency.

It would be good to get those problems fixed first. Could you send an
updated patch? I'll look into it in more details. For the time being I
am switching this patch to "Waiting on Author".
-- 
Michael


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

Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables

[Etsuro Fujita]

On 2015/02/26 11:38, Stephen Frost wrote:

I've pushed an update for this to master and 9.4 and improved the
comments and the commit message as discussed.

Would be great if you could test and let me know if you run into any
issues!


OK, thanks!

Best regards,
Etsuro Fujita


--
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] MemoryContext reset/delete callbacks

[Tom Lane]

Andres Freund  writes:
> It's a bit sad to push AllocSetContextData onto four cachelines from the
> current three... That stuff is hot. But I don't really see a way around
> it right now. And it seems like it'd give us more amunition to improve
> things than the small loss of speed it implies.

Meh.  I doubt it would make any difference, especially seeing that the
struct isn't going to be aligned on any special boundary.

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] GSoC idea - Simulated annealing to search for query plans

[Josh Berkus]

On 02/26/2015 05:50 PM, Fabrízio de Royes Mello wrote:
> 
> On Thu, Feb 26, 2015 at 10:27 PM, Andres Freund  > wrote:
>>
>> On 2015-02-26 20:23:33 -0500, Tom Lane wrote:
>> > Josh Berkus mailto:j...@agliodbs.com>> writes:
>> > > On 02/26/2015 01:59 PM, Grzegorz Parka wrote:
>> > >> I'm interested in one of old TODO items related to the optimizer -
>> > >> 'Consider compressed annealing to search for query plans'.
>> >
>> > > You might look at the earlier attempt to make the GEQO replacement
>> > > "pluggable".  That project failed to complete sufficiently to be a
>> > > feature though, but it did enough to show that our current GEQO
>> > > implementation was suboptimal.
>> >
>> > > I'm currently searching for this project ... it was a GSOC
> project, but
>> > > I think before they required posting to Google Code.
>> >
>> > I seem to recall somebody demo'ing a simulated-annealing GEQO
> replacement
>> > at PGCon a couple years back.  It never got to the point of being a
>> > submitted patch though.
>>
>> Yea, it was Jan Urbański (CCed).
>>
> 
> And the project link: https://github.com/wulczer/saio

So what w'ere saying, Grzegorz, is that we would love to see someone
pick this up and get it to the point of making it a feature as a GSOC
project.  I think if you can start from where Jan left off, you could
actually complete it.


-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.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] logical column ordering

[Gavin Flower]

On 27/02/15 14:08, David Steele wrote:
[...]

I agree with Jim's comments.  I've generally followed column ordering
that goes something like:

1) primary key
2) foreign keys
3) flags
4) other programmatic data fields (type, order, etc.)
5) non-programmatic data fields (name, description, etc.)

The immediate practical benefit of this is that users are more likely to
see fields that they need without scrolling right.  Documentation is
also clearer because fields tend to go from most to least important
(left to right, top to bottom).  Also, if you are consistent enough then
users just *know* where to look.

I wrote a function a while back that reorders columns in tables (it not
only deals with reordering, but triggers, constraints, indexes, etc.,
though there are some caveats).  It's painful and not very efficient,
but easy to use.

Most dimension tables that I've worked with are in the millions of rows
so reordering is not problem.  With fact tables, I assess on a
case-by-case basis. It would be nice to not have to do that triage.

The function is attached if anyone is interested.

I've never formally written down the order of how I define 
fields^H^H^H^H^H^H columns in a table, but David's list is the same 
order I use.


The only additional ordering I do: is to put fields that are likely to 
be long text fields, at the end of the record.


So I would certainly appreciate my logical ordering to be the natural 
order they appear.



Cheers,
Gavin


--
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] Partitioning WIP patch

[Amit Langote]

On 27-02-2015 AM 03:01, Jim Nasby wrote:
> On 2/26/15 3:09 AM, Amit Langote wrote:
>> Unless I'm missing something again, isn't allowing partitions to have
>> heterogeneous rowtypes a problem in the long run? I'm afraid I'm
>> confused as to your stand regarding inheritance vs. new partitioning. To
>> be specific, children with heterogeneous schemas sounds much like what
>> inheritance would be good for as you say. But then isn't that why we
>> have to plan children individually which you said new partitioning
>> should get away from?
> 
> Apologies if I haven't been clear enough. What I'd like to see is the
> best of both worlds; fast partitioning when not using inheritance, and
> perhaps somewhat slower when using inheritance, but still with the
> features partitioning gives you.
> 

I get the distinction, thanks.

Actually I wasn't quite thinking of altering the way any part of the
current partitioning based on inheritance works nor am I proposing to
get rid of it. It all stays as is. Not sure how we could say if it will
support features of the new partitioning before those features actually
begin to materialize.

> But my bigger concern from a project standpoint is that we not put
> ourselves in a position of supporting something that we really don't
> want to support (a partitioning system that's got inheritance mixed in).
> As much as I'd personally like to have both features together, I think
> it would be bad for the community to go down that road without careful
> thought.
> 
>> Yes, it does. In fact, I do intend to keep them separate the first
>> attempt of which is to choose to NOT transform a PARTITION OF parent
>> clause into INHERITS parent. Any code that may look like it's trying to
>> do that is because the patch is not fully baked yet.
> 
> Ok, good to know. That's why I was asking about ALTER TABLE ADD COLUMN
> on a partition. If we release something without that being restricted
> it'll probably cause trouble later on.

Yes, I agree. More generally, I think the patch/approach is in need of a
clear separation of internal implementation concerns and user-facing
notions even at this point. This may be one of them. For example, with
the patch, a partition is defined as:

CREATE "TABLE" name PARTITION OF parent ...

Unless that turns into something like:

CREATE PARTITION name OF parent ...

we may not be able to put all the restrictions we'd want to put on a
partition for the sake of what would be partitioning internals.

Thanks,
Amit



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

Re: [HACKERS] GSoC idea - Simulated annealing to search for query plans

[Fabrízio de Royes Mello]

On Thu, Feb 26, 2015 at 10:27 PM, Andres Freund 
wrote:
>
> On 2015-02-26 20:23:33 -0500, Tom Lane wrote:
> > Josh Berkus  writes:
> > > On 02/26/2015 01:59 PM, Grzegorz Parka wrote:
> > >> I'm interested in one of old TODO items related to the optimizer -
> > >> 'Consider compressed annealing to search for query plans'.
> >
> > > You might look at the earlier attempt to make the GEQO replacement
> > > "pluggable".  That project failed to complete sufficiently to be a
> > > feature though, but it did enough to show that our current GEQO
> > > implementation was suboptimal.
> >
> > > I'm currently searching for this project ... it was a GSOC project,
but
> > > I think before they required posting to Google Code.
> >
> > I seem to recall somebody demo'ing a simulated-annealing GEQO
replacement
> > at PGCon a couple years back.  It never got to the point of being a
> > submitted patch though.
>
> Yea, it was Jan Urbański (CCed).
>

And the project link: https://github.com/wulczer/saio

Regards,

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

Re: [HACKERS] contrib/fuzzystrmatch/dmetaphone.c license

[Michael Paquier]

On Thu, Feb 26, 2015 at 8:56 AM, Stephen Frost  wrote:
> * Jim Nasby (jim.na...@bluetreble.com) wrote:
>> On 2/25/15 4:10 PM, Andrew Dunstan wrote:
>> >
>> >On 02/25/2015 11:59 AM, Joe Conway wrote:
>> >>>
>> >>>It's largely because of such uncertainties that I have been advised
>> >>>in the past (by those with appropriate letters after their names)
>> >>>to stop using the Artistic licence. This is why I spent nearly a
>> >>>year working on changing pgAdmin to the PostgreSQL licence.
>> >>I committed this (1 July 2004), but cannot remember any details about
>> >>a license discussion. And I searched the list archives and curiously
>> >>cannot find any email at all about it either. Maybe Andrew remembers
>> >>something.
>> >>
>> >>I doubt we want to rip it out without some suitable replacement -- do we?
>> >>
>> >>
>> >
>> >That's more than 10 years ago. I remember creating this for my then work
>> >at the North Carolina State Highway Patrol and sending it to Joe, but
>> >that's about the extent of my recollection.
>> >
>> >If the Artistic License isn't acceptable. I guess we'd have to try to
>> >get the code relicensed, or reimplement the function ourselves. There
>> >are numerous implementations out there we could copy from or use as a
>> >basis for reimplementation, including several licensed under the Apache
>> >2.0 license - is that compatible with ours?
>>
>> Perhaps a company large enough to have in-house counsel
>> (EnterpriseDB?) could get a quick legal opinion on the license
>> before we start pursuing other things? Perhaps this is just a
>> non-issue...
>
> For my 2c (IANAL), I'm not convinced that it's an issue either..  I've
> certainly not heard of anyone complaining about it either, so..
>
> That said, we could also through SPI which might get us a bit of
> pro-bono work, if we really wanted to pursue this.  Just a hunch, but as
> they tend to be conservative (lawyers in general), I expect the answer
> we'd get is "yes, it might conflict and if you want to avoid any issues
> you wouldn't include it."

Exactly :), and I just had a discussion with some legal folks about
that because it has been an issue raised internally. So the boring
stuff being... The Perl License has two meanings: GPL v2.0 or Artistic
License 1.0, and there can be problems if fuzzystrmatch.so links with
something that has portions licensed as Apache v2 because Apache v2
and GPL v2.0 are incompatible, or anything who-know-what incompatible
with Apache v2. By choosing the Artistic license there are no problems
visibly, at least for the case I have been pinged about.

> To that end, I'd suggest -core simply formally ask the authors about it.
> Once we have that answer, we can figure out what to do.  In my
> experience, at least, it's a lot better to go that route and figure out
> what the original authors really *intended* than to go get a lawyer to
> weigh in on it.  One of those approaches is both free and gives a clear
> answer, while the other is expensive and doesn't provide any real
> certainty. :)

I would go this way as well, aka ask the authors and see if it is
possible to remove the license notice and keep everything licensed
under PostgreSQL license to avoid any future problems...
-- 
Michael


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

Re: [HACKERS] GSoC idea - Simulated annealing to search for query plans

[Andres Freund]

On 2015-02-26 20:23:33 -0500, Tom Lane wrote:
> Josh Berkus  writes:
> > On 02/26/2015 01:59 PM, Grzegorz Parka wrote:
> >> I'm interested in one of old TODO items related to the optimizer -
> >> 'Consider compressed annealing to search for query plans'.
> 
> > You might look at the earlier attempt to make the GEQO replacement
> > "pluggable".  That project failed to complete sufficiently to be a
> > feature though, but it did enough to show that our current GEQO
> > implementation was suboptimal.
> 
> > I'm currently searching for this project ... it was a GSOC project, but
> > I think before they required posting to Google Code.
> 
> I seem to recall somebody demo'ing a simulated-annealing GEQO replacement
> at PGCon a couple years back.  It never got to the point of being a
> submitted patch though.

Yea, it was Jan Urbański (CCed).

Greetings,

Andres Freund

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


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

Re: [HACKERS] GSoC idea - Simulated annealing to search for query plans

[Tom Lane]

Josh Berkus  writes:
> On 02/26/2015 01:59 PM, Grzegorz Parka wrote:
>> I'm interested in one of old TODO items related to the optimizer -
>> 'Consider compressed annealing to search for query plans'.

> You might look at the earlier attempt to make the GEQO replacement
> "pluggable".  That project failed to complete sufficiently to be a
> feature though, but it did enough to show that our current GEQO
> implementation was suboptimal.

> I'm currently searching for this project ... it was a GSOC project, but
> I think before they required posting to Google Code.

I seem to recall somebody demo'ing a simulated-annealing GEQO replacement
at PGCon a couple years back.  It never got to the point of being a
submitted patch though.

regards, tom lane


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

Re: [HACKERS] GSoC idea - Simulated annealing to search for query plans

[Josh Berkus]

On 02/26/2015 01:59 PM, Grzegorz Parka wrote:
> Dear Hackers,
> 
> I'm Grzegorz Parka, BSc Engineer of Technical Physics and student of
> Computer Science at WUT, Poland. Last year I've been a bit into
> evolutionary algorithms and during my research I found out about GEQO in
> Postgres. I also found out that there are plans to try a different
> attempt to find optimal query plans and thought it could be a good thing
> to use it as a project for GSoC.
> 
> I'm interested in one of old TODO items related to the optimizer -
> 'Consider compressed annealing to search for query plans'. I believe
> this would be potentially beneficial to Postgres to check if such a
> heuristic could really choose better query plans than GEQO does. Judging
> by the results that simulated annealing gives on the travelling salesman
> problem, it looks like a simpler and potentially more effective way of
> combinatorial optimization.
> 
> As deliverables of such a project I would provide a simple
> implementation of basic simulated annealing optimizer and some form of
> quantitative comparison with GEQO.

You might look at the earlier attempt to make the GEQO replacement
"pluggable".  That project failed to complete sufficiently to be a
feature though, but it did enough to show that our current GEQO
implementation was suboptimal.

I'm currently searching for this project ... it was a GSOC project, but
I think before they required posting to Google Code.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.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] logical column ordering

[David Steele]

On 2/26/15 1:49 PM, Jim Nasby wrote:
> On 2/23/15 5:09 PM, Tomas Vondra wrote:
>> Over the time I've heard various use cases for this patch, but in most
>> cases it was quite speculative. If you have an idea where this might be
>> useful, can you explain it here, or maybe point me to a place where it's
>> described?
> 
> For better or worse, table structure is a form of documentation for a
> system. As such, it's very valuable to group related fields in a table
> together. When creating a table, that's easy, but as soon as you need to
> alter your careful ordering can easily end up out the window.
> 
> Perhaps to some that just sounds like pointless window dressing, but my
> experience is that on a complex system the less organized things are the
> more bugs you get due to overlooking something.

I agree with Jim's comments.  I've generally followed column ordering
that goes something like:

1) primary key
2) foreign keys
3) flags
4) other programmatic data fields (type, order, etc.)
5) non-programmatic data fields (name, description, etc.)

The immediate practical benefit of this is that users are more likely to
see fields that they need without scrolling right.  Documentation is
also clearer because fields tend to go from most to least important
(left to right, top to bottom).  Also, if you are consistent enough then
users just *know* where to look.

I wrote a function a while back that reorders columns in tables (it not
only deals with reordering, but triggers, constraints, indexes, etc.,
though there are some caveats).  It's painful and not very efficient,
but easy to use.

Most dimension tables that I've worked with are in the millions of rows
so reordering is not problem.  With fact tables, I assess on a
case-by-case basis. It would be nice to not have to do that triage.

The function is attached if anyone is interested.

-- 
- David Steele
da...@pgmasters.net
/
 CATALOG_TABLE_COLUMN_MOVE Function
create
 or replace function _utility.catalog_table_column_move
(
strSchemaName text, 
strTableName text, 
strColumnName text, 
strColumnNameBefore text
)
returns void as $$
declare
rIndex record;
rConstraint record;
rColumn record;
strSchemaTable text = strSchemaName || '.' || strTableName;
strDdl text;
strClusterIndex text;
begin
-- Raise notice that a reorder is in progress
raise notice 'Reorder columns in table %.% (% before %)', strSchemaName, 
strTableName, strColumnName, strColumnNameBefore;

-- Get the cluster index
select pg_index.relname
  into strClusterIndex
  from pg_namespace
   inner join pg_class
on pg_class.relnamespace = pg_namespace.oid
   and pg_class.relname = strTableName
   inner join pg_index pg_index_map
on pg_index_map.indrelid = pg_class.oid
   and pg_index_map.indisclustered = true
   inner join pg_class pg_index
on pg_index.oid = pg_index_map.indexrelid
 where pg_namespace.nspname = strSchemaName;

if strClusterIndex is null then
raise exception 'Table %.% must have a cluster index before 
reordering', strSchemaName, strTableName;
end if;

-- Disable all user triggers
strDdl = 'alter table ' || strSchemaTable || ' disable trigger user';
raise notice 'Disable triggers [%]', strDdl;
execute strDdl;

-- Create temp table to hold ddl
create temp table temp_catalogtablecolumnreorder
(
type text not null,
name text not null,
ddl text not null
);

-- Save index ddl in a temp table
raise notice 'Save indexes';

for rIndex in
with index as
(
select _utility.catalog_index_list_get(strSchemaName, strTableName) 
as name
),
index_ddl as
(
select index.name,
   
_utility.catalog_index_create_get(_utility.catalog_index_get(strSchemaName, 
index.name)) as ddl
  from index
)
select index.name,
   index_ddl.ddl
  from index
   left outer join index_ddl
on index_ddl.name = index.name
   and index_ddl.ddl not like '%[function]%'
loop
raise notice 'Save %', rIndex.name;
insert into temp_catalogtablecolumnreorder values ('index', 
rIndex.name, rIndex.ddl);
end loop;

-- Save constraint ddl in a temp table
raise notice 'Save constraints';

for rConstraint in
with constraint_list as
(
select _utility.catalog_constraint_list_get(strSchemaName, 
strTableName, '{p,u,f,c}') as name
),
constraint_ddl as
(
select constraint_list.name,
   
_utility.catalog_constraint_create_get(_utility.catalog_c

Re: [HACKERS] MemoryContext reset/delete callbacks

[Andres Freund]

On 2015-02-27 01:54:27 +0100, Andres Freund wrote:
> On 2015-02-26 19:28:50 -0500, Tom Lane wrote:
> >   /*
> > *** typedef struct MemoryContextData
> > *** 59,72 
> > MemoryContext firstchild;   /* head of linked list of children */
> > MemoryContext nextchild;/* next child of same parent */
> > char   *name;   /* context name (just for 
> > debugging) */
> > boolisReset;/* T = no space alloced since 
> > last reset */
> >   #ifdef USE_ASSERT_CHECKING
> > !   boolallowInCritSection; /* allow palloc in critical 
> > section */
> >   #endif
> >   } MemoryContextData;
> 
> It's a bit sad to push AllocSetContextData onto four cachelines from the
> current three... That stuff is hot. But I don't really see a way around
> it right now. And it seems like it'd give us more amunition to improve
> things than the small loss of speed it implies.

Actually:
struct MemoryContextData {
NodeTagtype; /* 0 4 */

/* XXX 4 bytes hole, try to pack */

MemoryContextMethods * methods;  /* 8 8 */
MemoryContext  parent;   /*16 8 */
MemoryContext  firstchild;   /*24 8 */
MemoryContext  nextchild;/*32 8 */
char * name; /*40 8 */
bool   isReset;  /*48 1 */
bool   allowInCritSection;   /*49 1 */

/* size: 56, cachelines: 1, members: 8 */
/* sum members: 46, holes: 1, sum holes: 4 */
/* padding: 6 */
/* last cacheline: 56 bytes */
};

If we move isReset and allowInCritSection after type, we'd stay at the
same size...

Andres Freund

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


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

Re: [HACKERS] Partitioning WIP patch

[Michael Paquier]

On Fri, Feb 27, 2015 at 3:24 AM, Andres Freund  wrote:
> On 2015-02-26 12:15:17 +0900, Amit Langote wrote:
>> On 26-02-2015 AM 05:15, Josh Berkus wrote:
>> > On 02/24/2015 12:13 AM, Amit Langote wrote:
>> >> Here is an experimental patch that attempts to implement this.
>
>> > I would love to have it for 9.5, but I guess the
>> > patch isn't nearly baked enough for that?
>
>> I'm not quite sure what would qualify as baked enough for 9.5 though we
>> can surely try to reach some consensus on various implementation aspects
>> and perhaps even get it ready in time for 9.5.
>
> I think it's absolutely unrealistic to get this into 9.5. There's barely
> been any progress on the current (last!) commitfest - where on earth
> should the energy come to make this patch ready? And why would that be
> fair against all the others that have submitted in time?

+1. There are many other patches pending the in CF app waiting for
feedback, while this one showed up after the last CF deadline for 9.5
and needs design and spec decisions that should not be taken lightly
at the end of a major release development cycle. Please let's not rush
into something we may regret.
-- 
Michael


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

Re: [HACKERS] MemoryContext reset/delete callbacks

[Andres Freund]

Hi,

On 2015-02-26 19:28:50 -0500, Tom Lane wrote:
> We discussed this idea a couple weeks ago.

Hm, didn't follow that discussion.

> The core of it is that when a memory context is being deleted, you
> might want something extra to happen beyond just pfree'ing everything
> in the context.

I've certainly wished for this a couple times...

> Attached is a draft patch for this.  I've not really tested it more than
> seeing that it compiles, but it's so simple that there are unlikely to be
> bugs as such in it.  There are some design decisions that could be
> questioned though:
> 
> 1. I used ilists for the linked list of callback requests.  This creates a
> dependency from memory contexts to lib/ilist.  That's all right at the
> moment since lib/ilist does no memory allocation, but it might be
> logically problematic.  We could just use explicit "struct foo *" links
> with little if any notational penalty, so I wonder if that would be
> better.

Maybe I'm partial here, but I don't see a problem. Actually the reason I
started the ilist stuff was that I wrote a different memory context
implementation ;). Wish I'd time/energy to go back to that...

> 2. I specified that callers have to allocate the storage for the callback
> structs.  This saves a palloc cycle in just about every use-case I've
> thought of, since callers generally will need to palloc some larger struct
> of their own and they can just embed the MemoryContextCallback struct in
> that.  It does seem like this offers a larger surface for screwups, in
> particular putting the callback struct in the wrong memory context --- but
> there's plenty of surface for that type of mistake anyway, if you put
> whatever the "void *arg" is pointing at in the wrong context.
> So I'm OK with this but could also accept a design in which
> MemoryContextRegisterResetCallback takes just a function pointer and a
> "void *arg" and palloc's the callback struct for itself in the target
> context.  I'm unsure whether that adds enough safety to justify a separate
> palloc.

I'm unworried about this. Yes, one might be confused for a short while,
but compared to the complexity of using any such facility sanely it
seems barely relevant.

> 3. Is the "void *arg" API for the callback func sufficient?  I considered
> also passing it the MemoryContext, but couldn't really find a use-case
> to justify that.

Hm, seems sufficient to me.

> 4. I intentionally didn't provide a MemoryContextDeregisterResetCallback
> API.  Since it's just a singly-linked list, that could be an expensive
> operation and so I'd rather discourage it.  In the use cases I've thought
> of, you'd want the callback to remain active for the life of the context
> anyway, so there would be no need.  (And, of course, there is slist_delete
> for the truly determined ...)

Yea, that seems fine. If you don't want the callback to do anything
anymore, it's easy enough to just set a flag somewhere.

>   /*
> *** typedef struct MemoryContextData
> *** 59,72 
>   MemoryContext firstchild;   /* head of linked list of children */
>   MemoryContext nextchild;/* next child of same parent */
>   char   *name;   /* context name (just for 
> debugging) */
>   boolisReset;/* T = no space alloced since 
> last reset */
>   #ifdef USE_ASSERT_CHECKING
> ! boolallowInCritSection; /* allow palloc in critical 
> section */
>   #endif
>   } MemoryContextData;

It's a bit sad to push AllocSetContextData onto four cachelines from the
current three... That stuff is hot. But I don't really see a way around
it right now. And it seems like it'd give us more amunition to improve
things than the small loss of speed it implies.


I guess it might needa a warning about being careful what you directly
free if you use this. Might also better fit in a layer above this...

Greetings,

Andres Freund

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


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

Re: [HACKERS] Partitioning WIP patch

[Amit Langote]

On 27-02-2015 AM 03:18, Josh Berkus wrote:
> On 02/25/2015 07:15 PM, Amit Langote wrote:
>> I'm not quite sure what would qualify as baked enough for 9.5 though we
>> can surely try to reach some consensus on various implementation aspects
>> and perhaps even get it ready in time for 9.5.
> 
> Well, we don't have long at all to do that.  I guess I'm asking what
> kind of completeness of code we have; is this basically done pending API
> changes and bugs, or are there major bits (like, say, pg_dump  and
> EXPLAIN support) which are completely unimplemented?
> 

I would say I am not entirely sure/satisfied about some decisions I have
made (or not) when writing even the basic patch. Yes,
pg_dump/EXPLAIN/psql, etc. are not touched. So, it seems it might not be
fair to claim it's actually something for 9.5. Let me just call it WIP
for a while while keep I working on it and receive feedback.

>> Only one concern I can remember someone had raised is that having to
>> evaluate an expression for every row during bulk-inserts may end up
>> being pretty expensive. Though, we might have to live with that.
> 
> Well, it's not more expensive than having to materialize the value from
> a trigger and store it on disk.  The leading one here would be functions
> over timestamp; for example, the data has a timestamptz, but you want to
> partition by week.
> 
>>
>> I think one idea is to learn from ability to use expressions in indexes.
> 
> Sure.  So a feature to implement for the 2nd release.

Actually, I'm trying to add that and see how it works. I will post an
updated patch soon if it looks good enough.

Thanks,
Amit




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

[HACKERS] MemoryContext reset/delete callbacks

[Tom Lane]

We discussed this idea a couple weeks ago.  The core of it is that when a
memory context is being deleted, you might want something extra to happen
beyond just pfree'ing everything in the context.  I'm thinking in
particular that this might provide a nice solution to the problem we
discussed earlier today of managing cached information about a domain's
CHECK constraints: a function could make a reference-counted link to some
data in its FmgrInfo cache, and use a memory context callback on the
context containing the FmgrInfo to ensure that the reference count gets
decremented when needed and not before.

Attached is a draft patch for this.  I've not really tested it more than
seeing that it compiles, but it's so simple that there are unlikely to be
bugs as such in it.  There are some design decisions that could be
questioned though:

1. I used ilists for the linked list of callback requests.  This creates a
dependency from memory contexts to lib/ilist.  That's all right at the
moment since lib/ilist does no memory allocation, but it might be
logically problematic.  We could just use explicit "struct foo *" links
with little if any notational penalty, so I wonder if that would be
better.

2. I specified that callers have to allocate the storage for the callback
structs.  This saves a palloc cycle in just about every use-case I've
thought of, since callers generally will need to palloc some larger struct
of their own and they can just embed the MemoryContextCallback struct in
that.  It does seem like this offers a larger surface for screwups, in
particular putting the callback struct in the wrong memory context --- but
there's plenty of surface for that type of mistake anyway, if you put
whatever the "void *arg" is pointing at in the wrong context.
So I'm OK with this but could also accept a design in which
MemoryContextRegisterResetCallback takes just a function pointer and a
"void *arg" and palloc's the callback struct for itself in the target
context.  I'm unsure whether that adds enough safety to justify a separate
palloc.

3. Is the "void *arg" API for the callback func sufficient?  I considered
also passing it the MemoryContext, but couldn't really find a use-case
to justify that.

4. I intentionally didn't provide a MemoryContextDeregisterResetCallback
API.  Since it's just a singly-linked list, that could be an expensive
operation and so I'd rather discourage it.  In the use cases I've thought
of, you'd want the callback to remain active for the life of the context
anyway, so there would be no need.  (And, of course, there is slist_delete
for the truly determined ...)

Comments?

regards, tom lane

diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 202bc78..624d08a 100644
*** a/src/backend/utils/mmgr/mcxt.c
--- b/src/backend/utils/mmgr/mcxt.c
*** MemoryContext CurTransactionContext = NU
*** 54,59 
--- 54,60 
  /* This is a transient link to the active portal's memory context: */
  MemoryContext PortalContext = NULL;
  
+ static void MemoryContextCallResetCallbacks(MemoryContext context);
  static void MemoryContextStatsInternal(MemoryContext context, int level);
  
  /*
*** MemoryContextReset(MemoryContext context
*** 150,155 
--- 151,157 
  	/* Nothing to do if no pallocs since startup or last reset */
  	if (!context->isReset)
  	{
+ 		MemoryContextCallResetCallbacks(context);
  		(*context->methods->reset) (context);
  		context->isReset = true;
  		VALGRIND_DESTROY_MEMPOOL(context);
*** MemoryContextDelete(MemoryContext contex
*** 196,201 
--- 198,211 
  	MemoryContextDeleteChildren(context);
  
  	/*
+ 	 * It's not entirely clear whether 'tis better to do this before or after
+ 	 * delinking the context; but an error in a callback will likely result in
+ 	 * leaking the whole context (if it's not a root context) if we do it
+ 	 * after, so let's do it before.
+ 	 */
+ 	MemoryContextCallResetCallbacks(context);
+ 
+ 	/*
  	 * We delink the context from its parent before deleting it, so that if
  	 * there's an error we won't have deleted/busted contexts still attached
  	 * to the context tree.  Better a leak than a crash.
*** MemoryContextResetAndDeleteChildren(Memo
*** 243,248 
--- 253,308 
  }
  
  /*
+  * MemoryContextRegisterResetCallback
+  *		Register a function to be called before next context reset/delete.
+  *		Such callbacks will be called in reverse order of registration.
+  *
+  * The caller is responsible for allocating a MemoryContextCallback struct
+  * to hold the info about this callback request, and for filling in the
+  * "func" and "arg" fields in the struct to show what function to call with
+  * what argument.  Typically the callback struct should be allocated within
+  * the specified context, since that means it will automatically be freed
+  * when no longer needed.
+  *
+  * There is no API for deregistering a callback once registered.  If

Re: [HACKERS] Partitioning WIP patch

[Amit Langote]

On 27-02-2015 AM 03:24, Andres Freund wrote:
> On 2015-02-26 12:15:17 +0900, Amit Langote wrote:
>> On 26-02-2015 AM 05:15, Josh Berkus wrote:
>>> I would love to have it for 9.5, but I guess the
>>> patch isn't nearly baked enough for that?
> 
>> I'm not quite sure what would qualify as baked enough for 9.5 though we
>> can surely try to reach some consensus on various implementation aspects
>> and perhaps even get it ready in time for 9.5.
> 
> I think it's absolutely unrealistic to get this into 9.5. There's barely
> been any progress on the current (last!) commitfest - where on earth
> should the energy come to make this patch ready? And why would that be
> fair against all the others that have submitted in time?
> 

I realize and I apologize that it was irresponsible of me to have said
that; maybe got a bit too excited. I do not want to unduly draw people's
time on something that's not quite ready while there are other things
people have worked hard on to get in time. In all earnestness, I say we
spend time perfecting those things.

I'll add this into CF-JUN'15. I will keep posting updates meanwhile so
that when that commitfest finally starts, we will have something worth
considering.

Thanks,
Amit



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

Re: [HACKERS] Precedence of standard comparison operators

[Andres Freund]

On 2015-02-26 20:13:34 +, Simon Riggs wrote:
> On 26 February 2015 at 15:56, Tom Lane  wrote:
> 
> >> I think the way to do this is to have a pluggable parser, so users can
> >> choose 1) old parser, 2) new, better parser, 3) any other parser they
> >> fancy that they maintain to ensure application compatibility. We've
> >> got a pluggable executor and optimizer, so why not a parser too?
> >> Implementing that in the same way we have done for executor and
> >> optimizer is a 1 day patch, so easily achievable for 9.5.
> >
> > I don't find that particularly attractive either.  It seems quite unlikely
> > to me that anyone would actually use such a hook; replacing the whole
> > parser would be essentially unmaintainable.  Nobody uses the hooks you
> > mention to replace the whole planner or whole executor --- there are
> > wrappers out there, which is a different thing altogether.  But you could
> > not undo the issue at hand here without at the very least a whole new
> > copy of gram.y, which would need to be updated constantly if you wanted
> > it to keep working across more than one release.

I can see a point in having the ability to plug in a parser - I just
don't think it has much to do with this topic. It'd be a nightmare to
maintain two versions of our parser; I don't think anybody wants to
patch more than one.

> Whole planner replacement is used for extensions by CitusData and NTT,
> and will definitely be used in the future for other apps.

s/planner/parser/? Because replacing the planner can already be done as
a whole without much problems.

> Whole executor replacement is also used by one extension to produce
> DDL triggers.

Hm?

> In any case, whole planner replacement would be very desirable for
> people trying to minimize code churn in their applications. As soon as
> it exists, I will use to make some MySQL-only apps work seamlessly
> against PostgreSQL, such as WordPress. It doesn't need to be 100%
> perfect MySQL, it just needs to be enough to make the app work.
> Maintaining a code base that can work against multiple databases is
> hard. Writing it against one main database and maintaining a parser
> layer would be much easier.

Assuming you meant parser: Maybe. I have my doubt somebody will actually
invest the significant amount of time to develop something like
that. But I don't have a problem providing the capability; there seems
little point in allowing to replace the optimizer but not the planner. I
just don't see that it has much to do with this discussion.

Greetings,

Andres Freund

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


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

Re: [HACKERS] logical column ordering

[Jim Nasby]

On 2/26/15 4:34 PM, Andres Freund wrote:

On 2015-02-26 16:16:54 -0600, Jim Nasby wrote:

On 2/26/15 4:01 PM, Alvaro Herrera wrote:

The reason for doing it this way is that changing the underlying
architecture is really hard, without having to bear an endless hackers
bike shed discussion about the best userland syntax to use.  It seems a
much better approach to do the actually difficult part first, then let
the rest to be argued to death by others and let those others do the
easy part (and take all the credit along with that); that way, that
discussion does not kill other possible uses that the new architecture
allows.


I agree that it's a sane order of developing things. But: I don't think
it makes sense to commit it without the capability to change the
order. Not so much because it'll not serve a purpose at that point, but
rather because it'll essentially untestable. And this is a patch that
needs a fair amount of changes, both automated, and manual.


It's targeted for 9.6 anyway, so we have a while to decide on what's 
committed when. It's possible that there's no huge debate on the syntax.



At least during development I'd even add a function that randomizes the
physical order of columns, and keeps the logical one. Running the
regression tests that way seems likely to catch a fair number of bugs.


Yeah, I've thought that would be a necessary part of testing. Not really 
sure how we'd go about it with existing framework though...



+1. This patch is already several years old; lets not delay it further.

Plus, I suspect that you could hack the catalog directly if you really
wanted to change LCO. Worst case you could create a C function to do it.


I don't think that's sufficient for testing purposes. Not only for the
patch itself, but also for getting it right in new code.


We'll want to do testing that it doesn't make sense for the API to 
support. For example, randomizing the storage ordering; that's not a 
sane use case. Ideally we wouldn't even expose physical ordering because 
the code would be smart enough to figure it out.

--
Jim Nasby, Data Architect, Blue Treble Consulting
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] Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset

[Andres Freund]

On 2015-02-26 18:05:46 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > On 2015-02-26 17:45:26 -0500, Tom Lane wrote:
> > If the changes breaks some code it's likely actually a good thing:
> > Because, as you say, using MemoryContextReset() will likely be the wrong
> > thing, and they'll want to fix it for the existing branches as well.
> 
> Is that likely to happen?  I doubt it.  They'll just mutter under their
> breath about useless API breakage and move on.

Maybe.

> Basically, this is a judgment call, and I disagree with your judgment.

Right. That's ok, happens all the time.

> And I've got to say that I'm losing patience with
> backwards-compatibility arguments taken to this degree.  We might as
> well stop development entirely.

Meh, normally you're on the side I'm on right now...

Greetings,

Andres Freund

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


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

Re: [HACKERS] Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset

[Tom Lane]

Andres Freund  writes:
> On 2015-02-26 17:45:26 -0500, Tom Lane wrote:
>> With all due respect, that's utterly wrong.  I have looked at every single
>> MemoryContextReset call in the codebase, and as far as I can see the
>> *only* one that is in an error path is elog.c:336, which I'm quite certain
>> is wrong anyway (the other reset of ErrorContext in that file uses
>> MemoryContextResetAndDeleteChildren, this one should too).

> Sure, in the pg codebase. But there definitely are extensions using
> memory contexts. And there's code that has to work across several
> branches.  Backpatching alone imo presents a risk; there's nothing to
> warn you three years down the road that the MemoryContextReset() you
> just backpatched doesn't work the same in the backbranches.

> If the changes breaks some code it's likely actually a good thing:
> Because, as you say, using MemoryContextReset() will likely be the wrong
> thing, and they'll want to fix it for the existing branches as well.

Is that likely to happen?  I doubt it.  They'll just mutter under their
breath about useless API breakage and move on.

Basically, this is a judgment call, and I disagree with your judgment.
I think changing the behavior of MemoryContextReset is exactly what we
want to have happen.  (And I've got to say that I'm losing patience with
backwards-compatibility arguments taken to this degree.  We might as
well stop development entirely.)

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] [REVIEW] Re: Compression of full-page-writes

[Michael Paquier]

On Fri, Feb 27, 2015 at 6:54 AM, Rahila Syed  wrote:
> Hello,
>
>>Even this patch doesn't work fine. The standby emit the following
>>error messages.
>
> Yes this bug remains unsolved. I am still working on resolving this.
>
> Following chunk IDs have been added in the attached patch as suggested
> upthread.
> +#define XLR_CHUNK_BLOCK_REFERENCE  0x10
> +#define XLR_CHUNK_BLOCK_HAS_IMAGE  0x04
> +#define XLR_CHUNK_BLOCK_HAS_DATA   0x08
>
> XLR_CHUNK_BLOCK_REFERENCE denotes chunk ID of block references.
> XLR_CHUNK_BLOCK_HAS_IMAGE is a replacement of BKPBLOCK_HAS_IMAGE
> and XLR_CHUNK_BLOCK_HAS DATA a replacement of BKPBLOCK_HAS_DATA.

Before sending a new version, be sure that this get fixed by for
example building up a master with a standby replaying WAL, and running
make installcheck-world or similar. If the standby does not complain
at all, you have good chances to not have bugs. You could also build
with WAL_DEBUG to check record consistency.
-- 
Michael


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

Re: [HACKERS] Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset

[Tom Lane]

Andres Freund  writes:
> I'd really not even be surprised if a committer backpatches a
> MemoryContextReset() addition, not realizing it behaves differently in
> the back branches.

As far as that goes, the only consequence would be a possible memory leak
in the back branches; it would not be a really fatal problem.  I'd rather
live with that risk than with the sort of intentional API break (and
ensuing back-patch pain) that you're proposing.

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] Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset

[Tom Lane]

Andres Freund  writes:
> ... Without a compiler erroring out people won't
> notice that suddenly MemoryContextReset deletes much more; leading to
> possibly hard to find errors.

BTW, so far as *data* is concerned, the existing call deletes all data in
the child contexts already.  The only not-already-buggy operation you could
perform before that would no longer work is to allocate fresh data in one
of those child contexts, assuming you still had a pointer to such a
context.  I've not seen any coding pattern in which that's likely.  The
problem is exactly that whoever's resetting the parent context isn't aware
of child contexts having been attached to it.

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] Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset

[Andres Freund]

On 2015-02-26 17:45:26 -0500, Tom Lane wrote:
> Andres Freund  writes:
> Or are you arguing for an alternative proposal in which we remove
> MemoryContextReset (or at least rename it to something new) and thereby
> intentionally break all code that uses MemoryContextReset?

Yes, that I am.

After a bit of thought I just sent the suggestion to add a parameter to
MemoryContextReset(). That way the compiler will warn.

> > ... Without a compiler erroring out people won't
> > notice that suddenly MemoryContextReset deletes much more; leading to
> > possibly hard to find errors. Context resets frequently are in error
> > paths, and those won't necessarily be hit when running with assertions
> > enabled.
> 
> With all due respect, that's utterly wrong.  I have looked at every single
> MemoryContextReset call in the codebase, and as far as I can see the
> *only* one that is in an error path is elog.c:336, which I'm quite certain
> is wrong anyway (the other reset of ErrorContext in that file uses
> MemoryContextResetAndDeleteChildren, this one should too).

Sure, in the pg codebase. But there definitely are extensions using
memory contexts. And there's code that has to work across several
branches.  Backpatching alone imo presents a risk; there's nothing to
warn you three years down the road that the MemoryContextReset() you
just backpatched doesn't work the same in the backbranches.

If the changes breaks some code it's likely actually a good thing:
Because, as you say, using MemoryContextReset() will likely be the wrong
thing, and they'll want to fix it for the existing branches as well.

Greetings,

Andres Freund

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


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

Re: [HACKERS] Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset

[Tom Lane]

Andres Freund  writes:
> On 2015-02-26 17:01:53 -0500, Tom Lane wrote:
>> We've discussed doing $SUBJECT off and on for nearly ten years,
>> the oldest thread I could find about it being here:
>> http://www.postgresql.org/message-id/flat/1186435268.16321.37.ca...@dell.linuxdev.us.dell.com
>> It's come up again every time we found another leak of dead child
>> contexts, which happened twice last year for example.  And that patch
>> I'm pushing for "expanded" out-of-line objects really needs this to
>> become the default behavior anywhere that we can detoast objects.

> I don't object to the behavioural change per se, rather the contrary,
> but I find it a pretty bad idea to change the meaning of an existing
> functioname this way.

That's pretty much the whole point I think.

Or are you arguing for an alternative proposal in which we remove
MemoryContextReset (or at least rename it to something new) and thereby
intentionally break all code that uses MemoryContextReset?  I can't say
that I find that an improvement.

> ... Without a compiler erroring out people won't
> notice that suddenly MemoryContextReset deletes much more; leading to
> possibly hard to find errors. Context resets frequently are in error
> paths, and those won't necessarily be hit when running with assertions
> enabled.

With all due respect, that's utterly wrong.  I have looked at every single
MemoryContextReset call in the codebase, and as far as I can see the
*only* one that is in an error path is elog.c:336, which I'm quite certain
is wrong anyway (the other reset of ErrorContext in that file uses
MemoryContextResetAndDeleteChildren, this one should too).

I see no reason whatever to believe that this change is likely to do
anything except fix heretofore-unnoticed memory leaks.

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] Precedence of standard comparison operators

[Jim Nasby]

On 2/26/15 4:09 PM, Tom Lane wrote:

Andres Freund  writes:

On February 26, 2015 10:29:18 PM CET, Peter Eisentraut  wrote:

My suggestion was to treat this like the standard_conforming_string
change.  That is, warn for many years before changing.



I don't think scs is a good example to follow.


Yeah.  For one thing, there wouldn't be any way to suppress the warning
other than to parenthesize your code, which I would find problematic
because it would penalize standard-conforming queries.  I'd prefer an
arrangement whereby once you fix your code to be standard-conforming,
you're done.

A possible point of compromise would be to leave the warning turned on
by default, at least until we get a better sense of how this would
play out in the real world.  I continue to suspect that we're making
a mountain out of, if not a molehill, at least a hillock.  I think most
sane people would have parenthesized their queries to start with rather
than go look up whether IS DISTINCT FROM binds tighter than <= ...


Question of sanity aside, I can tell you that many business queries are 
written with little more sophistication than monkeys with typewriters, 
where you keep the monkeys fed with bananas and paper until your query 
results (not the SQL) looks like what someone expects it to look like. 
Then the output of that version is held as sacrosanct, and any future 
SQL changes are wrong unless they produce the expected result changes. 
In my experience this happens because some poor business person just 
needs to get their job done and either isn't allowed to bother the data 
people or the data people are just too busy, so they're stuck doing it 
themselves. From what I've seen, SQL written by developers is often a 
bit better than this... but not a lot. And part of that salvation is 
because application queries tend to be a lot less complex than 
reporting/BI ones.


I don't have a great feel for how much of an impact this specific change 
would have on that... the examples so far have all been pretty esoteric. 
I suspect most people writing such "wonderful" SQL don't know about IS 
DISTINCT FROM nor would they try doing things like bool_a > bool_b >= 
bool_c. So there may actually be very little code to fix, but I think we 
at least need a way for users to verify that.

--
Jim Nasby, Data Architect, Blue Treble Consulting
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] logical column ordering

[Kevin Grittner]

Tomas Vondra  wrote:

> Over the time I've heard various use cases for this patch, but in most
> cases it was quite speculative. If you have an idea where this might be
> useful, can you explain it here, or maybe point me to a place where it's
> described?


One use case is to be able to suppress default display of columns that are
used for internal purposes.  For example, incremental maintenance of
materialized views will require storing a "count(t)" column, and sometimes
state information for aggregate columns, in addition to what the users
explicitly request.  At the developers' meeting there was discussion of
whether and how to avoid displaying these by default, and it was felt that
when we have this logical column ordering it would be good to have a way to
suppress default display.  Perhaps this could be as simple as a special
value for logical position.
-- 

Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset

[Andres Freund]

On 2015-02-26 23:31:16 +0100, Andres Freund wrote:
> Without a compiler erroring out people won't notice that suddenly
> MemoryContextReset deletes much more; leading to possibly hard to find
> errors. Context resets frequently are in error paths, and those won't
> necessarily be hit when running with assertions enabled.

I'd really not even be surprised if a committer backpatches a
MemoryContextReset() addition, not realizing it behaves differently in
the back branches.

How about MemoryContextReset(bool reset_children)?

Greetings,

Andres Freund

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


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

Re: [HACKERS] logical column ordering

[Tom Lane]

Jim Nasby  writes:
> On 2/26/15 4:01 PM, Alvaro Herrera wrote:
>> Josh Berkus wrote:
>>> Oh, I didn't realize there weren't commands to change the LCO.  Without
>>> at least SQL syntax for LCO, I don't see why we'd take it; this sounds
>>> more like a WIP patch.

>> The reason for doing it this way is that changing the underlying
>> architecture is really hard, without having to bear an endless hackers
>> bike shed discussion about the best userland syntax to use.  It seems a
>> much better approach to do the actually difficult part first, then let
>> the rest to be argued to death by others and let those others do the
>> easy part (and take all the credit along with that); that way, that
>> discussion does not kill other possible uses that the new architecture
>> allows.

> +1. This patch is already several years old; lets not delay it further.

I tend to agree with that, but how are we going to test things if there's
no mechanism to create a table in which the orderings are different?

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] logical column ordering

[Andres Freund]

On 2015-02-26 16:16:54 -0600, Jim Nasby wrote:
> On 2/26/15 4:01 PM, Alvaro Herrera wrote:
> >The reason for doing it this way is that changing the underlying
> >architecture is really hard, without having to bear an endless hackers
> >bike shed discussion about the best userland syntax to use.  It seems a
> >much better approach to do the actually difficult part first, then let
> >the rest to be argued to death by others and let those others do the
> >easy part (and take all the credit along with that); that way, that
> >discussion does not kill other possible uses that the new architecture
> >allows.

I agree that it's a sane order of developing things. But: I don't think
it makes sense to commit it without the capability to change the
order. Not so much because it'll not serve a purpose at that point, but
rather because it'll essentially untestable. And this is a patch that
needs a fair amount of changes, both automated, and manual.

At least during development I'd even add a function that randomizes the
physical order of columns, and keeps the logical one. Running the
regression tests that way seems likely to catch a fair number of bugs.

> +1. This patch is already several years old; lets not delay it further.
> 
> Plus, I suspect that you could hack the catalog directly if you really
> wanted to change LCO. Worst case you could create a C function to do it.

I don't think that's sufficient for testing purposes. Not only for the
patch itself, but also for getting it right in new code.

Greetings,

Andres Freund

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


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

Re: [HACKERS] json_populate_record issue - TupleDesc reference leak

[Tom Lane]

Andrew Dunstan  writes:
> This doesn't look quite right. Shouldn't we unconditionally release the 
> Tupledesc before the returns at lines 2118 and 2127, just as we do at 
> the bottom of the function at line 2285?

I think Pavel's patch is probably OK as-is, because the tupdesc returned
by get_call_result_type isn't reference-counted; but I agree the code
would look cleaner your way.  If the main exit isn't bothering to
distinguish this then the early exits should not either.

What I'm wondering about, though, is this bit at line 2125:

/* same logic as for json */
if (!have_record_arg && rec)
PG_RETURN_POINTER(rec);

If that's supposed to be the same logic as in the other path, then how
is it that !have_record_arg has anything to do with whether the JSON
object is empty?  Either the code is broken, or the comment is.

regards, tom lane


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

Re: [HACKERS] Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset

[Andres Freund]

Hi,

On 2015-02-26 17:01:53 -0500, Tom Lane wrote:
> We've discussed doing $SUBJECT off and on for nearly ten years,
> the oldest thread I could find about it being here:
> http://www.postgresql.org/message-id/flat/1186435268.16321.37.ca...@dell.linuxdev.us.dell.com
> It's come up again every time we found another leak of dead child
> contexts, which happened twice last year for example.  And that patch
> I'm pushing for "expanded" out-of-line objects really needs this to
> become the default behavior anywhere that we can detoast objects.

I don't object to the behavioural change per se, rather the contrary,
but I find it a pretty bad idea to change the meaning of an existing
functioname this way. Without a compiler erroring out people won't
notice that suddenly MemoryContextReset deletes much more; leading to
possibly hard to find errors. Context resets frequently are in error
paths, and those won't necessarily be hit when running with assertions
enabled.

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] Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> Thoughts?  Any objections to pushing this?

> Is there any reason at all to keep
> MemoryContextResetButPreserveChildren()?  Since your patch doesn't add
> any callers, it seems pretty likely that there's none anywhere.

The only reason to keep it is to have an "out" if it turns out that some
third-party code actually needs that behavior.

On reflection, maybe a better API to offer for that eventuality is a
function named something like MemoryContextResetOnly(), which would
leave child contexts completely alone.  Then, if you want the old
functionality, you could get it with MemoryContextResetOnly plus
MemoryContextResetChildren.

BTW, the original thread discussed the idea of moving context bookkeeping
blocks into the immediate parent context, but the usefulness of
MemoryContextSetParent() negates the thought that that would be a good
plan.  So there's no real issue here other than potential backwards
compatibility for external code.

regards, tom lane


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

Re: [HACKERS] logical column ordering

[Jim Nasby]

On 2/26/15 4:01 PM, Alvaro Herrera wrote:

Josh Berkus wrote:

On 02/26/2015 01:54 PM, Alvaro Herrera wrote:

This patch decouples these three things so that they
can changed freely -- but provides no user interface to do so.  I think
that trying to only decouple the thing we currently have in two pieces,
and then have a subsequent patch to decouple again, is additional
conceptual complexity for no gain.


Oh, I didn't realize there weren't commands to change the LCO.  Without
at least SQL syntax for LCO, I don't see why we'd take it; this sounds
more like a WIP patch.


The reason for doing it this way is that changing the underlying
architecture is really hard, without having to bear an endless hackers
bike shed discussion about the best userland syntax to use.  It seems a
much better approach to do the actually difficult part first, then let
the rest to be argued to death by others and let those others do the
easy part (and take all the credit along with that); that way, that
discussion does not kill other possible uses that the new architecture
allows.


+1. This patch is already several years old; lets not delay it further.

Plus, I suspect that you could hack the catalog directly if you really 
wanted to change LCO. Worst case you could create a C function to do it.

--
Jim Nasby, Data Architect, Blue Treble Consulting
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] Precedence of standard comparison operators

[Tom Lane]

Andres Freund  writes:
> On February 26, 2015 10:29:18 PM CET, Peter Eisentraut  
> wrote:
>> My suggestion was to treat this like the standard_conforming_string
>> change.  That is, warn for many years before changing.

> I don't think scs is a good example to follow.

Yeah.  For one thing, there wouldn't be any way to suppress the warning
other than to parenthesize your code, which I would find problematic
because it would penalize standard-conforming queries.  I'd prefer an
arrangement whereby once you fix your code to be standard-conforming,
you're done.

A possible point of compromise would be to leave the warning turned on
by default, at least until we get a better sense of how this would
play out in the real world.  I continue to suspect that we're making
a mountain out of, if not a molehill, at least a hillock.  I think most
sane people would have parenthesized their queries to start with rather
than go look up whether IS DISTINCT FROM binds tighter than <= ...

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] Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset

[Alvaro Herrera]

Tom Lane wrote:

> Thoughts?  Any objections to pushing this?

Is there any reason at all to keep
MemoryContextResetButPreserveChildren()?  Since your patch doesn't add
any callers, it seems pretty likely that there's none anywhere.

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

[HACKERS] Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset

[Tom Lane]

We've discussed doing $SUBJECT off and on for nearly ten years,
the oldest thread I could find about it being here:
http://www.postgresql.org/message-id/flat/1186435268.16321.37.ca...@dell.linuxdev.us.dell.com
It's come up again every time we found another leak of dead child
contexts, which happened twice last year for example.  And that patch
I'm pushing for "expanded" out-of-line objects really needs this to
become the default behavior anywhere that we can detoast objects.

So attached is a patch to do it.  I've verified that this passes
"make check-world", not that that proves all that much :-(

I did not make an attempt to
s/MemoryContextResetAndDeleteChildren/MemoryContextReset/ globally,
as it's certainly unnecessary to do that for testing purposes.
I'm not sure whether we should do so, or skip the code churn
(there's about 30 occurrences in HEAD).  We'd want to keep the
macro in any case to avoid unnecessary breakage of 3rd-party code.

Thoughts?  Any objections to pushing this?

regards, tom lane

diff --git a/src/backend/utils/mmgr/README b/src/backend/utils/mmgr/README
index 45e610d..e01bcd4 100644
*** a/src/backend/utils/mmgr/README
--- b/src/backend/utils/mmgr/README
*** lifetimes that only partially overlap ca
*** 125,133 
  from different trees of the context forest (there are some examples
  in the next section).
  
! For convenience we will also want operations like "reset/delete all
! children of a given context, but don't reset or delete that context
! itself".
  
  
  Globally Known Contexts
--- 125,137 
  from different trees of the context forest (there are some examples
  in the next section).
  
! Actually, it turns out that resetting a given context should almost
! always imply deleting (not just resetting) any child contexts it has.
! So MemoryContextReset() means that, and if you really do want a forest of
! empty contexts you need to call MemoryContextResetButPreserveChildren().
! 
! For convenience we also provide operations like "reset/delete all children
! of a given context, but don't reset or delete that context itself".
  
  
  Globally Known Contexts
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 202bc78..f40c33e 100644
*** a/src/backend/utils/mmgr/mcxt.c
--- b/src/backend/utils/mmgr/mcxt.c
*** MemoryContextInit(void)
*** 132,145 
  
  /*
   * MemoryContextReset
   *		Release all space allocated within a context and its descendants,
   *		but don't delete the contexts themselves.
   *
   * The type-specific reset routine handles the context itself, but we
   * have to do the recursion for the children.
   */
  void
! MemoryContextReset(MemoryContext context)
  {
  	AssertArg(MemoryContextIsValid(context));
  
--- 132,176 
  
  /*
   * MemoryContextReset
+  *		Release all space allocated within a context and delete all its
+  *		descendant contexts (but not the named context itself).
+  *
+  * The type-specific reset routine handles the context itself, but we
+  * have to do the recursion for the children.
+  */
+ void
+ MemoryContextReset(MemoryContext context)
+ {
+ 	AssertArg(MemoryContextIsValid(context));
+ 
+ 	/* save a function call in common case where there are no children */
+ 	if (context->firstchild != NULL)
+ 		MemoryContextDeleteChildren(context);
+ 
+ 	/* Nothing to do if no pallocs since startup or last reset */
+ 	if (!context->isReset)
+ 	{
+ 		(*context->methods->reset) (context);
+ 		context->isReset = true;
+ 		VALGRIND_DESTROY_MEMPOOL(context);
+ 		VALGRIND_CREATE_MEMPOOL(context, 0, false);
+ 	}
+ }
+ 
+ /*
+  * MemoryContextResetButPreserveChildren
   *		Release all space allocated within a context and its descendants,
   *		but don't delete the contexts themselves.
   *
+  * Note: this was formerly the behavior of plain MemoryContextReset(), but
+  * we found out that you almost always want to delete children not keep them.
+  * This API may indeed have no use at all, but we keep it for the moment.
+  *
   * The type-specific reset routine handles the context itself, but we
   * have to do the recursion for the children.
   */
  void
! MemoryContextResetButPreserveChildren(MemoryContext context)
  {
  	AssertArg(MemoryContextIsValid(context));
  
*** MemoryContextResetChildren(MemoryContext
*** 171,177 
  	AssertArg(MemoryContextIsValid(context));
  
  	for (child = context->firstchild; child != NULL; child = child->nextchild)
! 		MemoryContextReset(child);
  }
  
  /*
--- 202,208 
  	AssertArg(MemoryContextIsValid(context));
  
  	for (child = context->firstchild; child != NULL; child = child->nextchild)
! 		MemoryContextResetButPreserveChildren(child);
  }
  
  /*
*** MemoryContextDeleteChildren(MemoryContex
*** 226,248 
  }
  
  /*
-  * MemoryContextResetAndDeleteChildren
-  *		Release all space allocated within a context and delete all
-  *		its descendants.
-  *
-  * This is a common combination case where we wan

Re: [HACKERS] Idea: GSoC - Query Rewrite with Materialized Views

[Eric Grinstein]

Thank you for your answers.
I would be very interested in tracking the staleness of the MV.
You see, I work in a research group in database tuning, and we have
implemented some solutions to take advantage of MV's and speed up queries.
The query rewrite feature would be extremely desirable for us.
Do you think that implementing the staleness check as suggested by Thomas
could get us started in the query rewrite business? Do you think I should
make a proposal
or there are more interesting subjects to GSoC? I'd be happy to hear
project suggestions, especially
related to the optimizer, tuning, etc.

Eric

2015-02-20 22:35 GMT-02:00 Tomas Vondra :

> On 21.2.2015 00:20, Kevin Grittner wrote:
> > Tomas Vondra  wrote:
> >
> >> I share the view that this would be very valuable, but the scope
> >> far exceeds what can be done within a single GSoC project. But
> >> maybe we could split that into multiple pieces, and Eric would
> >> implement only the first piece?
> >>
> >> For example the 'is_stale' flag for a MV would be really useful,
> >> making it possible to refresh only the MVs that actually need a
> >> refresh.
> >
> > You may be on to something there.  Frankly, though, I'm not sure
> > that we could even reach consensus within the community on a
> > detailed design for how we intend to track staleness (that will
> > hold up both now and once we have incremental maintenance of
> > materialized views working) within the time frame of a GSoC
> > project.  This would need to be done with an eye toward how it
> > might be used in direct references (will we allow a "staleness
> > limit" on a reference from a query?), for use in a rewrite, and how
> > it will interact with changes to base tables and with both REFRESH
> > statements and incremental maintenance at various levels of
> > "eagerness".  I'm not sure that staleness management wouldn't be
> > better left until we have some of those other parts for it to work
> > with.
>
> Doing that properly is going to be nontrivial, no doubt about that. I
> was thinking about keeping a simple list of updated tables (oids) and
> then at commit time, deciding which MVs to depend on that and setting
> some sort of flag (or XID) for all those MVs. But maybe there's a better
> way.
>
> > Questions to consider:
> >
> > Some other products allow materialized views to be partitioned and
> > staleness to be tracked by partition, and will check which partitions
> > will be accessed in determining staleness. Is that something we want
> > to allow for?
>
> I think we need to get this working for simple MVs, especially because
> we don't have partitioned MVs (or the type of declarative partitioning
> the other products do have).
>
> > Once we have incremental maintenance, an MV maintained in an "eager"
> > fashion (changes are visible in the MV as soon as the transaction
> > modifying the underlying table commit) could be accessed with a MVCC
> > snapshots, with different snapshots seeing different versions. It
> > seems pretty clear that such an MV would always be considered
> > "fresh", so there would be no need to constantly flipping to stale
> > and back again as the underlying table were changed and the changes
> > were reflected in the MV. How do we handle that?
>
> Yes, incrementally updated MVs might be used more easily, without
> tracking staleness. But we don't have that now, and it's going to take a
> significant amount of time to get there.
>
> Also, not all MVs can be updated incrementally, so either we allow only
> simple MVs to be used for rewrites, or we'll have to implement the
> 'stale' flag anyway.
>
> > If changes to an MV are less eager (they are queued for application
> > after COMMIT, as time permits) would we want to track the xid of how
> > far along they are, so that we can tell whether a particular snapshot
> > is safe to use? Do we want to allow a non-MVCC snapshot that shows
> > the latest version of each row? Only if staleness is minimal?
>
> Maybe. When I talk about 'flag' I actually mean a simple way to
> determine whether the MV is up-to-date or not. Snapshots and XIDs are
> probably the right way to do that in MVCC-based system.
>
> > What about MVs which don't have incremental maintenance? We can still
> > determine what xid they are current "as of", from the creation or the
> > latest refresh. Do we want to track that instead of a simple boolean
> > flag?
>
> How would we use the 'as of' XID? IMHO it's unacceptable to quietly use
> stale data unless the user explicitly references the MV, so we'd have to
> assume we can't use that MV.
>
> regards
>
> --
> Tomas Vondrahttp://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] logical column ordering

[Alvaro Herrera]

Josh Berkus wrote:
> On 02/26/2015 01:54 PM, Alvaro Herrera wrote:
> > This patch decouples these three things so that they
> > can changed freely -- but provides no user interface to do so.  I think
> > that trying to only decouple the thing we currently have in two pieces,
> > and then have a subsequent patch to decouple again, is additional
> > conceptual complexity for no gain.
> 
> Oh, I didn't realize there weren't commands to change the LCO.  Without
> at least SQL syntax for LCO, I don't see why we'd take it; this sounds
> more like a WIP patch.

The reason for doing it this way is that changing the underlying
architecture is really hard, without having to bear an endless hackers
bike shed discussion about the best userland syntax to use.  It seems a
much better approach to do the actually difficult part first, then let
the rest to be argued to death by others and let those others do the
easy part (and take all the credit along with that); that way, that
discussion does not kill other possible uses that the new architecture
allows.

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

[HACKERS] GSoC idea - Simulated annealing to search for query plans

[Grzegorz Parka]

Dear Hackers,

I'm Grzegorz Parka, BSc Engineer of Technical Physics and student of
Computer Science at WUT, Poland. Last year I've been a bit into
evolutionary algorithms and during my research I found out about GEQO in
Postgres. I also found out that there are plans to try a different attempt
to find optimal query plans and thought it could be a good thing to use it
as a project for GSoC.

I'm interested in one of old TODO items related to the optimizer -
'Consider compressed annealing to search for query plans'. I believe this
would be potentially beneficial to Postgres to check if such a heuristic
could really choose better query plans than GEQO does. Judging by the
results that simulated annealing gives on the travelling salesman problem,
it looks like a simpler and potentially more effective way of combinatorial
optimization.

As deliverables of such a project I would provide a simple implementation
of basic simulated annealing optimizer and some form of quantitative
comparison with GEQO.

I see that this may be considerably bigger than most of GSoC projects, but
I would like to know your opinion. Do you think that this would be
beneficial enough to make a proper GSoC project? I would also like to know
if you have any additional ideas about this project.

Best regards,
Grzegorz Parka

Re: [HACKERS] logical column ordering

[Josh Berkus]

On 02/26/2015 01:54 PM, Alvaro Herrera wrote:
> This patch decouples these three things so that they
> can changed freely -- but provides no user interface to do so.  I think
> that trying to only decouple the thing we currently have in two pieces,
> and then have a subsequent patch to decouple again, is additional
> conceptual complexity for no gain.

Oh, I didn't realize there weren't commands to change the LCO.  Without
at least SQL syntax for LCO, I don't see why we'd take it; this sounds
more like a WIP patch.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.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] [REVIEW] Re: Compression of full-page-writes

[Rahila Syed]

Hello,

>Even this patch doesn't work fine. The standby emit the following
>error messages.

Yes this bug remains unsolved. I am still working on resolving this.

Following chunk IDs have been added in the attached patch as suggested
upthread.
+#define XLR_CHUNK_BLOCK_REFERENCE  0x10
+#define XLR_CHUNK_BLOCK_HAS_IMAGE  0x04
+#define XLR_CHUNK_BLOCK_HAS_DATA   0x08

XLR_CHUNK_BLOCK_REFERENCE denotes chunk ID of block references.
XLR_CHUNK_BLOCK_HAS_IMAGE is a replacement of BKPBLOCK_HAS_IMAGE
and XLR_CHUNK_BLOCK_HAS DATA a replacement of BKPBLOCK_HAS_DATA.


Thank you,
Rahila Syed


Support-compression-of-full-page-writes-in-WAL_v21.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] logical column ordering

[Alvaro Herrera]

Josh Berkus wrote:
> On 02/26/2015 10:49 AM, Jim Nasby wrote:

> > The other reason for this patch (which it maybe doesn't support
> > anymore?) is to allow Postgres to use an optimal physical ordering of
> > fields on a page to reduce space wasted on alignment, as well as taking
> > nullability and varlena into account.
> 
> ... and that's the bigger reason.  I was under the impression that we'd
> get LCO in first, and then add the column arrangement optimization in
> the next version.

The changes in this patch aren't really optimizations -- they are a
complete rework on the design of storage of columns.  In the current
system, columns exist physically on disk in the same order as they are
created, and in the same order as they appear logically (i.e. SELECT *,
psql's \d, etc).  This patch decouples these three things so that they
can changed freely -- but provides no user interface to do so.  I think
that trying to only decouple the thing we currently have in two pieces,
and then have a subsequent patch to decouple again, is additional
conceptual complexity for no gain.

A future patch can implement physical ordering optimization (group
columns physically to avoid alignment padding, for instance, and also
put not-nullable fixed-length column at the start of the physical tuple,
so that the "attcacheoffset" thing can be applied in more cases), but I
think it's folly to try to attempt that in the current patch, which is
already hugely complicated.

> In fact, I would argue that LCO *needs* to be a feature at least one
> version before we try to add column ordering optimization (COO).  The
> reason being that with LCO, users can implement COO as a client tool or
> extension, maybe even an addition to pg_repack.  This will allow users
> to experiment with, and prove, algorithms for COO, allowing us to put in
> a tested algorithm when we're ready to add it to core.

The current patch will clear the road for such experimentation.

-- 
Á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] Precedence of standard comparison operators

[Andres Freund]

On February 26, 2015 10:29:18 PM CET, Peter Eisentraut  wrote:
>On 2/25/15 5:15 PM, Simon Riggs wrote:
>> Having a warn_if_screwed parameter that we disable by default won't
>> help much because if you are affected you can't change that
>situation.
>> There are too many applications to test all of them and not all
>> applications can be edited, even if they were tested.
>
>My suggestion was to treat this like the standard_conforming_string
>change.  That is, warn for many years before changing.

I don't think scs is a good example to follow. For one it didn't work well. For 
another the impact of scs was imo bigger and had security implications. Also 
people didn't really switch because of it until the default changed. At the 
very least there should be an option to error out, not warn.



--- 
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] Precedence of standard comparison operators

[Peter Eisentraut]

On 2/25/15 5:15 PM, Simon Riggs wrote:
> Having a warn_if_screwed parameter that we disable by default won't
> help much because if you are affected you can't change that situation.
> There are too many applications to test all of them and not all
> applications can be edited, even if they were tested.

My suggestion was to treat this like the standard_conforming_string
change.  That is, warn for many years before changing.



-- 
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] json_populate_record issue - TupleDesc reference leak

[Andrew Dunstan]

On 02/23/2015 12:56 PM, Pavel Stehule wrote:
by the way - this feature is undocumented - I though so only value 
used as type holder is not used.


Should be documented better, - if I understand - it is base stone for 
implementation #= hstore operator


some nice example

postgres=# select json_populate_record('(10,20)'::pt, '{"a":30}');
 json_populate_record
--
 (30,20)
(1 row)

a mentioned bug is corner case - bugfix is simple

diff --git a/src/backend/utils/adt/jsonfuncs.c 
b/src/backend/utils/adt/jsonfuncs.c

new file mode 100644
index a8cdeaa..6e83f78
*** a/src/backend/utils/adt/jsonfuncs.c
--- b/src/backend/utils/adt/jsonfuncs.c
*** populate_record_worker(FunctionCallInfo
*** 2114,2119 
--- 2114,2122 
 */
if (hash_get_num_entries(json_hash) == 0 && rec)
{
+   if (have_record_arg)
+   ReleaseTupleDesc(tupdesc);
+
hash_destroy(json_hash);
PG_RETURN_POINTER(rec);
}







This doesn't look quite right. Shouldn't we unconditionally release the 
Tupledesc before the returns at lines 2118 and 2127, just as we do at 
the bottom of the function at line 2285?


cheers

andrew


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

Re: [HACKERS] Precedence of standard comparison operators

[Tom Lane]

Simon Riggs  writes:
> On 26 February 2015 at 15:56, Tom Lane  wrote:
>> I don't find that particularly attractive either.  It seems quite unlikely
>> to me that anyone would actually use such a hook; replacing the whole
>> parser would be essentially unmaintainable.  Nobody uses the hooks you
>> mention to replace the whole planner or whole executor --- there are
>> wrappers out there, which is a different thing altogether.  But you could
>> not undo the issue at hand here without at the very least a whole new
>> copy of gram.y, which would need to be updated constantly if you wanted
>> it to keep working across more than one release.

> That's not what I see.

> Whole planner replacement is used for extensions by CitusData and NTT,
> and will definitely be used in the future for other apps.

> Whole executor replacement is also used by one extension to produce
> DDL triggers.

That sounds completely silly from here.  You'd be better off maintaining
your code as a set of patches on the core code and shipping your own
binaries.  It would be substantially less work to maintain, I'd think,
because otherwise you have to track every single patch made to what are
very large swaths of code.  The costs of merge resolution (when your
changes specifically touch something also updated by the community) would
be about the same, but the gruntwork aspect would be considerably better.
And you'd have to be certifiably insane to ship such things as extensions
not tied to specific core binaries, anyway, so I'm not seeing an upside
in doing it like that.

Having said that, I won't stand in the way of somebody else proposing
parser hooks.  I don't intend to do it myself though.

regards, tom lane


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

Re: [HACKERS] Precedence of standard comparison operators

[Simon Riggs]

On 26 February 2015 at 15:56, Tom Lane  wrote:

>> I think the way to do this is to have a pluggable parser, so users can
>> choose 1) old parser, 2) new, better parser, 3) any other parser they
>> fancy that they maintain to ensure application compatibility. We've
>> got a pluggable executor and optimizer, so why not a parser too?
>> Implementing that in the same way we have done for executor and
>> optimizer is a 1 day patch, so easily achievable for 9.5.
>
> I don't find that particularly attractive either.  It seems quite unlikely
> to me that anyone would actually use such a hook; replacing the whole
> parser would be essentially unmaintainable.  Nobody uses the hooks you
> mention to replace the whole planner or whole executor --- there are
> wrappers out there, which is a different thing altogether.  But you could
> not undo the issue at hand here without at the very least a whole new
> copy of gram.y, which would need to be updated constantly if you wanted
> it to keep working across more than one release.

That's not what I see.

Whole planner replacement is used for extensions by CitusData and NTT,
and will definitely be used in the future for other apps.

Whole executor replacement is also used by one extension to produce
DDL triggers.

In any case, whole planner replacement would be very desirable for
people trying to minimize code churn in their applications. As soon as
it exists, I will use to make some MySQL-only apps work seamlessly
against PostgreSQL, such as WordPress. It doesn't need to be 100%
perfect MySQL, it just needs to be enough to make the app work.
Maintaining a code base that can work against multiple databases is
hard. Writing it against one main database and maintaining a parser
layer would be much easier.

-- 
 Simon Riggs   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, RemoteDBA, 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] plpgsql versus domains

[Tom Lane]

Andres Freund  writes:
> On 2015-02-26 13:53:18 -0500, Tom Lane wrote:
>> I thought that's what I was proposing in point #1.

> Sure, but my second point was to avoid duplicating that information into
> ->fcinfo and such and instead reference the typecache entry from
> there. So that if the typecache entry is being rebuilt (a new mechanism
> I'm afraid), whatever uses ->fcinfo gets the updated
> functions/definition.

The trick there would be that if we don't want to copy then we'd need a
reference counting mechanism, and FmgrInfos aren't used in a way that
would allow that to work easily.  (Whatever the typcache is holding onto
clearly must be long-lived, but you do want an obsoleted one to go away
once there are no more FmgrInfos referencing it.)

I was just thinking though that we could possibly solve that if we went
ahead and invented the memory context reset callback mechanism that
I suggested in another thread a week or two back.  Then you could imagine
that when a domain-checking function caches a reference to a "domain
constraints info" object in its FmgrInfo, it could increment a refcount
on the info object, and register a callback on the context containing the
FmgrInfo to release that refcount.

A different approach would be to try to use the ResourceOwner mechanism
to track these info objects.  But I think that does not work nicely for
plpgsql; at least not unless it creates a ResourceOwner for every
function, which seems kinda messy.

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] logical column ordering

[Josh Berkus]

On 02/26/2015 10:49 AM, Jim Nasby wrote:
> On 2/23/15 5:09 PM, Tomas Vondra wrote:
>> Over the time I've heard various use cases for this patch, but in most
>> cases it was quite speculative. If you have an idea where this might be
>> useful, can you explain it here, or maybe point me to a place where it's
>> described?
> 
> For better or worse, table structure is a form of documentation for a
> system. As such, it's very valuable to group related fields in a table
> together. When creating a table, that's easy, but as soon as you need to
> alter your careful ordering can easily end up out the window.
> 
> Perhaps to some that just sounds like pointless window dressing, but my
> experience is that on a complex system the less organized things are the
> more bugs you get due to overlooking something.

Yes.  Consider a BI table which has 110 columns.  Having these columns
in a sensible order,even though some were added after table creation,
would be a significant usability benefit for DBAs.

A second usability benefit is making it easy to keep table columns for
import tables sync'd with COPY.

Neither of these is sufficient to overshadow performance penalties, but
they are both common activities/annoyances, and not speculative in the
least.  And I haven't heard that there are any performance issues
associated with this patch.  Are there?

> The other reason for this patch (which it maybe doesn't support
> anymore?) is to allow Postgres to use an optimal physical ordering of
> fields on a page to reduce space wasted on alignment, as well as taking
> nullability and varlena into account.

... and that's the bigger reason.  I was under the impression that we'd
get LCO in first, and then add the column arrangement optimization in
the next version.

In fact, I would argue that LCO *needs* to be a feature at least one
version before we try to add column ordering optimization (COO).  The
reason being that with LCO, users can implement COO as a client tool or
extension, maybe even an addition to pg_repack.  This will allow users
to experiment with, and prove, algorithms for COO, allowing us to put in
a tested algorithm when we're ready to add it to core.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.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] plpgsql versus domains

[Pavel Stehule]

2015-02-26 19:53 GMT+01:00 Tom Lane :

> Andres Freund  writes:
> > On 2015-02-26 12:31:46 -0500, Tom Lane wrote:
> >> 2. In plpgsql, explicitly model a domain type as being its base type
> >> plus some constraints --- that is, stop depending on domain_in() to
> >> enforce the constraints, and do it ourselves.  This would mean that
> >> the FmgrInfo in a PLpgSQL_type struct would reference the base type's
> >> input function rather than domain_in, so we'd not have to be afraid
> >> to cache that.  The constraints would be represented as a separate list,
> >> which we'd arrange to fetch from the typcache once per transaction.
>
> > Hm. A bit sad to open code that in plpgsql instead of making it
> > available more generally.
>
> The actual checking wouldn't be open-coded, it would come from
> domain_check() (or possibly a modified version of that).  It is true
> that plpgsql would know more about domains than it does today, but
> I'm not sure I see another use-case for this type of logic.
>
> To some extent this is a workaround for the fact that plpgsql does type
> conversions the way it does (ie, by I/O rather than by using the parser's
> cast mechanisms).  We've talked about changing that, but people seem to
> be afraid of the compatibility consequences, and I'm not planning to fight
> two compatibility battles concurrently ;-)
>

I understand, but in this case, a compatibility can be enforced simply - we
can support "a only know cast" mode and  "IO cast" mode.

IO cast is simple for lot of people, but there is a lot of performance
issues and few errors related to this topic. But this is offtopic now.

But, what can be related - for plpgsql_check is necessary some simple check
of a assigning - that should to return error or warning

This part of plpgsql_check is too complex now.


Regards

Pavel


> > You're probably going to kill me because of the increased complexity,
> > but how about making typecache.c smarter, more in the vein of
> > relcache.c, and store the relevant info in there?
>
> I thought that's what I was proposing in point #1.
>
> 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] plpgsql versus domains

[Andres Freund]

On 2015-02-26 13:53:18 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > Hm. A bit sad to open code that in plpgsql instead of making it
> > available more generally.
> 
> The actual checking wouldn't be open-coded, it would come from
> domain_check() (or possibly a modified version of that).  It is true
> that plpgsql would know more about domains than it does today, but

It'd still teach plpgsql more about types than it knows right now. But
on a second thought that ship has sailed long ago - and the amount of
knowledge seems relatively small. There's much more stuff about
composites there already.

> and I'm not planning to fight two compatibility battles concurrently ;-)

Heh ;)

> > You're probably going to kill me because of the increased complexity,
> > but how about making typecache.c smarter, more in the vein of
> > relcache.c, and store the relevant info in there?
> 
> I thought that's what I was proposing in point #1.

Sure, but my second point was to avoid duplicating that information into
->fcinfo and such and instead reference the typecache entry from
there. So that if the typecache entry is being rebuilt (a new mechanism
I'm afraid), whatever uses ->fcinfo gets the updated
functions/definition.

Greetings,

Andres Freund

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


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

Re: [HACKERS] plpgsql versus domains

[Tom Lane]

Andres Freund  writes:
> On 2015-02-26 12:31:46 -0500, Tom Lane wrote:
>> 2. In plpgsql, explicitly model a domain type as being its base type
>> plus some constraints --- that is, stop depending on domain_in() to
>> enforce the constraints, and do it ourselves.  This would mean that
>> the FmgrInfo in a PLpgSQL_type struct would reference the base type's
>> input function rather than domain_in, so we'd not have to be afraid
>> to cache that.  The constraints would be represented as a separate list,
>> which we'd arrange to fetch from the typcache once per transaction.

> Hm. A bit sad to open code that in plpgsql instead of making it
> available more generally.

The actual checking wouldn't be open-coded, it would come from
domain_check() (or possibly a modified version of that).  It is true
that plpgsql would know more about domains than it does today, but
I'm not sure I see another use-case for this type of logic.

To some extent this is a workaround for the fact that plpgsql does type
conversions the way it does (ie, by I/O rather than by using the parser's
cast mechanisms).  We've talked about changing that, but people seem to
be afraid of the compatibility consequences, and I'm not planning to fight
two compatibility battles concurrently ;-)

> You're probably going to kill me because of the increased complexity,
> but how about making typecache.c smarter, more in the vein of
> relcache.c, and store the relevant info in there?

I thought that's what I was proposing in point #1.

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] logical column ordering

[Jim Nasby]

On 2/23/15 5:09 PM, Tomas Vondra wrote:

Over the time I've heard various use cases for this patch, but in most
cases it was quite speculative. If you have an idea where this might be
useful, can you explain it here, or maybe point me to a place where it's
described?


For better or worse, table structure is a form of documentation for a 
system. As such, it's very valuable to group related fields in a table 
together. When creating a table, that's easy, but as soon as you need to 
alter your careful ordering can easily end up out the window.


Perhaps to some that just sounds like pointless window dressing, but my 
experience is that on a complex system the less organized things are the 
more bugs you get due to overlooking something.


The other reason for this patch (which it maybe doesn't support 
anymore?) is to allow Postgres to use an optimal physical ordering of 
fields on a page to reduce space wasted on alignment, as well as taking 
nullability and varlena into account.

--
Jim Nasby, Data Architect, Blue Treble Consulting
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] plpgsql versus domains

[Andres Freund]

Hi,

On 2015-02-26 12:31:46 -0500, Tom Lane wrote:
> It's really slow, because any assignment to a domain variable goes through
> the slow double-I/O-conversion path in exec_cast_value, even if no actual
> work is needed.

Hm, that's surely not nice for some types. Doing syscache lookups every
time can't help either.

> And I noticed that the domain's constraints get looked up
> only once per function per session; for example this script gets
> unexpected results:
> The reason for this is that domain_in looks up the domain's constraints
> and caches them under the calling FmgrInfo struct.

That's probably far from the only such case we have :(

> 1. Arrange to cache the constraints for domain types in typcache.c,
> so as to reduce the number of trips to the actual catalogs.  I think
> typcache.c will have to flush these cache items upon seeing any sinval
> change in pg_constraint, but that's still cheaper than searching
> pg_constraint for every query.

That sounds sane.

> 2. In plpgsql, explicitly model a domain type as being its base type
> plus some constraints --- that is, stop depending on domain_in() to
> enforce the constraints, and do it ourselves.  This would mean that
> the FmgrInfo in a PLpgSQL_type struct would reference the base type's
> input function rather than domain_in, so we'd not have to be afraid
> to cache that.  The constraints would be represented as a separate list,
> which we'd arrange to fetch from the typcache once per transaction.

Hm. A bit sad to open code that in plpgsql instead of making it
available more generally.

> 3. We could still have domains.c responsible for most of the details;
> the domain_check() API may be good enough as-is, though it seems to lack
> any simple way to force re-lookup of the domain constraints once per xact.
> 
> Thoughts, better ideas?

You're probably going to kill me because of the increased complexity,
but how about making typecache.c smarter, more in the vein of
relcache.c, and store the relevant info in there? And then, to avoid
repeated lookups, store a reference to that in fcinfo? The lifetime of
objects wouldn't be entirely trivial, but it doesn't sound impossible.

Greetings,

Andres Freund

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


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

Re: [HACKERS] How to create virtual indexes on postgres

[Jim Nasby]

On 2/26/15 6:17 AM, Sreerama Manoj wrote:

But, it runs with  Postgres 9.1 version...But I use 9.4..I think I cant
use that. Or as an alternative Is there any provision in postgres to
know use(Increase in Performance) of an index before creating that index.


No. It might not be too hard to port the hypothetical index work to 9.4 
though.


Also, just to let you know, this is really a topic for pgsql-general, 
not pgsql-hackers. It's also best to reply to list emails in-line, or at 
the bottom, not at the top.

--
Jim Nasby, Data Architect, Blue Treble Consulting
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] Partitioning WIP patch

[Andres Freund]

On 2015-02-26 12:15:17 +0900, Amit Langote wrote:
> On 26-02-2015 AM 05:15, Josh Berkus wrote:
> > On 02/24/2015 12:13 AM, Amit Langote wrote:
> >> Here is an experimental patch that attempts to implement this.

> > I would love to have it for 9.5, but I guess the
> > patch isn't nearly baked enough for that?

> I'm not quite sure what would qualify as baked enough for 9.5 though we
> can surely try to reach some consensus on various implementation aspects
> and perhaps even get it ready in time for 9.5.

I think it's absolutely unrealistic to get this into 9.5. There's barely
been any progress on the current (last!) commitfest - where on earth
should the energy come to make this patch ready? And why would that be
fair against all the others that have submitted in time?

Greetings,

Andres Freund

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


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

Re: [HACKERS] Partitioning WIP patch

[Josh Berkus]

On 02/25/2015 07:15 PM, Amit Langote wrote:
> On 26-02-2015 AM 05:15, Josh Berkus wrote:
>> On 02/24/2015 12:13 AM, Amit Langote wrote:
>>> Here is an experimental patch that attempts to implement this.
>>
>> This looks awesome. 
> 
> Thanks!
> 
>> I would love to have it for 9.5, but I guess the
>> patch isn't nearly baked enough for that?
>>
> 
> I'm not quite sure what would qualify as baked enough for 9.5 though we
> can surely try to reach some consensus on various implementation aspects
> and perhaps even get it ready in time for 9.5.

Well, we don't have long at all to do that.  I guess I'm asking what
kind of completeness of code we have; is this basically done pending API
changes and bugs, or are there major bits (like, say, pg_dump  and
EXPLAIN support) which are completely unimplemented?

>>> where key_spec consists of partition key column names and optional
>>> operator class per column. Currently, there are restrictions on the
>>> key_spec such as allowing only column names (not arbitrary expressions
>>> of them), only one column for list strategy, etc.
>>
>> What's the obstacle to supporting expressions and/or IMMUTABLE
>> functions?  I think it's fine to add this feature without them
>> initially, I'm just asking about the roadmap for eventually supporting
>> expressions in the key spec.
>>
> 
> Only one concern I can remember someone had raised is that having to
> evaluate an expression for every row during bulk-inserts may end up
> being pretty expensive. Though, we might have to live with that.

Well, it's not more expensive than having to materialize the value from
a trigger and store it on disk.  The leading one here would be functions
over timestamp; for example, the data has a timestamptz, but you want to
partition by week.

> 
> I think one idea is to learn from ability to use expressions in indexes.

Sure.  So a feature to implement for the 2nd release.


-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.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] Partitioning WIP patch

[Jim Nasby]

On 2/26/15 3:22 AM, Andres Freund wrote:

On 2015-02-26 02:20:21 -0600, Jim Nasby wrote:

The reason I'd like to do this with partitioning vs plain inheritance is
presumably as we build out partitioning we'll get very useful things like
the ability to have FKs to properly partitioned tables. Insert tuple routing
could also be useful.


The problem there imo isn't so much inheritance, but lack of working
unique checks across partitions. That's something we can implement
independent of this, it's just not trivial.


There's been discussion of allowing for uniqueness when we can guarantee 
no overlap between partitions, and the partition key is part of the 
unique constraint. That's the particular use case I was thinking of.


I suspect there's other partitioning features that would be useful in a 
generic inheritance setup as well; that's why I'd love to see both 
features work together... but I fear there's enough work to get there 
that it may not happen, and I don't want us to accidentally start mixing 
the two and have users start relying on it.

--
Jim Nasby, Data Architect, Blue Treble Consulting
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] mogrify and indent features for jsonb

[Josh Berkus]

On 02/26/2015 07:25 AM, Thom Brown wrote:
> Yeah, I think that may be problematic.  I agree with Josh that there's
> probably no sane mix of operators for this, as I would expect your
> example to replace "d": ["aa","bb","cc","dd"] with "d": ["ee"] rather
> than append to it.  Hmm... unless we used a + operator, but then I'm not
> sure what should happen in the instance of '{"d": ["aa"]}' + '{"d": "bb"}'.

Yeah, that's what I would expect too.  In fact, I could would count on
replacement.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.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] Partitioning WIP patch

[Jim Nasby]

On 2/26/15 3:09 AM, Amit Langote wrote:

Yes. If it helps, the exact use-case I have in mind is using list-based
>partitioning + additional columns in some/all children (different
>between children). For example, if you need to track different types of
>customer payment methods, you'd have a payment parent table, a list
>partition for credit & debit cards, a different list partition for bank
>accounts, etc.
>
>The reason I'd like to do this with partitioning vs plain inheritance is
>presumably as we build out partitioning we'll get very useful things
>like the ability to have FKs to properly partitioned tables. Insert
>tuple routing could also be useful.
>

Unless I'm missing something again, isn't allowing partitions to have
heterogeneous rowtypes a problem in the long run? I'm afraid I'm
confused as to your stand regarding inheritance vs. new partitioning. To
be specific, children with heterogeneous schemas sounds much like what
inheritance would be good for as you say. But then isn't that why we
have to plan children individually which you said new partitioning
should get away from?


Apologies if I haven't been clear enough. What I'd like to see is the 
best of both worlds; fast partitioning when not using inheritance, and 
perhaps somewhat slower when using inheritance, but still with the 
features partitioning gives you.


But my bigger concern from a project standpoint is that we not put 
ourselves in a position of supporting something that we really don't 
want to support (a partitioning system that's got inheritance mixed in). 
As much as I'd personally like to have both features together, I think 
it would be bad for the community to go down that road without careful 
thought.



>>With explicit partitioning, shouldn't we go in direction where we remove
>>some restrictions imposed by inheritance (think multiple inheritance)? I
>>recall a link Alvaro had started the discussion with think link to a
>>Tom's remark about something very related:
>>
>>http://www.postgresql.org/message-id/1598.1399826...@sss.pgh.pa.us

>
>That post looks like Tom figured out a way to eliminate a problem that
>hurts inheritance, so that's good.
>
>My fear is that at some point we'll hit a problem with partitioning that
>we can't solve in the inheritance model. If we allow inheritance
>features into partitioning now we'll painted into a corner. If we
>disallow those features now we can always re-enable them if we get to
>the point where we're in the clear.
>
>Does that make sense?

Yes, it does. In fact, I do intend to keep them separate the first
attempt of which is to choose to NOT transform a PARTITION OF parent
clause into INHERITS parent. Any code that may look like it's trying to
do that is because the patch is not fully baked yet.


Ok, good to know. That's why I was asking about ALTER TABLE ADD COLUMN 
on a partition. If we release something without that being restricted 
it'll probably cause trouble later on.

--
Jim Nasby, Data Architect, Blue Treble Consulting
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] plpgsql versus domains

[Pavel Stehule]

Hi

2015-02-26 18:31 GMT+01:00 Tom Lane :

> At the behest of Salesforce, I've been looking into improving plpgsql's
> handling of variables of domain types, which is currently pretty awful.
> It's really slow, because any assignment to a domain variable goes through
> the slow double-I/O-conversion path in exec_cast_value, even if no actual
> work is needed.  And I noticed that the domain's constraints get looked up
> only once per function per session; for example this script gets
> unexpected results:
>
> create domain di as int;
>
> create function foo1(int) returns di as $$
> declare d di;
> begin
>   d := $1;
>   return d;
> end
> $$ language plpgsql immutable;
>
> select foo1(0); -- call once to set up cache
>
> alter domain di add constraint pos check (value > 0);
>
> select 0::di;   -- fails, as expected
>
> select foo1(0); -- should fail, does not
>
> \c -
>
> select foo1(0); -- now it fails
>
> The reason for this is that domain_in looks up the domain's constraints
> and caches them under the calling FmgrInfo struct.  That behavior was
> designed to ensure we'd do just one constraint-lookup per query, which
> I think is reasonable.  But plpgsql sets up an FmgrInfo in the variable's
> PLpgSQL_type record, which persists for the whole session unless the
> function's parse tree is flushed for some reason.  So the constraints
> only get looked up once.
>
> The rough sketch I have in mind for fixing this is:
>
> 1. Arrange to cache the constraints for domain types in typcache.c,
> so as to reduce the number of trips to the actual catalogs.  I think
> typcache.c will have to flush these cache items upon seeing any sinval
> change in pg_constraint, but that's still cheaper than searching
> pg_constraint for every query.
>
> 2. In plpgsql, explicitly model a domain type as being its base type
> plus some constraints --- that is, stop depending on domain_in() to
> enforce the constraints, and do it ourselves.  This would mean that
> the FmgrInfo in a PLpgSQL_type struct would reference the base type's
> input function rather than domain_in, so we'd not have to be afraid
> to cache that.  The constraints would be represented as a separate list,
> which we'd arrange to fetch from the typcache once per transaction.
> (I think checking for new constraints once per transaction is sufficient
> for reasonable situations, though I suppose that could be argued about.)
> The advantage of this approach is first that we could avoid an I/O
> conversion when the incoming value to be assigned matches the domain's
> base type, which is the typical case; and second that a domain without
> any CHECK constraints would become essentially free, which is also a
> common case.
>

I like this variant

There can be some good optimization with  scalar types: text, varchars,
numbers and reduce IO cast.



>
> 3. We could still have domains.c responsible for most of the details;
> the domain_check() API may be good enough as-is, though it seems to lack
> any simple way to force re-lookup of the domain constraints once per xact.
>
> Thoughts, better ideas?
>
> Given the lack of field complaints, I don't feel a need to try to
> back-patch a fix for this, but I do want to see it fixed for 9.5.
>

+1

Regards

Pavel


>
> 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] Reduce pinning in btree indexes

[Kevin Grittner]

Heikki Linnakangas  wrote:> On 02/15/2015 02:19 AM, 
Kevin Grittner wrote:
>> Interestingly, the btree README points out that using the old TID
>> with a new tuple poses no hazard for a scan using an MVCC snapshot,
>> because the new tuple would not be visible to a snapshot created
>> that long ago.
>
> The first question is: Do we really need that interlock for the non-MVCC 

> snapshots either?

We don't need the interlock for non-MVCC snapshots if the conditions we
limit or filter on at the index level are rechecked when we get to the heap
tuple; otherwise we do.

> If we do: For non-MVCC snapshots, we need to ensure that all index scans 

> that started before the VACUUM did complete before the VACUUM does.

Isn't it enough to be sure that if we're not going to recheck any index
tests against the heap tuple that any process-local copies of index entries
which exist at the start of the index scan phase of the vacuum are not used
after the vacuum enters the phase of freeing line pointers -- that is, any
scan which has read a page when the vacuum starts to scan the index moves
on to another page before vacuum finishes scanning that index?


> I wonder if we could find a different mechanism to enforce that. Using the 

> pin-interlock for that has always seemed a bit silly to me.

It certainly is abusing the semantics of the pin to treat it as a type of
lock on the contents.


> Perhaps grab a new heavy-weight lock on the table whenever a non-MVCC index
> scan on  the table begins, and have VACUUM wait on it.


-1

The problem I'm most interested in fixing based on user feedback is a vacuum
blocking when a cursor which is using an index scan is sitting idle.  Not
only does the vacuum of that table stall, but if it is an autovacuum worker,
that worker is prevented from cleaning up any other tables.  I have seen all
autovacuum workers blocked in exactly this way, leading to massive bloat.
What you suggest is just using a less awkward way to keep the problem.

>> I found that the LP_DEAD hinting
>> would be a problem with an old TID, but I figured we could work
>> around that by storing the page LSN into the scan position structure
>> when the page contents were read, and only doing hinting if that
>> matched when we were ready to do the hinting.  That wouldn't work
>> for an index which was not WAL-logged, so the patch still holds
>> pins for those.
>

> Or you could use GetFakeLSNForUnloggedRel().

I'm unfamiliar with that, but will take a look.  (I will be back at
my usual development machine late tomorrow.)

>> Robert pointed out that the visibility information
>> for an index-only scan wasn't checked while the index page READ
>> lock was held, so those scans also still hold the pins.
>

> Why does an index-only scan need to hold the pin?

Robert already answered this one, but there is a possible solution
-- provide some way to check the heap's visibility map for the TIDs
read from an index page before releasing the read lock on that page.

>> Finally, there was an "optimization" for marking buffer position
>> for possible restore that was incompatible with releasing the pin.
>> I use quotes because the optimization adds overhead to every move
>> to the next page in order set some variables in a structure when a
>> mark is requested instead of running two fairly small memcpy()
>> statements.  The two-day benchmark of the customer showed no
>> performance hit, and looking at the code I would be amazed if the
>> optimization yielded a measurable benefit.  In general, optimization
>> by adding overhead to moving through a scan to save time in a mark
>> operation seems dubious.
>
> Hmm. Did your test case actually exercise mark/restore? The memcpy()s 
> are not that small. Especially if it's an index-only scan, you're 
> copying a large portion of the page. Some scans call markpos on every 
> tuple, so that could add up.


I have results from the `make world` regression tests and a 48-hour
customer test.  Unfortunately I don't know how heavily either of those
exercise this code.  Do you have a suggestion for a way to test whether
there is a serious regression for something approaching a "worst case"?

>> At some point we could consider building on this patch to recheck
>> index conditions for heap access when a non-MVCC snapshot is used,
>> check the visibility map for referenced heap pages when the TIDs
>> are read for an index-only scan, and/or skip LP_DEAD hinting for
>> non-WAL-logged indexes.  But all those are speculative future work;
>> this is a conservative implementation that just didn't modify
>> pinning where there were any confounding factors.
>
> Understood. Still, I'd like to see if we can easily get rid of the 
> pinning altogether.


That would be great, but I felt that taking care of the easy cases in
on patch and following with patches to handle the trickier cases as
separate follow-on patches made more sense than trying to do everything
at once.  Assuming we sort out the mark/restore issue

Re: [HACKERS] Performance improvement for joins where outer side is unique

[Tomas Vondra]

On 26.2.2015 18:34, Tom Lane wrote:
> Tomas Vondra  writes:
>> FWIW this apparently happens because the code only expect that
>> EquivalenceMembers only contain Var, but in this particular case that's
>> not the case - it contains RelabelType (because oprcode is regproc, and
>> needs to be relabeled to oid).
> 
> If it thinks an EquivalenceMember must be a Var, it's outright
> broken; I'm pretty sure any nonvolatile expression is possible.

I came to the same conclusion, because even with the RelabelType fix
it's trivial to crash it with a query like this:

SELECT 1 FROM pg_proc p JOIN pg_operator o
  ON oprcode = (p.oid::int4 + 1);

I think fixing this (e.g. by restricting the optimization to Var-only
cases) should not be difficult, although it might be nice to handle
generic expressions too, and something like examine_variable() should do
the trick. But I think UNIQUE indexes on expressions are not all that
common.

-- 
Tomas Vondrahttp://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] Precedence of standard comparison operators

[Tom Lane]

Andrew Dunstan  writes:
> On 02/26/2015 10:56 AM, Tom Lane wrote:
>> I find this argument to be unhelpful, because it could be made in exactly
>> the same words against any non-backwards-compatible change whatsoever.
>> Nonetheless, we do make non-backwards-compatible changes all the time.

> That's true, we do. But finding out where apps are going to break is not 
> going to be easy. Reviewing a million lines of code to examine where 
> changes in operator precendence might affect you could be an enormous 
> undertaking for many users. I understand the need, but the whole 
> prospect makes me very, very nervous, TBH.

Well, again, it's not apparent to me why such arguments can't be used
to prevent us from ever again making any user-visible semantics change.

In this particular case, given I've gone to the trouble of creating a
warning mode, I think it would be easier to find potential problems than
it is for many of the compatibility changes we've made in the past.
A not-terribly-far-away example is your own recent change in casting
timestamp values to JSON; if someone had demanded a way to audit their
applications to find places where that might break things, could that
patch have been accepted?  Indeed, could *any* of the backwards
compatibility breaks called out in the 9.4 notes have passed such a test?
I'm not honestly sure why we're holding this particular change to such
a high standard.

(Also, I think that most cases in practice are going to end up as parse
errors, not silently different query results, though I admit I haven't
got much evidence one way or the other on that.  It'd be useful perhaps
if someone tried some large existing application against the patch to
see what happens.  How many parse failures come up, and how many
warnings?)

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] Performance improvement for joins where outer side is unique

[Tom Lane]

Tomas Vondra  writes:
> FWIW this apparently happens because the code only expect that
> EquivalenceMembers only contain Var, but in this particular case that's
> not the case - it contains RelabelType (because oprcode is regproc, and
> needs to be relabeled to oid).

If it thinks an EquivalenceMember must be a Var, it's outright broken;
I'm pretty sure any nonvolatile expression is possible.

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] plpgsql versus domains

[Tom Lane]

At the behest of Salesforce, I've been looking into improving plpgsql's
handling of variables of domain types, which is currently pretty awful.
It's really slow, because any assignment to a domain variable goes through
the slow double-I/O-conversion path in exec_cast_value, even if no actual
work is needed.  And I noticed that the domain's constraints get looked up
only once per function per session; for example this script gets
unexpected results:

create domain di as int;

create function foo1(int) returns di as $$
declare d di;
begin
  d := $1;
  return d;
end
$$ language plpgsql immutable;

select foo1(0); -- call once to set up cache

alter domain di add constraint pos check (value > 0);

select 0::di;   -- fails, as expected

select foo1(0); -- should fail, does not

\c -

select foo1(0); -- now it fails

The reason for this is that domain_in looks up the domain's constraints
and caches them under the calling FmgrInfo struct.  That behavior was
designed to ensure we'd do just one constraint-lookup per query, which
I think is reasonable.  But plpgsql sets up an FmgrInfo in the variable's
PLpgSQL_type record, which persists for the whole session unless the
function's parse tree is flushed for some reason.  So the constraints
only get looked up once.

The rough sketch I have in mind for fixing this is:

1. Arrange to cache the constraints for domain types in typcache.c,
so as to reduce the number of trips to the actual catalogs.  I think
typcache.c will have to flush these cache items upon seeing any sinval
change in pg_constraint, but that's still cheaper than searching
pg_constraint for every query.

2. In plpgsql, explicitly model a domain type as being its base type
plus some constraints --- that is, stop depending on domain_in() to
enforce the constraints, and do it ourselves.  This would mean that
the FmgrInfo in a PLpgSQL_type struct would reference the base type's
input function rather than domain_in, so we'd not have to be afraid
to cache that.  The constraints would be represented as a separate list,
which we'd arrange to fetch from the typcache once per transaction.
(I think checking for new constraints once per transaction is sufficient
for reasonable situations, though I suppose that could be argued about.)
The advantage of this approach is first that we could avoid an I/O
conversion when the incoming value to be assigned matches the domain's
base type, which is the typical case; and second that a domain without
any CHECK constraints would become essentially free, which is also a
common case.

3. We could still have domains.c responsible for most of the details;
the domain_check() API may be good enough as-is, though it seems to lack
any simple way to force re-lookup of the domain constraints once per xact.

Thoughts, better ideas?

Given the lack of field complaints, I don't feel a need to try to
back-patch a fix for this, but I do want to see it fixed for 9.5.

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] pgaudit - an auditing extension for PostgreSQL

[David Steele]

On 2/26/15 1:00 AM, Fujii Masao wrote:
> On Thu, Feb 26, 2015 at 1:40 PM, Alvaro Herrera
>> Clearly if you log only DROP TABLE, and then the malicious user hides
>> one such call inside a function that executes the drop (which is called
>> via a SELECT top-level SQL), you're not going to be happy.
> 
> Yep, so what SQL should be logged in this case? Only "targeted" nested DDL?
> Both top and nested ones? Seems the later is better to me.
> 
> What about the case where the function A calls the function B executing DDL?
> Every ancestor SQLs of the "targeted" DDL should be logged? Probably yes.

Currently only the targeted nested DDL would be logged.  However, it
would log the top-level statement as well as the object that was dropped.

Here's an example from the unit tests:

do $$
begin
create table test_block (id int);
drop table test_block;
end; $$

When pg_audit.log = 'function, ddl' the output will be:

AUDIT: SESSION,FUNCTION,DO,,,do $$ begin create table test_block (id
int); drop table test_block; end; $$
AUDIT: SESSION,DDL,CREATE TABLE,TABLE,public.test_block,do $$ begin
create table test_block (id int); drop table test_block; end; $$
AUDIT: SESSION,DDL,DROP TABLE,TABLE,public.test_block,do $$ begin
create table test_block (id int); drop table test_block; end; $$

You can see that in the create and drop audit entries the
fully-qualified name is given.  The statement comes from
debug_query_string so it shows the top-level statement, even though more
detail is given in the other fields when possible.

If pg_audit.log = 'ddl' then the DO entry would not be logged even
though statements under it were logged.

At least, that's the way it works currently.

-- 
- David Steele
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Precedence of standard comparison operators

[Andrew Dunstan]

On 02/26/2015 10:56 AM, Tom Lane wrote:

Simon Riggs  writes:

On 20 February 2015 at 20:44, Tom Lane  wrote:

Well, assuming that we're satisfied with just having a way to warn
when the behavior changed (and not, in particular, a switch that can
select old or new behavior)

I'm in favour of your proposed improvements, but I'm having a problem
thinking about random application breakage that would result.
Having a warn_if_screwed parameter that we disable by default won't
help much because if you are affected you can't change that situation.
There are too many applications to test all of them and not all
applications can be edited, even if they were tested.

I find this argument to be unhelpful, because it could be made in exactly
the same words against any non-backwards-compatible change whatsoever.
Nonetheless, we do make non-backwards-compatible changes all the time.




That's true, we do. But finding out where apps are going to break is not 
going to be easy. Reviewing a million lines of code to examine where 
changes in operator precendence might affect you could be an enormous 
undertaking for many users. I understand the need, but the whole 
prospect makes me very, very nervous, TBH.


cheers

andrew



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

Re: [HACKERS] Performance improvement for joins where outer side is unique

[Tomas Vondra]

On 26.2.2015 01:15, Tomas Vondra wrote:
>
> I'm not quite sure why, but eclassjoin_is_unique_join() never actually
> jumps into this part (line ~400):
> 
> if (relvar != NULL && candidaterelvar != NULL)
> {
> ...
> index_vars = lappend(index_vars, candidaterelvar);
> ...
> }
> 
> so the index_vars is NIL. Not sure why, but I'm sure you'll spot the
> issue right away.

FWIW this apparently happens because the code only expect that
EquivalenceMembers only contain Var, but in this particular case that's
not the case - it contains RelabelType (because oprcode is regproc, and
needs to be relabeled to oid).

Adding this before the IsA(var, Var) check fixes the issue


if (IsA(var, RelabelType))
var = (Var*) ((RelabelType *) var)->arg;

but this makes the code even more confusing, because 'var' suggests it's
a Var node, but it's RelabelType node.

Also, specialjoin_is_unique_join() may have the same problem, but I've
been unable to come up with an example.


regards

-- 
Tomas Vondrahttp://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] Precedence of standard comparison operators

[Tom Lane]

Simon Riggs  writes:
> On 20 February 2015 at 20:44, Tom Lane  wrote:
>> Well, assuming that we're satisfied with just having a way to warn
>> when the behavior changed (and not, in particular, a switch that can
>> select old or new behavior)

> I'm in favour of your proposed improvements, but I'm having a problem
> thinking about random application breakage that would result.

> Having a warn_if_screwed parameter that we disable by default won't
> help much because if you are affected you can't change that situation.
> There are too many applications to test all of them and not all
> applications can be edited, even if they were tested.

I find this argument to be unhelpful, because it could be made in exactly
the same words against any non-backwards-compatible change whatsoever.
Nonetheless, we do make non-backwards-compatible changes all the time.

> I think the way to do this is to have a pluggable parser, so users can
> choose 1) old parser, 2) new, better parser, 3) any other parser they
> fancy that they maintain to ensure application compatibility. We've
> got a pluggable executor and optimizer, so why not a parser too?
> Implementing that in the same way we have done for executor and
> optimizer is a 1 day patch, so easily achievable for 9.5.

I don't find that particularly attractive either.  It seems quite unlikely
to me that anyone would actually use such a hook; replacing the whole
parser would be essentially unmaintainable.  Nobody uses the hooks you
mention to replace the whole planner or whole executor --- there are
wrappers out there, which is a different thing altogether.  But you could
not undo the issue at hand here without at the very least a whole new
copy of gram.y, which would need to be updated constantly if you wanted
it to keep working across more than one release.

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] [RFC: bug fix?] Connection attempt block forever when the synchronous standby is not running

[Alvaro Herrera]

FWIW a fix for this has been posted to all active branches:

Author: Andres Freund 
Branch: master [fd6a3f3ad] 2015-02-26 12:50:07 +0100
Branch: REL9_4_STABLE [d72115112] 2015-02-26 12:50:07 +0100
Branch: REL9_3_STABLE [abce8dc7d] 2015-02-26 12:50:07 +0100
Branch: REL9_2_STABLE [d67076529] 2015-02-26 12:50:07 +0100
Branch: REL9_1_STABLE [5c8dabecd] 2015-02-26 12:50:08 +0100
Branch: REL9_0_STABLE [82e0d6eb5] 2015-02-26 12:50:08 +0100

Reconsider when to wait for WAL flushes/syncrep during commit.

Up to now RecordTransactionCommit() waited for WAL to be flushed (if
synchronous_commit != off) and to be synchronously replicated (if
enabled), even if a transaction did not have a xid assigned. The primary
reason for that is that sequence's nextval() did not assign a xid, but
are worthwhile to wait for on commit.

This can be problematic because sometimes read only transactions do
write WAL, e.g. HOT page prune records. That then could lead to read only
transactions having to wait during commit. Not something people expect
in a read only transaction.

This lead to such strange symptoms as backends being seemingly stuck
during connection establishment when all synchronous replicas are
down. Especially annoying when said stuck connection is the standby
trying to reconnect to allow syncrep again...

This behavior also is involved in a rather complicated <= 9.4 bug where
the transaction started by catchup interrupt processing waited for
syncrep using latches, but didn't get the wakeup because it was already
running inside the same overloaded signal handler. Fix the issue here
doesn't properly solve that issue, merely papers over the problems. In
9.5 catchup interrupts aren't processed out of signal handlers anymore.

To fix all this, make nextval() acquire a top level xid, and only wait for
transaction commit if a transaction both acquired a xid and emitted WAL
records.  If only a xid has been assigned we don't uselessly want to
wait just because of writes to temporary/unlogged tables; if only WAL
has been written we don't want to wait just because of HOT prunes.

The xid assignment in nextval() is unlikely to cause overhead in
real-world workloads. For one it only happens SEQ_LOG_VALS/32 values
anyway, for another only usage of nextval() without using the result in
an insert or similar is affected.

Discussion: 20150223165359.gf30...@awork2.anarazel.de,
369698E947874884A77849D8FE3680C2@maumau,
5CF4ABBA67674088B3941894E22A0D25@maumau

Per complaint from maumau and Thom Brown

Backpatch all the way back; 9.0 doesn't have syncrep, but it seems
better to be consistent behavior across all maintained branches.

-- 
Á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] mogrify and indent features for jsonb

[Thom Brown]

On 26 February 2015 at 15:09, Dmitry Dolgov <9erthali...@gmail.com> wrote:

> Hi, Thom.
>
> > Would this support deleting "type" and the value 'dd'
>
> With this patch you can delete them one by one:
>
>  select '{"a": 1, "b": 2, "c": {"type": "json", "stuff": "test"}, "d":
> ["aa","bb","cc","dd"]}'::jsonb - '{c, type}'::text[] - '{d, -1}'::text[];
>  ?column?
> ---
>  {"a": 1, "b": 2, "c": {"stuff": "test"}, "d": ["aa", "bb", "cc"]}
> (1 row)
>

Doesn't work for me:

# select '{"a": 1, "b": 2, "c": {"type": "json", "stuff": "test"}, "d":
["aa","bb","cc","dd"]}'::jsonb - '{c, type}'::text[] - '{d, -1}'::text[];
ERROR:  operator does not exist: jsonb - text[]
LINE 1: ...ff": "test"}, "d": ["aa","bb","cc","dd"]}'::jsonb - '{c, typ...
 ^
HINT:  No operator matches the given name and argument type(s). You might
need to add explicit type casts.


> > Is there a way to take the json:
> > '{"a": 1, "b": 2, "c": {"type": "json", "stuff": "test"}, "d":
> ["aa","bb","cc","dd"]}'
> > and add "ee" to "d" without replacing it?
>
> No, looks like there is no way to add a new element to array with help of
> this patch. I suppose this feature can be implemented easy enough inside
> the "jsonb_concat" function:
>
>  select '{"a": 1, "b": 2, "c": {"type": "json", "stuff": "test"}, "d":
> ["aa","bb","cc","dd"]}'::jsonb || '{"d": ["ee"]}'::jsonb
>
> but I'm not sure, that it will be the best way.
>

Yeah, I think that may be problematic.  I agree with Josh that there's
probably no sane mix of operators for this, as I would expect your example
to replace "d": ["aa","bb","cc","dd"] with "d": ["ee"] rather than append
to it.  Hmm... unless we used a + operator, but then I'm not sure what
should happen in the instance of '{"d": ["aa"]}' + '{"d": "bb"}'.

-- 
Thom

Re: [HACKERS] pgaudit - an auditing extension for PostgreSQL

[David Steele]

On 2/25/15 11:40 PM, Alvaro Herrera wrote:
> Fujii Masao wrote:
>> On Tue, Feb 24, 2015 at 1:29 AM, David Steele  wrote:
> 
>>> 1) Follow Oracle's "as session" option and only log each statement type
>>> against an object the first time it happens in a session.  This would
>>> greatly reduce logging, but might be too little detail.  It would
>>> increase the memory footprint of the module to add the tracking.
> 
> Doesn't this mean that a user could conceal malicious activity simply by
> doing a innocuous query that silences all further activity?

True enough, but I thought it might be useful as an option.  I tend to
lock down users pretty tightly so there's not much they can do without
somehow getting superuser access, at which point all bets are off anyway.

In the case where users are highly constrained, the idea here would be
to just keeps tabs on the sort of things they are reading/writing for
financial audits and ISO compliance.

>>> 2) Only log once per call to the backend.  Essentially, we would only
>>> log the statement you see in pg_stat_activity.  This could be a good
>>> option because it logs what the user accesses directly, rather than
>>> functions, views, etc. which hopefully are already going through a
>>> review process and can be audited that way.
> 
> ... assuming the user does not create stuff on the fly behind your back.

Sure, but then the thing they created/modified would also be logged
somewhere earlier (assuming pg_audit.log = 'all').  It would require
some analysis to figure out what they did but I think that would be true
in many cases.

>>> Would either of those address your concerns?
>>
>> Before discussing how to implement, probably we need to consider the
>> spec about this. For example, should we log even nested statements for
>> the audit purpose? If yes, how should we treat the case where
>> the user changes the setting so that only DDL is logged, and then
>> the user-defined function which internally executes DDL is called?
>> Since the top-level SQL (calling the function) is not the target of
>> audit, we should not log even the nested DDL?
> 
> Clearly if you log only DROP TABLE, and then the malicious user hides
> one such call inside a function that executes the drop (which is called
> via a SELECT top-level SQL), you're not going to be happy.

If the purpose of the auditing it to catch malicious activity then all
statements would need to be logged.  If only top-level statements are
logged then it might be harder to detect, but it would still be logged.

-- 
- David Steele
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] mogrify and indent features for jsonb

[Dmitry Dolgov]

Hi, Thom.

> Would this support deleting "type" and the value 'dd'

With this patch you can delete them one by one:

 select '{"a": 1, "b": 2, "c": {"type": "json", "stuff": "test"}, "d":
["aa","bb","cc","dd"]}'::jsonb - '{c, type}'::text[] - '{d, -1}'::text[];
 ?column?
---
 {"a": 1, "b": 2, "c": {"stuff": "test"}, "d": ["aa", "bb", "cc"]}
(1 row)


> Is there a way to take the json:
> '{"a": 1, "b": 2, "c": {"type": "json", "stuff": "test"}, "d":
["aa","bb","cc","dd"]}'
> and add "ee" to "d" without replacing it?

No, looks like there is no way to add a new element to array with help of
this patch. I suppose this feature can be implemented easy enough inside
the "jsonb_concat" function:

 select '{"a": 1, "b": 2, "c": {"type": "json", "stuff": "test"}, "d":
["aa","bb","cc","dd"]}'::jsonb || '{"d": ["ee"]}'::jsonb

but I'm not sure, that it will be the best way.


On 26 February 2015 at 01:13, Josh Berkus  wrote:

> On 02/25/2015 03:13 AM, Thom Brown wrote:
> > Can you think of a reasonable syntax for doing that via operators?  I
> > can imagine that as a json_path function, i.e.:
> >
> > jsonb_add_to_path(jsonb, text[], jsonb)
> >
> > or where the end of the path is an array:
> >
> > jsonb_add_to_path(jsonb, text[], text|int|float|bool)
> >
> > But I simply can't imagine an operator syntax which would make it
> clear
> > what the user intended.
> >
> >
> > No, there probably isn't a sane operator syntax for such an operation.
> > A function would be nice.  I'd just want to avoid hacking away at arrays
> > by exploding them, adding a value then re-arraying them and replacing
> > the value.
>
> Well, anyway, that doesn't seem like a reason to block the patch.
> Rather, it's a reason to create another one for 9.6 ...
>
>
> --
> Josh Berkus
> PostgreSQL Experts Inc.
> http://pgexperts.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] pgaudit - an auditing extension for PostgreSQL

[David Steele]

On 2/25/15 10:42 PM, Fujii Masao wrote:
> On Tue, Feb 24, 2015 at 1:29 AM, David Steele  wrote:
>> On 2/18/15 10:25 AM, David Steele wrote:
>>> On 2/18/15 6:11 AM, Fujii Masao wrote:
 The pg_audit doesn't log BIND parameter values when prepared statement is 
 used.
 Seems this is an oversight of the patch. Or is this intentional?
>>>
>>> It's actually intentional - following the model I talked about in my
>>> earlier emails, the idea is to log statements only.  This also follows
>>> on 2ndQuadrant's implementation.
>>
>> Unfortunately, I think it's beyond the scope of this module to log bind
>> variables.
> 
> Maybe I can live with that at least in the first version.
> 
>> I'm following not only 2ndQuadrant's implementation, but
>> Oracle's as well.
> 
> Oracle's audit_trail (e.g., = db, extended) can log even bind values.
> Also log_statement=on in PostgreSQL also can log bind values.
> Maybe we can reuse the same technique that log_statement uses.

I'll look at how this is done in the logging code and see if it can be
used in pg_audit.

 Imagine the case where you call the user-defined function which executes
 many nested statements. In this case, pg_audit logs only top-level 
 statement
 (i.e., issued directly by client) every time nested statement is executed.
 In fact, one call of such UDF can cause lots of *same* log messages. I 
 think
 this is problematic.
>>>
>>> I agree - not sure how to go about addressing it, though.  I've tried to
>>> cut down on the verbosity of the logging in general, but of course it
>>> can still be a problem.
>>>
>>> Using security definer and a different logging GUC for the defining role
>>> might work.  I'll add that to my unit tests and see what happens.
>>
>> That didn't work - but I didn't really expect it to.
>>
>> Here are two options I thought of:
>>
>> 1) Follow Oracle's "as session" option and only log each statement type
>> against an object the first time it happens in a session.  This would
>> greatly reduce logging, but might be too little detail.  It would
>> increase the memory footprint of the module to add the tracking.
>>
>> 2) Only log once per call to the backend.  Essentially, we would only
>> log the statement you see in pg_stat_activity.  This could be a good
>> option because it logs what the user accesses directly, rather than
>> functions, views, etc. which hopefully are already going through a
>> review process and can be audited that way.
>>
>> Would either of those address your concerns?
> 
> Before discussing how to implement, probably we need to consider the
> spec about this. For example, should we log even nested statements for
> the audit purpose? If yes, how should we treat the case where
> the user changes the setting so that only DDL is logged, and then
> the user-defined function which internally executes DDL is called?
> Since the top-level SQL (calling the function) is not the target of
> audit, we should not log even the nested DDL?

I think logging nested statements should at least be an option.  And
yes, I think that nested statements should be logged even if the
top-level SQL is not (depending on configuration). The main case for
this would be DO blocks which can be run by anybody.

-- 
- David Steele
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Primary not sending to synchronous standby

[Thom Brown]

On 26 February 2015 at 13:08, Andres Freund  wrote:

> On 2015-02-23 17:09:24 +, Thom Brown wrote:
> > On 23 February 2015 at 16:53, Andres Freund 
> wrote:
> > > Comments? This is obviously just a POC, but I think something like this
> > > does make a great deal of sense.
> > >
> > > Thom, does that help?
>
> > Yeah, this appears to eliminate the problem, at least in the case I
> > reported.
>
> I've pushed a somewhat more evolved version of this after more testing.
>

Thanks.  I'll give it another round of testing later.

-- 
Thom

Re: [HACKERS] Primary not sending to synchronous standby

[Andres Freund]

On 2015-02-23 17:09:24 +, Thom Brown wrote:
> On 23 February 2015 at 16:53, Andres Freund  wrote:
> > Comments? This is obviously just a POC, but I think something like this
> > does make a great deal of sense.
> >
> > Thom, does that help?

> Yeah, this appears to eliminate the problem, at least in the case I
> reported.

I've pushed a somewhat more evolved version of this after more testing.

Greetings,

Andres Freund

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


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

Re: [HACKERS] New CF app deployment

[Michael Paquier]

On Thu, Feb 26, 2015 at 9:15 PM, Asif Naeem  wrote:
> This thread seems relevant, Please guide me to how can access older CF pages
>> The MSVC portion of this fix got completely lost in the void:
>> https://commitfest.postgresql.org/action/patch_view?id=1330
>
> Above link result in the following error i.e.
>
>> Not found
>> The specified URL was not found.
>
> Please do let me know if I missed something. Thanks.

Try commitfest-old instead, that is where the past CF app stores its
data, like that:
https://commitfest-old.postgresql.org/action/patch_view?id=1330
-- 
Michael


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

Re: [HACKERS] How to create virtual indexes on postgres

[Sreerama Manoj]

But, it runs with  Postgres 9.1 version...But I use 9.4..I think I cant use
that. Or as an alternative Is there any provision in postgres to know
use(Increase in Performance) of an index before creating that index.

On Thu, Feb 26, 2015 at 5:37 PM, Michael Paquier 
wrote:

> On Thu, Feb 26, 2015 at 6:20 PM, Sreerama Manoj
>  wrote:
> > So my question was can we know whether the planner  will use the index
> > before actually creating a real Index..or can we create "virtual" or
> > "Hypothetical" Index those can only be known to the planner and not the
> user
> > or Is there any alternative to do it..If present,share with me.
>
> No, the index needs to be created to allow the planner to use it. This
> reminds me of this project though, that if I recall correctly has
> patches Postgres to allow the use of hypothetical indexes:
> https://sourceforge.net/projects/hypotheticalind/
> --
> Michael
>

Re: [HACKERS] New CF app deployment

[Asif Naeem]

Hi,

This thread seems relevant, Please guide me to how can access older CF
pages e.g.

Thread
http://www.postgresql.org/message-id/flat/51f19059.7050...@pgexperts.com#51f19059.7050...@pgexperts.com
mentions
following link i.e.

The MSVC portion of this fix got completely lost in the void:
> https://commitfest.postgresql.org/action/patch_view?id=1330


Above link result in the following error i.e.

Not found
> The specified URL was not found.
>

Please do let me know if I missed something. Thanks.

Regards,
Muhammad Asif Naeem

On Tue, Feb 24, 2015 at 11:59 PM, Peter Eisentraut  wrote:

> On 2/22/15 3:12 PM, Magnus Hagander wrote:
> > Would you suggest removing the automated system completely, or keep it
> > around and just make it possible to override it (either by removing the
> > note that something is a patch, or by making something that's not listed
> > as a patch become marked as such)?
>
> I would remove it completely.  It has been demonstrated to not work.
>
>
>
> --
> 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] How to create virtual indexes on postgres

[Michael Paquier]

On Thu, Feb 26, 2015 at 6:20 PM, Sreerama Manoj
 wrote:
> So my question was can we know whether the planner  will use the index
> before actually creating a real Index..or can we create "virtual" or
> "Hypothetical" Index those can only be known to the planner and not the user
> or Is there any alternative to do it..If present,share with me.

No, the index needs to be created to allow the planner to use it. This
reminds me of this project though, that if I recall correctly has
patches Postgres to allow the use of hypothetical indexes:
https://sourceforge.net/projects/hypotheticalind/
-- 
Michael


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

Re: [HACKERS] Bug in pg_dump

[Michael Paquier]

On Wed, Feb 25, 2015 at 7:46 AM, Gilles Darold  wrote:
> This is a far better patch and the test to export/import of the
> postgis_topology extension works great for me.
>
> Thanks for the work.

Attached is a patch that uses an even better approach by querying only
once all the FK dependencies of tables in extensions whose data is
registered as dumpable by getExtensionMembership(). Could you check
that it works correctly? My test case passes but an extra check would
be a good nice. The patch footprint is pretty low so we may be able to
backport this patch easily.
-- 
Michael
From 72e369ee725301003cf065b0bf9875724455dac1 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 17 Feb 2015 07:39:23 +
Subject: [PATCH 1/3] Fix ordering of tables part of extensions linked with
 constraints

Additional checks on FK constraints potentially linking between them
extension objects are done and data dump ordering is ensured. Note
that this does not take into account foreign keys of tables that are
not part of an extension linking to an extension table.
---
 src/bin/pg_dump/pg_dump.c | 80 +--
 1 file changed, 78 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 2b53c72..bbcd600 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -15236,7 +15236,8 @@ dumpRule(Archive *fout, DumpOptions *dopt, RuleInfo *rinfo)
 }
 
 /*
- * getExtensionMembership --- obtain extension membership data
+ * getExtensionMembership --- obtain extension membership data and check FK
+ * dependencies among extension tables.
  */
 void
 getExtensionMembership(Archive *fout, DumpOptions *dopt, ExtensionInfo extinfo[],
@@ -15368,7 +15369,9 @@ getExtensionMembership(Archive *fout, DumpOptions *dopt, ExtensionInfo extinfo[]
 		  parsePGArray(extcondition, &extconditionarray, &nconditionitems) &&
 			nconfigitems == nconditionitems)
 		{
-			int			j;
+			int			j, i_conrelid, i_confrelid;
+			PQExpBuffer query2;
+			bool		first_elt = true;
 
 			for (j = 0; j < nconfigitems; j++)
 			{
@@ -15423,6 +15426,79 @@ getExtensionMembership(Archive *fout, DumpOptions *dopt, ExtensionInfo extinfo[]
 	}
 }
 			}
+
+			/*
+			 * Now that all the TableInfoData objects have been created for
+			 * all the extensions, check their FK dependencies and register
+			 * them to ensure correct data ordering.  Note that this is not
+			 * a problem for user tables not included in an extension
+			 * referencing with a FK tables in extensions as their constraint
+			 * is declared after dumping their data. In --data-only mode the
+			 * table ordering is ensured as well thanks to
+			 * getTableDataFKConstraints().
+			 */
+			query2 = createPQExpBuffer();
+
+			/*
+			 * Query all the foreign key dependencies for all the extension
+			 * tables found previously. Only tables whose data need to be
+			 * have to be tracked.
+			 */
+			appendPQExpBuffer(query2,
+	  "SELECT conrelid, confrelid "
+	  "FROM pg_catalog.pg_constraint "
+	  "WHERE contype = 'f' AND conrelid IN (");
+
+			for (j = 0; j < nconfigitems; j++)
+			{
+TableInfo  *configtbl;
+Oid			configtbloid = atooid(extconfigarray[j]);
+
+configtbl = findTableByOid(configtbloid);
+if (configtbl == NULL || configtbl->dataObj == NULL)
+	continue;
+
+if (first_elt)
+{
+	appendPQExpBuffer(query2, "%u", configtbloid);
+	first_elt = false;
+}
+else
+	appendPQExpBuffer(query2, ", %u", configtbloid);
+			}
+			appendPQExpBuffer(query2, ");");
+
+			res = ExecuteSqlQuery(fout, query2->data, PGRES_TUPLES_OK);
+			ntups = PQntuples(res);
+
+			i_conrelid = PQfnumber(res, "conrelid");
+			i_confrelid = PQfnumber(res, "confrelid");
+
+			/* Now get the dependencies and register them */
+			for (j = 0; j < ntups; j++)
+			{
+Oid			conrelid, confrelid;
+TableInfo  *reftable, *contable;
+
+conrelid = atooid(PQgetvalue(res, j, i_conrelid));
+confrelid = atooid(PQgetvalue(res, j, i_confrelid));
+contable = findTableByOid(conrelid);
+reftable = findTableByOid(confrelid);
+
+if (reftable == NULL ||
+	reftable->dataObj == NULL ||
+	contable == NULL ||
+	contable->dataObj == NULL)
+	continue;
+
+/*
+ * Make referencing TABLE_DATA object depend on the
+ * referenced table's TABLE_DATA object.
+ */
+addObjectDependency(&contable->dataObj->dobj,
+	reftable->dataObj->dobj.dumpId);
+			}
+			resetPQExpBuffer(query2);
 		}
 		if (extconfigarray)
 			free(extconfigarray);
-- 
2.3.0

From 1d8fe1c39ec5049c39810f94153d347971738616 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 17 Feb 2015 22:29:28 +0900
Subject: [PATCH 2/3] Make prove_check install contents of current directory as
 well

This is useful for example for TAP tests in extensions.
---
 src/Makefile.global.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 7

[HACKERS] BDR Multiple database

[Jirayut Nimsaeng]

Hi all,

I'm using PostgreSQL BDR 9.4.1 to test BDR capability right now

$ psql --version
psql (PostgreSQL) 9.4.1

We want to use BDR with multiple database but now all the document didn't
show any example how to config BDR with multiple database. We've tried with
many combination as below but still no luck. Anyone can point us this?

1st combination

bdr.connections = 'bdrnode02db1'
bdr.bdrnode02db1_dsn = 'dbname=db1 host=172.17.42.1 port=49264
user=postgres'
bdr.connections = 'bdrnode02db2'
bdr.bdrnode02db2_dsn = 'dbname=db2 host=172.17.42.1 port=49264
user=postgres'


2nd combination

bdr.connections = 'bdrnode02'
bdr.bdrnode02_dsn = 'dbname=db1 host=172.17.42.1 port=49264 user=postgres'
bdr.bdrnode02_dsn = 'dbname=db2 host=172.17.42.1 port=49264 user=postgres'


Regards,
Jirayut

Re: [HACKERS] How about to have relnamespace and relrole?

[Kyotaro HORIGUCHI]

Sorry, I fixed a silly typo in documentation in the previous version.

- of theses types has a significance... 
+ of these types has a significance...

# My fingers frequently slip as above..

I incremented the version of this revised patch to get rid of
confusion.

===
Hello, thank you for reviewing.

The attatched are the fourth version of this patch.

0001-Add-regrole_v4.patch
0002-Add-regnamespace_v4.patch

- Rearranged into regrole patch and regnamespace patch as seen
  above, each of them consists of changes for code, docs,
  regtests. regnamespace patch depends on the another patch.

- Removed the irrelevant change and corrected mistakes in
  comments.

- Renamed role_or_oid to role_name_or_oid in regrolein().

- Changed the example name for regrole in the docmentation to
  'smithee' as an impossible-in-reality-but-well-known name, from
  'john', the most common in US (according to Wikipedia).



At Tue, 24 Feb 2015 16:30:44 +0530, Jeevan Chalke 
 wrote in 

> I agree on Tom's concern on MVCC issue, but we already hit that when we
> introduced regclass and others. So I see no additional issue with these
> changes as such. About planner slowness, I guess updated documentation looks
> perfect for that.
> 
> So I went ahead reviewing these patches.
> 
> All patches are straight forward and since we have similar code already
> exists, I did not find any issue at code level. They are consistent with
> other
> functions.

One concern is about arbitrary allocation of OIDs for the new
objects - types, functions. They are isolated from other similar
reg* types, but there's no help for it without global renumbering
of static(system) OIDs.

> Patches applies with patch -p1. make, make install, make check has
> no issues. Testing was fine too.
> 
> However here are few review comments on the patches attached:
> 
> Review points on 0001-Add-regrole.patch
> ---
> 1.
> +#include "utils/acl.h"
> 
> Can you please add it at appropriate place as #include list is an ordered
> list

regrolein calls reg_role_oid in acl.c, which is declared in acl.h.

> 2.
> +char   *role_or_oid = PG_GETARG_CSTRING(0);
> 
> Can we have variable named as role_name_or_oid, like other similar
> functions?

I might somehow have thought it a bit long. Fixed.

> 3.
> +/*
> + * Normal case: parse the name into components and see if it matches
> any
> + * pg_role entries in the current search path.
> + */
> 
> I believe, roles are never searched per search path. Thus need to update
> comments here.

Ouch. I forgot to edit it properly. I edit it. The correct
catalog name is pg_authid.

+   /* Normal case: see if the name matches any pg_authid entry. */

I also edited comments for regnamespacein.


> Review points on 0002-Add-regnamespace.patch
> ---
> 1.
> + * regnamespacein- converts "classname" to class OID
> 
> Typos. Should be nspname instead of classname and namespase OID instead of
> class OID

Thank you for pointing out. I fixed the same mistake in
regrolein, p_ts_dict was there instead of pg_authid..  The names
of oid kinds appear in multiple forms, that is, oprname and
opr_name. Although I don't understand the reason, I followed the
convention.

> Review points on 0003-Check-newpatch
> ---
> 1.
> @@ -1860,6 +1860,7 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,
>  OidattrdefOid;
>  ObjectAddress colobject,
>  defobject;
> +Oidexprtype;
> 
> This seems unrelated. Please remove.

It's a trace of the previous code to ruling out the new oid
types. Removed.

> Apart from this, it will be good if you have ONLY two patches,
> (1) For regrole and (2) For regnamespace specific
> With all related changes into one patch. I mean, all role related changes
> i.e.
> 0001 + 0003 role related changes + 0004 role related changes and docs update
> AND
> 0002 + 0003 nsp related changes + 0004 nsp related changes

I prudently separated it since I wasn't confident on the
pertinence of ruling out. I rearranged them as your advise
including docs.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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

Re: [HACKERS] How about to have relnamespace and relrole?

[Kyotaro HORIGUCHI]

Hello, thank you for reviewing.

The attatched are the third version of this patch.

0001-Add-regrole_v3.patch
0002-Add-regnamespace_v3.patch

- Rearranged into regrole patch and regnamespace patch as seen
  above, each of them consists of changes for code, docs,
  regtests. regnamespace patch depends on the another patch.

- Removed the irrelevant change and corrected mistakes in
  comments.

- Renamed role_or_oid to role_name_or_oid in regrolein().

- Changed the example name for regrole in the docmentation to
  'smithee' as an impossible-in-reality-but-well-known name, from
  'john', the most common in US (according to Wikipedia).



At Tue, 24 Feb 2015 16:30:44 +0530, Jeevan Chalke 
 wrote in 

> I agree on Tom's concern on MVCC issue, but we already hit that when we
> introduced regclass and others. So I see no additional issue with these
> changes as such. About planner slowness, I guess updated documentation looks
> perfect for that.
> 
> So I went ahead reviewing these patches.
> 
> All patches are straight forward and since we have similar code already
> exists, I did not find any issue at code level. They are consistent with
> other
> functions.

One concern is about arbitrary allocation of OIDs for the new
objects - types, functions. They are isolated from other similar
reg* types, but there's no help for it without global renumbering
of static(system) OIDs.

> Patches applies with patch -p1. make, make install, make check has
> no issues. Testing was fine too.
> 
> However here are few review comments on the patches attached:
> 
> Review points on 0001-Add-regrole.patch
> ---
> 1.
> +#include "utils/acl.h"
> 
> Can you please add it at appropriate place as #include list is an ordered
> list

regrolein calls reg_role_oid in acl.c, which is declared in acl.h.

> 2.
> +char   *role_or_oid = PG_GETARG_CSTRING(0);
> 
> Can we have variable named as role_name_or_oid, like other similar
> functions?

I might somehow have thought it a bit long. Fixed.

> 3.
> +/*
> + * Normal case: parse the name into components and see if it matches
> any
> + * pg_role entries in the current search path.
> + */
> 
> I believe, roles are never searched per search path. Thus need to update
> comments here.

Ouch. I forgot to edit it properly. I edit it. The correct
catalog name is pg_authid.

+   /* Normal case: see if the name matches any pg_authid entry. */

I also edited comments for regnamespacein.


> Review points on 0002-Add-regnamespace.patch
> ---
> 1.
> + * regnamespacein- converts "classname" to class OID
> 
> Typos. Should be nspname instead of classname and namespase OID instead of
> class OID

Thank you for pointing out. I fixed the same mistake in
regrolein, p_ts_dict was there instead of pg_authid..  The names
of oid kinds appear in multiple forms, that is, oprname and
opr_name. Although I don't understand the reason, I followed the
convention.

> Review points on 0003-Check-newpatch
> ---
> 1.
> @@ -1860,6 +1860,7 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,
>  OidattrdefOid;
>  ObjectAddress colobject,
>  defobject;
> +Oidexprtype;
> 
> This seems unrelated. Please remove.

It's a trace of the previous code to ruling out the new oid
types. Removed.

> Apart from this, it will be good if you have ONLY two patches,
> (1) For regrole and (2) For regnamespace specific
> With all related changes into one patch. I mean, all role related changes
> i.e.
> 0001 + 0003 role related changes + 0004 role related changes and docs update
> AND
> 0002 + 0003 nsp related changes + 0004 nsp related changes

I prudently separated it since I wasn't confident on the
pertinence of ruling out. I rearranged them as your advise
including docs.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From 4d56a68e2bf2b7ee0da0447ad9f82f0b46277133 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Wed, 18 Feb 2015 14:38:32 +0900
Subject: [PATCH 1/2] Add regrole

Add new OID aliass type regrole. The new type has the scope of whole
the database cluster so it doesn't behave as the same as the existing
OID alias types which have database scope, concerning object
dependency. To get rid of confusion, inhibit constants of the new type
from appearing where dependencies are made involving it.

Documentation about this new type is added and some existing
descriptions are modified according to the restriction of the
type. Addition to it, put a note about possible MVCC violation and
optimization issues, which are general over the all reg* types.
---
 doc/src/sgml/datatype.sgml| 55 +---
 src/backend/bootstrap/bootstrap.c |  2 +
 src/backend/catalog/dependency.c  | 22 
 src/backend/utils/adt/regproc.c   | 98 +++
 src/backend/utils/adt/selfuncs.c  |  2 +
 src/backend/utils/cache/catcache.c|  1 +
 src/include/catalog/pg_cast.h |  7 +++
 sr

Re: [HACKERS] Refactoring GUC unit conversions

[Fujii Masao]

On Fri, Feb 13, 2015 at 10:26 PM, Heikki Linnakangas
 wrote:
> In the "redesign checkpoint_segments" patch, Robert suggested keeping the
> settings' base unit as "number of segments", but allow conversions from MB,
> GB etc. I started looking into that and found that adding a new unit to
> guc.c is quite messy. The conversions are done with complicated
> if-switch-case constructs.
>
> Attached is a patch to refactor that, making the conversions table-driven.
> This doesn't change functionality, it just makes the code nicer.

Isn't it good idea to allow even wal_keep_segments to converse from MB, GB etc?

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] Review of GetUserId() Usage

[Jeevan Chalke]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:tested, passed

I have reviewed the patch.
Patch is excellent in shape and does what is expected and discussed.
Also changes are straight forward too.

So looks good to go in.

However I have one question:

What is the motive for splitting the function return value from
SIGNAL_BACKEND_NOPERMISSION into
SIGNAL_BACKEND_NOSUPERUSER and SIGNAL_BACKEND_NOPERMISSION?

Is that required for some other upcoming patches OR just for simplicity?

Currently, we have combined error for both which is simply split into two.
No issue as such, just curious as it does not go well with the subject.

You can mark this for ready for committer.

The new status of this patch is: Waiting on Author


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