Re: [HACKERS] Audit of logout

[Amit Kapila]

On Tue, Aug 5, 2014 at 8:04 PM, Fujii Masao  wrote:
>
> Yep, the attached patch introduces PGC_SU_BACKEND and
> changes the contexts of log_connections and log_disconnections
> to PGC_SU_BACKEND. Review?
>

1.
! else if (context != PGC_POSTMASTER && context != PGC_SU_BACKEND &&
!  context != PGC_SU_BACKEND && source != PGC_S_CLIENT)

In the above check for PGC_SU_BACKEND is repeated, here
one of the check should be PGC_SU_BACKEND  and other
should be PGC_BACKEND.

2.
+ case PGC_SU_BACKEND:
+ if (context == PGC_BACKEND)
+ {
..
..
+ return 0;
+ }
  case PGC_BACKEND:
  if (context == PGC_SIGHUP)

Changing PGC_SU_BACKEND parameter (log_connections) is
visible even with a non-super user client due to above code.
Shouldn't it be only visible for super-user logins?

Simple steps to reproduce the problem:
a. start Server (default configuration)
b. connect with superuser
c. change in log_connections to on in postgresql.conf
d. perform select pg_reload_conf();
e. connect with non-super-user
f.  show log_connections;  --This step shows the value as on,
   --whereas I think it should have
been off

This test has been performed on *Windows*.


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

Re: [HACKERS] Support for N synchronous standby servers

[Michael Paquier]

On Fri, Aug 22, 2014 at 11:42 PM, Michael Paquier
 wrote:
>>
>> 2. Logic of deciding the highest priority one seems to be in-correct.
>> Assume, s_s_num = 3, s_s_names = 3,4,2,1
>> standby nodes are in order as: 1,2,3,4,5,6,7
>>
>> As per the logic in patch, node 4 with priority 2 will not be added 
>> in the list whereas 1,2,3 will be added.
>>
>> The problem is because priority updated for next tracking is not the 
>> highest priority as of that iteration, it is just priority of last node 
>> added to the list. So it may happen that a node with   higher priority 
>> is still there in list but we are comparing with some other smaller priority.
>
>
> Fixed. Nice catch!


Actually by re-reading the code I wrote yesterday I found that the fix
in v6 for that is not correct. That's really fixed with v7 attached.
Regards,
-- 
Michael
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index f23e5dc..d085f48 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -2586,12 +2586,13 @@ include_dir 'conf.d'
 Specifies a comma-separated list of standby names that can support
 synchronous replication, as described in
 .
-At any one time there will be at most one active synchronous standby;
-transactions waiting for commit will be allowed to proceed after
-this standby server confirms receipt of their data.
-The synchronous standby will be the first standby named in this list
-that is both currently connected and streaming data in real-time
-(as shown by a state of streaming in the
+At any one time there will be at a number of active synchronous standbys
+defined by , transactions
+waiting for commit will be allowed to proceed after those standby
+servers confirm receipt of their data. The synchronous standbys will be
+the first entries named in this list that are both currently connected
+and streaming data in real-time (as shown by a state of
+streaming in the
 
 pg_stat_replication view).
 Other standby servers appearing later in this list represent potential
@@ -2627,6 +2628,58 @@ include_dir 'conf.d'
   
  
 
+ 
+  synchronous_standby_num (integer)
+  
+   synchronous_standby_num configuration parameter
+  
+  
+  
+   
+Specifies the number of standbys that support
+synchronous replication.
+   
+   
+Default value is -1. In this case, if
+ is empty all the
+standby nodes are considered asynchronous. If there is at least
+one node name defined, process will wait for one synchronous
+standby listed.
+   
+   
+When this parameter is set to 0, all the standby
+nodes will be considered as asynchronous.
+   
+   
+   This parameter value cannot be higher than
+.
+   
+   
+Are considered as synchronous the first elements of
+ in number of
+ that are
+connected. If there are more elements than the number of stansbys
+required, all the additional standbys are potential synchronous
+candidates. If  is
+empty, all the standbys are asynchronous. If it is set to the
+special entry *, a number of standbys equal to
+ with the highest
+pritority are elected as being synchronous.
+   
+   
+Server will wait for commit confirmation from
+ standbys, meaning that
+if  has less elements
+than the number of standbys required, server will wait indefinitely
+for a commit confirmation.
+   
+   
+This parameter can only be set in the postgresql.conf
+file or on the server command line.
+   
+  
+ 
+
  
   vacuum_defer_cleanup_age (integer)
   
diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml
index d249959..ec0ea70 100644
--- a/doc/src/sgml/high-availability.sgml
+++ b/doc/src/sgml/high-availability.sgml
@@ -1081,12 +1081,12 @@ primary_slot_name = 'node_a_slot'
 WAL record is then sent to the standby. The standby sends reply
 messages each time a new batch of WAL data is written to disk, unless
 wal_receiver_status_interval is set to zero on the standby.
-If the standby is the first matching standby, as specified in
-synchronous_standby_names on the primary, the reply
-messages from that standby will be used to wake users waiting for
-confirmation that the commit record has been received. These parameters
-allow the administrator to specify which standby servers should be
-synchronous standbys. Note that the configuration of synchronous
+If the standby is the first synchronous_standby_num matching
+standbys, as specified in synchronous_standby_names on the
+primary, the reply messages from that standby will be u

Re: [HACKERS] Re: proposal: ignore null fields in not relation type composite type based constructors

[Pavel Stehule]

Hello


2014-08-22 12:21 GMT+02:00 Jeevan Chalke :

> Hi Pavel,
>
> You have said that XMLFOREST has something which ignores nulls, what's
> that?
> Will you please provide an example ?
>

I was partially wrong - XMLFOREST ignore null always

postgres=# select xmlforest(10 as a,20 as b,null as c);
 xmlforest

 1020
(1 row)

so if you would to empty elements, you should to use xmlelement and
xmlconcat

postgres=# select xmlconcat(xmlforest(10 as a,20 as b), xmlelement(name c,
null));
   xmlconcat

 1020
(1 row)



>
> I am NOT sure, but here you are trying to omit entire field from the output
> when its value is NULL. But that will add an extra efforts at other end
> which is using output of this. That application need to know all fields and
> then need to replace NULL values for missing fields. However we have an
> optional argument for ignoring nulls and thus we are safe. Application will
> use as per its choice.
>

with my patch, you can do decision - lot of REST services doesn't
distinguishes between empty and missing tag - and some developers prefer
remove empty tags due less size of message.


>
> Well, apart from that, tried reviewing the patch. Patch was applied but
> make
> failed with following error.
>
> make[3]: Entering directory `/home/jeevan/pg_master/src/backend/catalog'
> cd ../../../src/include/catalog && '/usr/bin/perl' ./duplicate_oids
> 3255
> make[3]: *** [postgres.bki] Error 1
>
> Please run unused_oids script to find unused oid.
>

it needs remastering

update in attachemnt



>
> However, I had a quick look over code changes. At first glance it looks
> good. But still need to check on SQL level and then code walk-through.
>
> Waiting for updated patch.
>

thank you for review

Regards

Pavel


>
> Thanks
>
> --
> Jeevan B Chalke
> Principal Software Engineer, Product Development
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>
>
commit e1dd47ca1f881a2ce41117526a536555c23c2d81
Author: Pavel Stehule 
Date:   Sat Aug 23 07:19:46 2014 +0200

remastering, change used oid in pg_proc

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index c715ca2..a27aff4 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -10294,11 +10294,12 @@ table2-mapping
   
   

- row_to_json(record [, pretty_bool])
+ row_to_json(record [, pretty_bool [, ignore_nulls] ])


  Returns the row as a JSON object. Line feeds will be added between
- level-1 elements if pretty_bool is true.
+ level-1 elements if pretty_bool is true. Ignore
+ NULL when ignore_nulls is true.

row_to_json(row(1,'foo'))
{"f1":1,"f2":"foo"}
diff --git a/src/backend/utils/adt/json.c b/src/backend/utils/adt/json.c
index 494a028..f35db04 100644
--- a/src/backend/utils/adt/json.c
+++ b/src/backend/utils/adt/json.c
@@ -79,7 +79,8 @@ static void report_invalid_token(JsonLexContext *lex);
 static int	report_json_context(JsonLexContext *lex);
 static char *extract_mb_char(char *s);
 static void composite_to_json(Datum composite, StringInfo result,
-  bool use_line_feeds);
+  bool use_line_feeds,
+  bool ignore_nulls);
 static void array_dim_to_json(StringInfo result, int dim, int ndims, int *dims,
   Datum *vals, bool *nulls, int *valcount,
   JsonTypeCategory tcategory, Oid outfuncoid,
@@ -1362,7 +1363,7 @@ datum_to_json(Datum val, bool is_null, StringInfo result,
 			array_to_json_internal(val, result, false);
 			break;
 		case JSONTYPE_COMPOSITE:
-			composite_to_json(val, result, false);
+			composite_to_json(val, result, false, false);
 			break;
 		case JSONTYPE_BOOL:
 			outputstr = DatumGetBool(val) ? "true" : "false";
@@ -1591,7 +1592,8 @@ array_to_json_internal(Datum array, StringInfo result, bool use_line_feeds)
  * Turn a composite / record into JSON.
  */
 static void
-composite_to_json(Datum composite, StringInfo result, bool use_line_feeds)
+composite_to_json(Datum composite, StringInfo result, bool use_line_feeds,
+bool ignore_nulls)
 {
 	HeapTupleHeader td;
 	Oid			tupType;
@@ -1630,6 +1632,12 @@ composite_to_json(Datum composite, StringInfo result, bool use_line_feeds)
 		if (tupdesc->attrs[i]->attisdropped)
 			continue;
 
+		val = heap_getattr(tuple, i + 1, tupdesc, &isnull);
+
+		/* Don't serialize NULL field when we don't want it */
+		if (isnull && ignore_nulls)
+			continue;
+
 		if (needsep)
 			appendStringInfoString(result, sep);
 		needsep = true;
@@ -1638,8 +1646,6 @@ composite_to_json(Datum composite, StringInfo result, bool use_line_feeds)
 		escape_json(result, attname);
 		appendStringInfoChar(result, ':');
 
-		val = heap_getattr(tuple, i + 1, tupdesc, &isnull);
-
 		if (isnull)
 		{
 			tcategory = JSONTYPE_NULL;
@@ -1731,7 +1737,7 @@ row_to_json(PG_FUNCTION_ARGS)
 
 	result = makeStringInfo();
 
-	composite_to_json(array, result, false);
+	composite_to_json(array, result, false, false)

Re: [HACKERS] proposal: rounding up time value less than its unit.

[Tomonari Katsumata]

Thank you for the comments.

It was a bug in my patch as another developer says.
I've not considered about the value 'zero', sorry.

I attached new patch.
This patch rounds up the value when only it's less than required unit.
Like below.
(unit: min)
0->0
0s->0
10s->1
70s->1

Although my original complaint is fixed, I'm worried about this change will
make users confusing.
Is it better to raise a message(ex. INFO) when a value less than required
unit is set?



2014-08-21 21:00 GMT+09:00 Heikki Linnakangas :

> On 07/10/2014 09:52 AM, Tomonari Katsumata wrote:
>
>> Hi,
>>
>> Several couple weeks ago, I posted a mail to pgsql-doc.
>> http://www.postgresql.org/message-id/53992ff8.2060...@po.ntts.co.jp
>>
>> In this thread, I concluded that it's better to
>> round up the value which is less than its unit.
>> Current behavior (rounding down) has undesirable setting risk,
>> because some GUCs have special meaning for 0.
>>
>> And then I made a patch for this.
>> Please check the attached patch.
>>
>
> The patch also rounds a zero up to one. A naked zero with no unit is not
> affected, but e.g if you have "log_rotation_age=0s", it will not disable
> the feature as you might expect, but set it to 1 minute. Should we do
> something about that?
>
> If we're going to explain the rounding up in the manual, we also need to
> explain the normal rule, which is to round down. How about this:
>
> --- a/doc/src/sgml/config.sgml
> +++ b/doc/src/sgml/config.sgml
> @@ -44,6 +44,15 @@
>   (seconds), min (minutes), h
>   (hours), and d (days).  Note that the multiplier
>   for memory units is 1024, not 1000.
> +
> +
> + If a memory or time setting is specified with more precision than the
> + implicit unit of the setting, it is rounded down.  However, if
> rounding
> + down would yield a zero, it is rounded up to one instead.  For
> example,
> + the implicit unit of log_rotation_age so if
> + you set it to 150s, it will be rounded down to two
> + minutes. However, if you set it to 10s, it will be
> + rounded up to one minute.
>  
>
> - Heikki
>
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>


time-unit-guc-round-up_v2.patch
Description: Binary data

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

Re: [HACKERS] Is this a bug?

[Bruce Momjian]

On Fri, Aug 22, 2014 at 03:12:47PM -0400, Robert Haas wrote:
> On Fri, Aug 22, 2014 at 2:33 PM, Bruce Momjian  wrote:
> >> Yes, you remember well.  I will have to find a different way for
> >> pg_upgrade to call a no-op ALTER TABLE, which is fine.
> >
> > Looking at the ALTER TABLE options, I am going to put this check in a
> > !IsBinaryUpgrade block so pg_upgrade can still use its trick.
> 
> -1, that's really ugly.
> 
> Maybe the right solution is to add a form of ALTER TABLE that is
> specifically defined to do only this check.  This is an ongoing need,
> so that might not be out of line.

Ah, seems ALTER TABLE ... DROP CONSTRAINT IF EXISTS also works --- I
will use that.

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

  + Everyone has their own god. +


-- 
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] [v9.5] Custom Plan API

[Kohei KaiGai]

2014-08-23 0:39 GMT+09:00 Robert Haas :
> On Thu, Jul 17, 2014 at 3:38 PM, Tom Lane  wrote:
>> Alvaro Herrera  writes:
>>> I haven't followed this at all, but I just skimmed over it and noticed
>>> the CustomPlanMarkPos thingy; apologies if this has been discussed
>>> before.  It seems a bit odd to me; why isn't it sufficient to have a
>>> boolean flag in regular CustomPlan to indicate that it supports
>>> mark/restore?
>>
>> Yeah, I thought that was pretty bogus too, but it's well down the
>> list of issues that were there last time I looked at this ...
>
> I think the threshold question for this incarnation of the patch is
> whether we're happy with new DDL (viz, CREATE CUSTOM PLAN PROVIDER) as
> a way of installing new plan providers into the database.  If we are,
> then I can go ahead and enumerate a long list of things that will need
> to be fixed to make that code acceptable (such as adding pg_dump
> support).  But if we're not, there's no point in spending any time on
> that part of the patch.
>
Even though I'm patch author, I'd like to agree with this approach.
In fact, the previous custom-plan interface I proposed to the v9.4
development cycle does not include DDL support to register the
custom-plan providers, however, it works fine.

One thing I was pointed out, it is the reason why I implemented
DDL support, is that intermediation of c-language function also
loads the extension module implicitly. It is an advantage, but
not sure whether it shall be supported from the beginning.

> I can see a couple of good reasons to think that this approach might
> be reasonable:
>
> - In some ways, a custom plan provider (really, at this point, a
> custom scan provider) is very similar to a foreign data wrapper.  To
> the guts of PostgreSQL, an FDW is a sort of black box that knows how
> to scan some data not managed by PostgreSQL.  A custom plan provider
> is similar, except that it scans data that *is* managed by PostgreSQL.
>
> - There's also some passing resemblance between a custom plan provider
> and an access method.  Access methods provide a set of APIs for fast
> access to data via an index, while custom plan providers provide an
> API for fast access to data via magic that the core system doesn't
> (and need not) understand.  While access methods don't have associated
> SQL syntax, they do include catalog structure, so perhaps this should
> too, by analogy.
>
> All that having been said, I'm having a hard time mustering up
> enthusiasm for this way of doing things.  As currently constituted,
> the pg_custom_plan_provider catalog contains only a name, a "class"
> that is always 's' for scan, and a handler function OID.  Quite
> frankly, that's a whole lot of nothing.  If we got rid of the
> pg_catalog structure and just had something like
> RegisterCustomPlanProvider(char *name, void (*)(customScanArg *),
> which could be invoked from _PG_init(), hundreds and hundreds of lines
> of code could go away and we wouldn't lose any actual functionality;
> you'd just list your custom plan providers in shared_preload_libraries
> or local_preload_libraries instead of listing them in a system
> catalog.  In fact, you might even have more functionality, because you
> could load providers into particular sessions rather than system-wide,
> which isn't possible with this design.
>
Indeed. It's an advantage of the approach without system catalog.


> I think the underlying issue here really has to do with when custom
> plan providers get invoked - what triggers that?  For foreign data
> wrappers, we have some relations that are plain tables (relkind = 'r')
> and no foreign data wrapper code is invoked.  We have others that are
> flagged as foreign tables (relkind = 'f') and for those we look up the
> matching FDW (via ftserver) and run the code.  Similarly, for an index
> AM, we notice that the relation is an index (relkind = 'r') and then
> consult relam to figure out which index AM we should invoke.  But as
> KaiGai is conceiving this feature, it's quite different.  Rather than
> applying only to particular relations, and being mutually exclusive
> with other options that might apply to those relations, it applies to
> *everything* in the database in addition to whatever other options may
> be present.  The included ctidscan implementation is just as good an
> example as PG-Strom: you inspect the query and see, based on the
> operators present, whether there's any hope of accelerating things.
> In other words, there's no user configuration - and also, not
> irrelevantly, no persistent on-disk state the way you have for an
> index, or even an FDW, which has on disk state to the extent that
> there have to be catalog entries tying a particular FDW to a
> particular table.
>
Yes, that's my point. In case of FDW or index AM, the query planner
can have some expectations how relevant executor node will handle
the given relation scan according to the persistent state.
However, custom-plan is a black-box to the query pla

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

[Tomas Vondra]

On 20.8.2014 08:11, Jeff Davis wrote:
> On Tue, 2014-08-19 at 12:54 +0200, Tomas Vondra wrote:
>
> It would be easier to resolve the performance concern if I could
> reliably get the results Robert is getting. I think I was able to
> reproduce the regression with the old patch, but the results were still
> noisy.

I created a small extension for this purpose, it's available here:

   https://github.com/tvondra/palloc_bench

In short, it creates a chain of memory contexts, and then repeats
palloc/pfree a given number of times (with a chosen request size).

It either calls AllocSetContextCreate or AllocSetContextCreateTracked,
depending on whether there's a

#define TRACKING_FLAG

so either leave it there or comment it out. The time is printed as a
warhing.

I did a number of tests to get an idea of the overhead, using this call

  select palloc_bench(10,1,32768);

which means 10 memory contexts, 1 palloc/free cycles with 32kB
requests.

And I got these results:

master
---
3246.03 ms
3125.24 ms
3247.86 ms
3251.85 ms
3113.03 ms
3140.35 ms

v3 patch (AllocSetContextCreate => no tracing)
3303.64 ms
3278.57 ms
3295.11 ms
3325.63 ms
3329.84 ms
3439.27 ms

-- v3 patch (AllocSetContextCreateTracked => tracing)
6296.43 ms
5228.83 ms
5271.43 ms
5158.60 ms
5404.57 ms
5240.40 ms

-- v4 (tracing all the time)
6728.84 ms
6478.88 ms
6478.82 ms
6473.57 ms
6554.96 ms
6528.66 ms

I think this makes the overhead clearly visible. I also worth mentioning
that this does nothing else except for palloc/free, which is not really
what a real workload does. And 1 palloc/free of 32kB blocks
means ~3TB of RAM, unless my math is broken.

So looking at the numbers and saying "7 seconds >> 3 seconds", all is
lost is not really appropriate IMHO.

Anyway, ISTM that v4 is actually a bitm ore expensive than v3 for some
reason. I'm not entirely sure why, but I suspect it's because of
updating the few additional memory contexts up to TopMemoryContext.
That's something v3 didn't do.

I tried to hack a bit on the idea of using a single byte for the flags
(isReset and track_mem) - patch attached. That got me pretty much to v3
performance (or maybe slightly better):

-- v4 + flag (tracing all the time)
5222.38 ms
4958.37 ms
5072.21 ms
5100.43 ms
5059.65 ms
4995.52 ms

But nothing that'd magically save the day ... and with disabled tracing
we get pretty much to v3 numbers (with trace_mem=false). So this
gymnastics gave us pretty much nothing ...

But I have realized that maybe the problem is that we're handling memory
contexts and accounting as 1:1. But that's not really the case - most of
the context is not really interested in this. They don't use accounting
now, and it won't change. So only small fraction of memory contexts will
ask for acounting. Yet all the contexts are forced to pass accounting
info from their children to their parent, if there happens to be a
context with track_mem=true somewhere above them.

And that's exactly the problem, because most of the time is spent in
update_accounting, in the loop over parents.

So my proposal is to separate those two things into two hierarchies,
that are somehow parallel, but not exactly.

That means:

(1) creating a structure with the accouting info

typedef struct MemoryAccountingData {

Size  total_allocated;
Size  self_allocated;

struct MemoryAccountingData * parent;

} MemoryAccountingData;

(2) adding a pointer to MemoryAccountingData to MemoryContextData, and
a 'track_mem' flag for contexts that requested tracking

typedef struct MemoryContextData
{
...
MemoryAccounting  accounting;
...
} MemoryContextData;

(3) when a context does not request accounting, it just uses the
accounting pointer from the parent context, and track_mem=false

(4) when the context requests accounting, it allocates it's own
accounting structure, sets accounting->parent to the accounting
from parent, and sets track_mem=true

Now all the contexts have a direct pointer to the accounting of the
nearest parent context that explicitly requested accounting, and don't
need to walk through all the parents.

Contexts that did not request tracking have track_mem=false, and their
accounting points to the parent with explicit accounting, or is NULL if
there's no such parent. For these contexts, GetAllocated always returns
0, but that's OK because they haven't requested accounting anyway.

Contexts that requested tracking have have track_mem=true, and have
their own specific accounting instance. The accounting->parent plays
pretty much the same role as 'accounting' with 'track_mem=false' (see
previous paragraph). These contexts return GetAllocated properly.

Now, I did a quick with the palloc_bench - 1 context with tracking
enabled, and a chain of 10 contexts without tracking (but updating the
accounting for the first context).

And I see this - with tracking enabled:

3235.57 ms
3240.09 ms
3225.47 ms
3306.95 ms
3249.1

Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED

[Fabrízio de Royes Mello]

On Fri, Aug 22, 2014 at 4:45 PM, Alvaro Herrera 
wrote:
>
> BTW why is it that index_build() checks the heap's relpersistence flag
> rather than the index'?
>

I'm curious about it too... the code in src/backend/catalog/index.c is:

1975 if (heapRelation->rd_rel->relpersistence ==
RELPERSISTENCE_UNLOGGED &&
1976 !smgrexists(indexRelation->rd_smgr, INIT_FORKNUM))
1977 {


Should not to be in that way?

1975 if (indexRelation->rd_rel->relpersistence ==
RELPERSISTENCE_UNLOGGED &&
1976 !smgrexists(indexRelation->rd_smgr, INIT_FORKNUM))
1977 {


Alvaro, is this your concern? Right?

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] WIP Patch for GROUPING SETS phase 1

[Andrew Gierth]

> "Tom" == Tom Lane  writes:

 Tom> I'm not convinced of that; I think some creative hackery in the
 Tom> grammar might be able to deal with this.

Making GROUP BY CUBE(a,b) parse as grouping sets rather than as a
function turned out to be the easy part: give CUBE a lower precedence
than '(' (equal to the one for IDENT and various other unreserved
keywords), and a rule that has an explicit CUBE '(' gets preferred
over one that reduces the CUBE to an unreserved_keyword.

The (relatively minor) ugliness required is mostly in the ruleutils
logic to decide how to output a cube(...) function call in such a way
that it doesn't get misparsed as a grouping set. See my other mail on
that.

-- 
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] SKIP LOCKED DATA (work in progress)

[Alvaro Herrera]

heap_lock_tuple() has the following comment on top:

 * In the failure cases, the routine fills *hufd with the tuple's t_ctid,
 * t_xmax (resolving a possible MultiXact, if necessary), and t_cmax
 * (the last only for HeapTupleSelfUpdated, since we
 * cannot obtain cmax from a combocid generated by another transaction).
 * See comments for struct HeapUpdateFailureData for additional info.

With the patch as submitted, this struct is not filled in the
HeapTupleWouldBlock case.  I'm not sure this is okay, though I admit the
only caller that passes LockWaitSkip doesn't care, so maybe we could
just ignore the issue (after properly modifying the comment).  It seems
easy to add a LockBuffer() and "goto failed" rather than a return; that
would make that failure case conform to HeapTupleUpdated and
HeapTupleSelfUpdated.  OTOH it might be pointless to worry about what
would be essentially dead code currently ...


Did you consider heap_lock_updated_tuple?  A rationale for saying it
doesn't need to pay attention to the wait policy is: if you're trying to
lock-skip-locked an updated tuple, then you either skip it because its
updater is running, or you return it because it's no longer running; and
if you return it, it is not possible for the updater to be locking the
updated version.  However, what if there's a third transaction that
locked the updated version?  It might be difficult to hit this case or
construct an isolationtester spec file though, since there's a narrow
window you need to race to.

I also pgindented.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 31073bc..5792036 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -109,8 +109,7 @@ static void MultiXactIdWait(MultiXactId multi, MultiXactStatus status, uint16 in
 Relation rel, ItemPointer ctid, XLTW_Oper oper,
 int *remaining);
 static bool ConditionalMultiXactIdWait(MultiXactId multi, MultiXactStatus status,
-		   uint16 infomask, Relation rel, ItemPointer ctid,
-		   XLTW_Oper oper, int *remaining);
+		   uint16 infomask, Relation rel, int *remaining);
 static XLogRecPtr log_heap_new_cid(Relation relation, HeapTuple tup);
 static HeapTuple ExtractReplicaIdentity(Relation rel, HeapTuple tup, bool key_modified,
 	   bool *copy);
@@ -2818,7 +2817,7 @@ l1:
 		if (result == HeapTupleSelfUpdated)
 			hufd->cmax = HeapTupleHeaderGetCmax(tp.t_data);
 		else
-			hufd->cmax = 0;		/* for lack of an InvalidCommandId value */
+			hufd->cmax = InvalidCommandId;
 		UnlockReleaseBuffer(buffer);
 		if (have_tuple_lock)
 			UnlockTupleTuplock(relation, &(tp.t_self), LockTupleExclusive);
@@ -3415,7 +3414,7 @@ l2:
 		if (result == HeapTupleSelfUpdated)
 			hufd->cmax = HeapTupleHeaderGetCmax(oldtup.t_data);
 		else
-			hufd->cmax = 0;		/* for lack of an InvalidCommandId value */
+			hufd->cmax = InvalidCommandId;
 		UnlockReleaseBuffer(buffer);
 		if (have_tuple_lock)
 			UnlockTupleTuplock(relation, &(oldtup.t_self), *lockmode);
@@ -4222,22 +4221,27 @@ l3:
 		 */
 		if (!have_tuple_lock)
 		{
-			if (wait_policy == LockWaitBlock)
+			switch (wait_policy)
 			{
-LockTupleTuplock(relation, tid, mode);
-			}
-			else if (wait_policy == LockWaitError)
-			{
-if (!ConditionalLockTupleTuplock(relation, tid, mode))
-	ereport(ERROR,
-			(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
-	errmsg("could not obtain lock on row in relation \"%s\"",
-		   RelationGetRelationName(relation;
-			}
-			else /* wait_policy == LockWaitSkip */
-			{
-if (!ConditionalLockTupleTuplock(relation, tid, mode))
-	return HeapTupleWouldBlock;
+case LockWaitBlock:
+	LockTupleTuplock(relation, tid, mode);
+	break;
+case LockWaitSkip:
+	if (!ConditionalLockTupleTuplock(relation, tid, mode))
+	{
+		result = HeapTupleWouldBlock;
+		/* recovery code expects to have buffer lock held */
+		LockBuffer(*buffer, BUFFER_LOCK_EXCLUSIVE);
+		goto failed;
+	}
+	break;
+case LockWaitError:
+	if (!ConditionalLockTupleTuplock(relation, tid, mode))
+		ereport(ERROR,
+(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
+ errmsg("could not obtain lock on row in relation \"%s\"",
+		RelationGetRelationName(relation;
+	break;
 			}
 			have_tuple_lock = true;
 		}
@@ -4441,34 +4445,34 @@ l3:
 if (status >= MultiXactStatusNoKeyUpdate)
 	elog(ERROR, "invalid lock mode in heap_lock_tuple");
 
-/* wait for multixact to end */
-if (wait_policy == LockWaitBlock)
-{
-	MultiXactIdWait((MultiXactId) xwait, status, infomask,
-	relation, &tuple->t_data->t_ctid,
-	XLTW_Lock, NULL);
-}
-else if (wait_policy == LockWaitError)
-{
-	if (!ConditionalMultiXactIdWait((MultiXactId) xwait,
-  status, infomask, relation,
-	&tuple->t_data->t_ctid

Re: [HACKERS] WIP Patch for GROUPING SETS phase 1

[Stephen Frost]

* Greg Stark (st...@mit.edu) wrote:
> On Fri, Aug 22, 2014 at 10:37 PM, Stephen Frost  wrote:
> > Agreed- and how many of those have *every extension available* loaded...
> 
> Actually that was also in the talk.a few slides later. 0.7%

So, 0.3% install cube w/o installing *every* extension..?  That seems
like the more relevant number then, to me anyway.

Admittedly, it's non-zero, but it's also a rather small percentage..

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] WIP Patch for GROUPING SETS phase 1

[Greg Stark]

On Fri, Aug 22, 2014 at 10:37 PM, Stephen Frost  wrote:
> Agreed- and how many of those have *every extension available* loaded...

Actually that was also in the talk.a few slides later. 0.7%

-- 
greg


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

Re: [HACKERS] WIP Patch for GROUPING SETS phase 1

[Stephen Frost]

* Andrew Dunstan (and...@dunslane.net) wrote:
> 
> On 08/22/2014 02:42 PM, Greg Stark wrote:
> >On Fri, Aug 22, 2014 at 7:02 PM, Tom Lane  wrote:
> >>So the proposal you are pushing is going
> >>to result in seriously teeing off some fraction of our userbase;
> >>and the argument why that would be acceptable seems to boil down to
> >>"I think there are few enough of them that we don't have to care"
> >>(an opinion based on little evidence IMO
> >FWIW here's some evidence... Craig Kersteins did a talk on the
> >statistics across the Heroku fleet: Here are the slides from 2013
> >though I think there's an updated slide deck with more recent numbers
> >out there:
> >https://speakerdeck.com/craigkerstiens/postgres-what-they-really-use
> >
> >Cube shows up as the number 9 most popular extension with about 1% of
> >databases having it installed (tied with pg_crypto and earthdistance).
> >That's a lot more than I would have expected actually.
> 
> 
> That's an interesting statistic. What I'd be more interested in is
> finding out how many of those are actually using it as opposed to
> having loaded it into a database.

Agreed- and how many of those have *every extension available* loaded...

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] WIP Patch for GROUPING SETS phase 1

[Merlin Moncure]

On Fri, Aug 22, 2014 at 1:52 PM, Robert Haas  wrote:
> On Fri, Aug 22, 2014 at 2:02 PM, Tom Lane  wrote:
> https://www.youtube.com/watch?v=MT2gzzbyWpw
>
> At around 8 minutes, he shows utilization statistics for cube at
> around 1% across their install base.  That's higher than I would have
> guessed, so, eh, shows what I know.  In any case, I'm not so much
> advocating not caring at all as confining the level of caring to what
> can be done without moving the earth.

cube is a dependency for earthdistance and it's gotten some light
advocacy throughout the years as the 'way to do it'.  I tried it
myself way back in the day and concluded a while back that the 'box'
type + gist was better than earthdistance for bounding box operations
-- it just seemed easier to understand and use.  If you search the
archives you'll probably find a couple of examples of me suggesting as
such.

merlin


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

Re: [HACKERS] WIP Patch for GROUPING SETS phase 1

[Andrew Dunstan]

On 08/22/2014 02:42 PM, Greg Stark wrote:

On Fri, Aug 22, 2014 at 7:02 PM, Tom Lane  wrote:

So the proposal you are pushing is going
to result in seriously teeing off some fraction of our userbase;
and the argument why that would be acceptable seems to boil down to
"I think there are few enough of them that we don't have to care"
(an opinion based on little evidence IMO

FWIW here's some evidence... Craig Kersteins did a talk on the
statistics across the Heroku fleet: Here are the slides from 2013
though I think there's an updated slide deck with more recent numbers
out there:
https://speakerdeck.com/craigkerstiens/postgres-what-they-really-use

Cube shows up as the number 9 most popular extension with about 1% of
databases having it installed (tied with pg_crypto and earthdistance).
That's a lot more than I would have expected actually.



That's an interesting statistic. What I'd be more interested in is 
finding out how many of those are actually using it as opposed to having 
loaded it into a database.


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] [GSoC2014] Patch ALTER TABLE ... SET LOGGED

[Tom Lane]

Alvaro Herrera  writes:
> Robert Haas wrote:
>> 1. Loggedness is not a word.  I think that "persistence" or
>> "relpersistence" would be better.

> You want me to change that to chgPersistence and so on?  No prob, just
> LMK.

+1 for s/loggedness/persistence/ -- I agree with Robert that it's
a bit grating on the native ear.

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] [GSoC2014] Patch ALTER TABLE ... SET LOGGED

[Tom Lane]

Robert Haas  writes:
> 2. The patch seems to think that it can sometimes be safe to change
> the relpersistence of an existing relation.  Unless you can be sure
> that no buffers can possibly be present in shared_buffers and nobody
> will use an existing relcache entry to read a new one in, it's not,
> because the buffers won't have the right BM_PERSISTENT marking.  I'm
> very nervous about the fact that this patch seems not to have touched
> bufmgr.c, but maybe I'm missing something.

Maybe I misunderstood something, but I had the impression that this was
handled by assigning a new relfilenode (and hence copying all the data).
So the buffers with one marking would be disjoint from the ones with the
other marking.

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] [GSoC2014] Patch ALTER TABLE ... SET LOGGED

[Alvaro Herrera]

Fabrízio de Royes Mello wrote:
> On Fri, Aug 22, 2014 at 4:45 PM, Alvaro Herrera 
> wrote:

> > I pointed out, in the email just before pushing the patch, that perhaps
> > we should pass down the new relpersistence flag into finish_heap_swap,
> > and from there we could pass it down to reindex_index() which is where
> > it would be needed.  I'm not sure it's worth the trouble, but I think we
> > can still ask Fabrizio to rework that part.

> I can rework this part if it's a real concern.

I guess we can make a better assessment by seeing what it would take.
I'm afraid it will turn out to be really ugly.

Now, there's some desire to have unlogged indexes on logged tables; I
guess if we have that, then eventually there will also be a desire to
swap individual indexes from logged to unlogged.  Perhaps whatever fix
we come up with here would pave the road for that future feature.

-- 
Álvaro Herrerahttp://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] [GSoC2014] Patch ALTER TABLE ... SET LOGGED

[Fabrízio de Royes Mello]

On Fri, Aug 22, 2014 at 4:45 PM, Alvaro Herrera 
wrote:
>
> > 2. The patch seems to think that it can sometimes be safe to change
> > the relpersistence of an existing relation.  Unless you can be sure
> > that no buffers can possibly be present in shared_buffers and nobody
> > will use an existing relcache entry to read a new one in, it's not,
> > because the buffers won't have the right BM_PERSISTENT marking.  I'm
> > very nervous about the fact that this patch seems not to have touched
> > bufmgr.c, but maybe I'm missing something.
>
> Right.  Andres pointed this out previously, and the patch was updated
> consequently.  The only remaining case in which we do that, again AFAIR,
> is for indexes, just after the table has been rewritten and just before
> the indexes are reindexed.  There should be no buffer access of the old
> indexes at that point, so there would be no buffer marked with the wrong
> flag.  Also, the table is locked access-exclusively (is that a word?),
> so no other transaction could possibly be reading buffers for its
> indexes.
>
> I pointed out, in the email just before pushing the patch, that perhaps
> we should pass down the new relpersistence flag into finish_heap_swap,
> and from there we could pass it down to reindex_index() which is where
> it would be needed.  I'm not sure it's worth the trouble, but I think we
> can still ask Fabrizio to rework that part.
>
> Maybe it is me missing something.
>
> BTW why is it that index_build() checks the heap's relpersistence flag
> rather than the index'?
>

I can rework this part if it's a real concern.

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] [GSoC2014] Patch ALTER TABLE ... SET LOGGED

[Alvaro Herrera]

Fabrízio de Royes Mello wrote:
> On Fri, Aug 22, 2014 at 4:22 PM, Robert Haas  wrote:

> > 2. The patch seems to think that it can sometimes be safe to change
> > the relpersistence of an existing relation.  Unless you can be sure
> > that no buffers can possibly be present in shared_buffers and nobody
> > will use an existing relcache entry to read a new one in, it's not,
> > because the buffers won't have the right BM_PERSISTENT marking.  I'm
> > very nervous about the fact that this patch seems not to have touched
> > bufmgr.c, but maybe I'm missing something.
> 
> Because of this concern I implement a solution pointed by Andres [1].

Actually what you did was better than what he suggested.

-- 
Álvaro Herrerahttp://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] [GSoC2014] Patch ALTER TABLE ... SET LOGGED

[Alvaro Herrera]

Robert Haas wrote:

> Hmm.  I confess to not having paid enough attention to this,

Sorry about that.  I guess I should somehow flag threads "I'm planning
to commit this" so that other people can review stuff carefully.

> but:
>
> 1. Loggedness is not a word.  I think that "persistence" or
> "relpersistence" would be better.

Yeah, AFAICS this is only used in the variable chgLoggedness and a
couple of functions.  I don't think we're tense about unwordness of
words we use in source code (as opposed to error messages and docs), and
we have lots of russianisms left by Vadim and others, so I didn't see it
as a serious issue.  But then I'm not a native english speaker and I'm
not bothered by it.  OTOH I came up with "loggedness" on my own -- you
wouldn't have seen it even in Fabrizio's latest version of the patch.

Who knows, it might become a real english word one day.

You want me to change that to chgPersistence and so on?  No prob, just
LMK.

> 2. The patch seems to think that it can sometimes be safe to change
> the relpersistence of an existing relation.  Unless you can be sure
> that no buffers can possibly be present in shared_buffers and nobody
> will use an existing relcache entry to read a new one in, it's not,
> because the buffers won't have the right BM_PERSISTENT marking.  I'm
> very nervous about the fact that this patch seems not to have touched
> bufmgr.c, but maybe I'm missing something.

Right.  Andres pointed this out previously, and the patch was updated
consequently.  The only remaining case in which we do that, again AFAIR,
is for indexes, just after the table has been rewritten and just before
the indexes are reindexed.  There should be no buffer access of the old
indexes at that point, so there would be no buffer marked with the wrong
flag.  Also, the table is locked access-exclusively (is that a word?),
so no other transaction could possibly be reading buffers for its
indexes.

I pointed out, in the email just before pushing the patch, that perhaps
we should pass down the new relpersistence flag into finish_heap_swap,
and from there we could pass it down to reindex_index() which is where
it would be needed.  I'm not sure it's worth the trouble, but I think we
can still ask Fabrizio to rework that part.

Maybe it is me missing something.

BTW why is it that index_build() checks the heap's relpersistence flag
rather than the index'?

-- 
Álvaro Herrerahttp://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] [GSoC2014] Patch ALTER TABLE ... SET LOGGED

[Fabrízio de Royes Mello]

On Fri, Aug 22, 2014 at 4:22 PM, Robert Haas  wrote:
>
> On Fri, Aug 22, 2014 at 2:32 PM, Alvaro Herrera
>  wrote:
> > Fabrízio de Royes Mello wrote:
> >> Em sexta-feira, 22 de agosto de 2014, Alvaro Herrera <
> >> alvhe...@2ndquadrant.com> escreveu:
> >>
> >> > Fabrízio de Royes Mello wrote:
> >> >
> >> > > I forgot to mention... I did again a lot of tests using different
> >> > > replication scenarios to make sure all is ok:
> >> > > - slaves async
> >> > > - slaves sync
> >> > > - cascade slaves
> >> >
> >> > On v13 you mean?
> >> >
> >> Exactly!
> >
> > Great.  Pushed.  Thanks for the patch.
>
> Hmm.  I confess to not having paid enough attention to this, but:
>
> 1. Loggedness is not a word.  I think that "persistence" or
> "relpersistence" would be better.
>

I changed to "Persistence"...


> 2. The patch seems to think that it can sometimes be safe to change
> the relpersistence of an existing relation.  Unless you can be sure
> that no buffers can possibly be present in shared_buffers and nobody
> will use an existing relcache entry to read a new one in, it's not,
> because the buffers won't have the right BM_PERSISTENT marking.  I'm
> very nervous about the fact that this patch seems not to have touched
> bufmgr.c, but maybe I'm missing something.
>

Because of this concern I implement a solution pointed by Andres [1].

Regards,

[1]
http://www.postgresql.org/message-id/20140717230220.gk21...@awork2.anarazel.de

--
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/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d37534e..5a233e2 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -152,7 +152,7 @@ typedef struct AlteredTableInfo
 	bool		new_notnull;	/* T if we added new NOT NULL constraints */
 	bool		rewrite;		/* T if a rewrite is forced */
 	Oid			newTableSpace;	/* new tablespace; 0 means no change */
-	bool		chgLoggedness;	/* T if SET LOGGED/UNLOGGED is used */
+	bool		chgPersistence;	/* T if SET LOGGED/UNLOGGED is used */
 	char		newrelpersistence;		/* if above is true */
 	/* Objects to rebuild after completing ALTER TYPE operations */
 	List	   *changedConstraintOids;	/* OIDs of constraints to rebuild */
@@ -388,8 +388,8 @@ static void change_owner_recurse_to_sequences(Oid relationOid,
 static void ATExecClusterOn(Relation rel, const char *indexName,
 LOCKMODE lockmode);
 static void ATExecDropCluster(Relation rel, LOCKMODE lockmode);
-static bool ATPrepChangeLoggedness(Relation rel, bool toLogged);
-static void ATChangeIndexesLoggedness(Oid relid, char relpersistence);
+static bool ATPrepChangePersistence(Relation rel, bool toLogged);
+static void ATChangeIndexesPersistence(Oid relid, char relpersistence);
 static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel,
 	char *tablespacename, LOCKMODE lockmode);
 static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode);
@@ -3174,19 +3174,19 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
 			break;
 		case AT_SetLogged:		/* SET LOGGED */
 			ATSimplePermissions(rel, ATT_TABLE);
-			tab->chgLoggedness = ATPrepChangeLoggedness(rel, true);
+			tab->chgPersistence = ATPrepChangePersistence(rel, true);
 			tab->newrelpersistence = RELPERSISTENCE_PERMANENT;
 			/* force rewrite if necessary */
-			if (tab->chgLoggedness)
+			if (tab->chgPersistence)
 tab->rewrite = true;
 			pass = AT_PASS_MISC;
 			break;
 		case AT_SetUnLogged:	/* SET UNLOGGED */
 			ATSimplePermissions(rel, ATT_TABLE);
-			tab->chgLoggedness = ATPrepChangeLoggedness(rel, false);
+			tab->chgPersistence = ATPrepChangePersistence(rel, false);
 			tab->newrelpersistence = RELPERSISTENCE_UNLOGGED;
 			/* force rewrite if necessary */
-			if (tab->chgLoggedness)
+			if (tab->chgPersistence)
 tab->rewrite = true;
 			pass = AT_PASS_MISC;
 			break;
@@ -3668,7 +3668,7 @@ ATRewriteTables(List **wqueue, LOCKMODE lockmode)
 			 * Select persistence of transient table (same as original unless
 			 * user requested a change)
 			 */
-			persistence = tab->chgLoggedness ?
+			persistence = tab->chgPersistence ?
 tab->newrelpersistence : OldHeap->rd_rel->relpersistence;
 
 			heap_close(OldHeap, NoLock);
@@ -3705,8 +3705,8 @@ ATRewriteTables(List **wqueue, LOCKMODE lockmode)
 			 * because the rewrite step might read the indexes, and that would
 			 * cause buffers for them to have the wrong setting.
 			 */
-			if (tab->chgLoggedness)
-ATChangeIndexesLoggedness(tab->relid, tab->newrelpersistence);
+			if (tab->chgPersistence)
+ATChangeIndexesPersistence(tab->relid, tab->newrelpersistence);
 
 			/*
 			 * Swap the physical files of the old and new heaps, then rebuild
@@ -4119,7 +4119,7 @@ ATGetQueueEntry(List **wqueue, Rela

Re: [HACKERS] SKIP LOCKED DATA (work in progress)

[Alvaro Herrera]

One thing I just noticed is that we uselessly set an error context
callback when "waiting" in ConditionalMultiXactIdWait, which is pretty
useless (because we don't actually wait there at all) -- we don't set
one in ConditionalXactLockTableWait, which makes sense, but for some
reason I failed to realize this in review of f88d4cfc9.  I think I will
take that out instead of cluttering this patch with it.

-- 
Álvaro Herrerahttp://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] 9.5: Better memory accounting, towards memory-bounded HashAgg

[Robert Haas]

On Fri, Aug 22, 2014 at 2:13 PM, Tomas Vondra  wrote:
> I don't think we really need to abandon the 'tracked' flag (or that we
> should). I think it was useful, and removing it might be one of the
> reasons why Robert now sees worse impact than before.

The version that introduced that flag had the same overhead as the
previous version that didn't, at least in my testing, so I'm not sure
it's at all useful.  Part of the problem here is that the number of
instructions that you can afford to take to complete a memory
allocation is just really small, and so every one hurts.  Memory
latency may be an issue as well, of course.

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


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

Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED

[Robert Haas]

On Fri, Aug 22, 2014 at 2:32 PM, Alvaro Herrera
 wrote:
> Fabrízio de Royes Mello wrote:
>> Em sexta-feira, 22 de agosto de 2014, Alvaro Herrera <
>> alvhe...@2ndquadrant.com> escreveu:
>>
>> > Fabrízio de Royes Mello wrote:
>> >
>> > > I forgot to mention... I did again a lot of tests using different
>> > > replication scenarios to make sure all is ok:
>> > > - slaves async
>> > > - slaves sync
>> > > - cascade slaves
>> >
>> > On v13 you mean?
>> >
>> Exactly!
>
> Great.  Pushed.  Thanks for the patch.

Hmm.  I confess to not having paid enough attention to this, but:

1. Loggedness is not a word.  I think that "persistence" or
"relpersistence" would be better.

2. The patch seems to think that it can sometimes be safe to change
the relpersistence of an existing relation.  Unless you can be sure
that no buffers can possibly be present in shared_buffers and nobody
will use an existing relcache entry to read a new one in, it's not,
because the buffers won't have the right BM_PERSISTENT marking.  I'm
very nervous about the fact that this patch seems not to have touched
bufmgr.c, but maybe I'm missing something.

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


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

Re: [HACKERS] [PATCH] Incremental backup: add backup profile to base backup

[Bruce Momjian]

On Fri, Aug 22, 2014 at 12:00:19PM -0400, Robert Haas wrote:
> On Wed, Aug 20, 2014 at 7:33 PM, Claudio Freire  
> wrote:
> > On Wed, Aug 20, 2014 at 8:24 PM, Bruce Momjian  wrote:
> >> On Mon, Aug 18, 2014 at 04:05:07PM +0300, Heikki Linnakangas wrote:
> >>> But more to the point, I thought the consensus was to use the
> >>> highest LSN of all the blocks in the file, no? That's essentially
> >>> free to calculate (if you have to read all the data anyway), and
> >>> isn't vulnerable to collisions.
> >>
> >> The highest-LSN approach allows you to read only the tail part of each
> >> 8k block.  Assuming 512-byte storage sector sizes, you only have to read
> >> 1/8 of the file.
> >>
> >> Now, the problem is that you lose kernel prefetch, but maybe
> >> posix_fadvise() would fix that problem.
> >
> > Sequential read of 512-byte blocks or 8k blocks takes the same amount
> > of time in rotating media (if they're scheduled right). Maybe not in
> > SSD media.
> >
> > Not only, the kernel will read in 4k blocks, instead of 8k (at least in 
> > linux).
> >
> > So, the benefit is dubious.
> 
> Agreed.  But, there could be a CPU benefit, too.  Pulling the LSN out
> of a block is probably a lot cheaper than checksumming the whole
> thing.

Oh, good, I hoped someone would find something useful about my idea.  :-)

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

  + Everyone has their own god. +


-- 
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] Is this a bug?

[Robert Haas]

On Fri, Aug 22, 2014 at 2:33 PM, Bruce Momjian  wrote:
>> Yes, you remember well.  I will have to find a different way for
>> pg_upgrade to call a no-op ALTER TABLE, which is fine.
>
> Looking at the ALTER TABLE options, I am going to put this check in a
> !IsBinaryUpgrade block so pg_upgrade can still use its trick.

-1, that's really ugly.

Maybe the right solution is to add a form of ALTER TABLE that is
specifically defined to do only this check.  This is an ongoing need,
so that might not be out of line.

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


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

Re: [HACKERS] Is this a bug?

[Andres Freund]

On August 22, 2014 8:33:57 PM CEST, Bruce Momjian  wrote:
>On Fri, Aug 22, 2014 at 12:53:30PM -0400, Bruce Momjian wrote:
>> On Fri, Aug 22, 2014 at 10:27:02AM -0400, Robert Haas wrote:
>> > On Thu, Aug 21, 2014 at 7:17 PM, Bruce Momjian 
>wrote:
>> > > On Tue, Mar 18, 2014 at 09:11:46AM -0400, Robert Haas wrote:
>> > >> On Mon, Mar 17, 2014 at 10:27 PM, Michael Paquier
>> > >>  wrote:
>> > >> > On Tue, Mar 18, 2014 at 10:24 AM, Fabrízio de Royes Mello
>> > >> >  wrote:
>> > >> >>
>> > >> >> On Thu, Mar 13, 2014 at 10:22 AM, Robert Haas
> wrote:
>> > >> >>> Well, it's fairly harmless, but it might not be a bad idea
>to tighten that
>> > >> >>> up.
>> > >> >> The attached patch tighten that up.
>> > >> > Hm... It might be interesting to include it in 9.4 IMO,
>somewhat
>> > >> > grouping with what has been done in a6542a4 for SET and ABORT.
>> > >>
>> > >> Meh.  There will always be another thing we could squeeze in; I
>don't
>> > >> think this is particularly urgent, and it's late to the party.
>> > >
>> > > Do we want this patch for 9.5?  It throws an error for invalid
>reloption
>> > > specifications.
>> > 
>> > Fine with me.  But I have a vague recollection of seeing pg_upgrade
>> > doing this on purpose to create TOAST tables or something... am I
>> > misremembering?
>> 
>> Yes, you remember well.  I will have to find a different way for
>> pg_upgrade to call a no-op ALTER TABLE, which is fine.
>
>Looking at the ALTER TABLE options, I am going to put this check in a
>!IsBinaryUpgrade block so pg_upgrade can still use it?

Why not simply not do anything?  This doesn't prevent any bugs and requiring 
pg-upgrade specific checks in there seems absurd. Also somebody might use it 
for similar purposes.




--- 
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] WIP Patch for GROUPING SETS phase 1

[Robert Haas]

On Fri, Aug 22, 2014 at 2:02 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Thu, Aug 21, 2014 at 2:13 PM, Tom Lane  wrote:
>>> Well, if there are any extant applications that use that exact phrasing,
>>> they're going to be broken in any case.  That does not mean that we have
>>> to break every other appearance of "cube".  I think that special-casing
>>> appearances of cube(...) in GROUP BY lists might be a feasible approach.
>
>> Not really.  As pointed out downthread, you can't distinguish "cube"
>> from CUBE.  We could fix that with a big enough hammer, of course, but
>> it would be a mighty big hammer.
>
> I'm not convinced of that; I think some creative hackery in the grammar
> might be able to deal with this.  It would be a bit ugly, for sure, but
> if it works it would be a localized fix.

Well, I have no idea how to do that.  I think the only way you'd be
able to is if you make productions like ColId and ColLabel return
something different for a keyword than they do for an IDENT.  And
that's not going to be a localized change.  If you've got another
proposal, I'm all ears...

> Meanwhile, I don't believe
> that it's going to be possible to rename the cube extension in any way
> that's even remotely acceptable for its users ("remotely acceptable"
> here means "pg_upgrade works", never mind what's going to be needed
> to fix their applications).
>
> So the proposal you are pushing is going
> to result in seriously teeing off some fraction of our userbase;
> and the argument why that would be acceptable seems to boil down to
> "I think there are few enough of them that we don't have to care"
> (an opinion based on little evidence IMO).

The only hard statistics I am aware of are from Heroku.  Peter
Geoghegan was kind enough to find me the link:

https://www.youtube.com/watch?v=MT2gzzbyWpw

At around 8 minutes, he shows utilization statistics for cube at
around 1% across their install base.  That's higher than I would have
guessed, so, eh, shows what I know.  In any case, I'm not so much
advocating not caring at all as confining the level of caring to what
can be done without moving the earth.

> I think it's worth investing
> some work, and perhaps accepting some ugly code, to try to avoid that.

I can accept ugly code, but I feel strongly that we shouldn't accept
ugly semantics.  Forcing cube to get out of the way may not be pretty,
 but I think it will be much worse if we violate the rule that quoting
a keyword strips it of its special meaning; or the rule that there are
four kinds of keywords and, if a keyword of a particular class is
accepted as an identifier in a given context, all other keywords in
that class will also be accepted as identifiers in that context.
Violating those rules could have not-fun-at-all consequences like
needing to pass additional context information to ruleutils.c's
quote_identifier() function, or not being able to dump and restore a
query from an older version even with --quote-all-identifiers.
Renaming the cube type will suck for people who are using it; but it
will only have to be done once; weird stuff like the above will be
with us forever.

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


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

Re: [HACKERS] B-Tree support function number 3 (strxfrm() optimization)

[Peter Geoghegan]

On Fri, Aug 22, 2014 at 7:19 AM, Robert Haas  wrote:
> Patch 0002 no longer applies; please rebase.

I attach rebased patch.

Note that there is currently a bug in the master branch:

+   if (len2 >= tss->buflen2)
+   {
+   pfree(tss->buf2);
+   tss->buflen1 = Max(len2 + 1, Min(tss->buflen2 * 2, MaxAllocSize));
+   tss->buf2 = MemoryContextAlloc(ssup->ssup_cxt, tss->buflen2);
+   }

Thanks
-- 
Peter Geoghegan
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
new file mode 100644
index c09ca7e..adf4b8c
*** a/src/backend/commands/analyze.c
--- b/src/backend/commands/analyze.c
*** compute_scalar_stats(VacAttrStatsP stats
*** 2292,2297 
--- 2292,2303 
  	/* We always use the default collation for statistics */
  	ssup.ssup_collation = DEFAULT_COLLATION_OID;
  	ssup.ssup_nulls_first = false;
+ 	/*
+ 	 * For now, don't perform abbreviated key conversion, because full values
+ 	 * are required for MCV slot generation.  Supporting that optimization
+ 	 * would necessitate teaching compare_scalars() to call a tie-breaker.
+ 	 */
+ 	ssup.position = sortKeyOther;
  
  	PrepareSortSupportFromOrderingOp(mystats->ltopr, &ssup);
  
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
new file mode 100644
index 510d1c5..3b335e7
*** a/src/backend/executor/nodeAgg.c
--- b/src/backend/executor/nodeAgg.c
*** initialize_aggregates(AggState *aggstate
*** 346,351 
--- 346,352 
  	{
  		AggStatePerAgg peraggstate = &peragg[aggno];
  		AggStatePerGroup pergroupstate = &pergroup[aggno];
+ 		Agg		   		*node = (Agg *) aggstate->ss.ps.plan;
  
  		/*
  		 * Start a fresh sort operation for each DISTINCT/ORDER BY aggregate.
*** initialize_aggregates(AggState *aggstate
*** 363,368 
--- 364,373 
  			 * We use a plain Datum sorter when there's a single input column;
  			 * otherwise sort the full tuple.  (See comments for
  			 * process_ordered_aggregate_single.)
+ 			 *
+ 			 * FIXME: It ought to be useful to force tuplesort_begin_heap()
+ 			 * case where the abbreviated key optimization can thereby be used,
+ 			 * even when numInputs == 1.
  			 */
  			peraggstate->sortstate =
  (peraggstate->numInputs == 1) ?
*** initialize_aggregates(AggState *aggstate
*** 377,383 
  	 peraggstate->sortOperators,
  	 peraggstate->sortCollations,
  	 peraggstate->sortNullsFirst,
! 	 work_mem, false);
  		}
  
  		/*
--- 382,390 
  	 peraggstate->sortOperators,
  	 peraggstate->sortCollations,
  	 peraggstate->sortNullsFirst,
! 	 work_mem,
! 	 node->plan.plan_rows,
! 	 false);
  		}
  
  		/*
diff --git a/src/backend/executor/nodeMergeAppend.c b/src/backend/executor/nodeMergeAppend.c
new file mode 100644
index 47ed068..677a953
*** a/src/backend/executor/nodeMergeAppend.c
--- b/src/backend/executor/nodeMergeAppend.c
*** ExecInitMergeAppend(MergeAppend *node, E
*** 137,142 
--- 137,151 
  		sortKey->ssup_nulls_first = node->nullsFirst[i];
  		sortKey->ssup_attno = node->sortColIdx[i];
  
+ 		/*
+ 		 * It isn't feasible to perform abbreviated key conversion, since
+ 		 * tuples are pulled into mergestate's binary heap as needed.  It would
+ 		 * likely be counter-productive to convert tuples into an abbreviated
+ 		 * representation as they're pulled up, so opt out of that additional
+ 		 * optimization entirely.
+ 		 */
+ 		sortKey->position = sortKeyOther;
+ 
  		PrepareSortSupportFromOrderingOp(node->sortOperators[i], sortKey);
  	}
  
diff --git a/src/backend/executor/nodeMergejoin.c b/src/backend/executor/nodeMergejoin.c
new file mode 100644
index fdf2f4c..39f0ff1
*** a/src/backend/executor/nodeMergejoin.c
--- b/src/backend/executor/nodeMergejoin.c
*** MJExamineQuals(List *mergeclauses,
*** 229,234 
--- 229,242 
  			elog(ERROR, "cannot merge using non-equality operator %u",
   qual->opno);
  
+ 		/*
+ 		 * sortsupport routine must know if abbreviation optimization is
+ 		 * applicable in principle.  It is never applicable for merge joins
+ 		 * because there is no convenient opportunity to convert to alternative
+ 		 * representation.  Only elide fmgr trampoline where supported.
+ 		 */
+ 		clause->ssup.position = sortKeyOther;
+ 
  		/* And get the matching support or comparison function */
  		Assert(clause->ssup.comparator == NULL);
  		sortfunc = get_opfamily_proc(opfamily,
diff --git a/src/backend/executor/nodeSort.c b/src/backend/executor/nodeSort.c
new file mode 100644
index b88571b..31d3ead
*** a/src/backend/executor/nodeSort.c
--- b/src/backend/executor/nodeSort.c
*** ExecSort(SortState *node)
*** 89,94 
--- 89,95 
  			  plannode->collations,
  			  plannode->nullsFirst,
  			  work_mem,
+ 			  plannode->plan.plan_rows,
  			  node->randomAccess);
  		if (node->bounded)
  			tuplesort_set_bound(tuplesortstate,

Re: [HACKERS] WIP Patch for GROUPING SETS phase 1

[Greg Stark]

On Fri, Aug 22, 2014 at 7:02 PM, Tom Lane  wrote:
> So the proposal you are pushing is going
> to result in seriously teeing off some fraction of our userbase;
> and the argument why that would be acceptable seems to boil down to
> "I think there are few enough of them that we don't have to care"
> (an opinion based on little evidence IMO

FWIW here's some evidence... Craig Kersteins did a talk on the
statistics across the Heroku fleet: Here are the slides from 2013
though I think there's an updated slide deck with more recent numbers
out there:
https://speakerdeck.com/craigkerstiens/postgres-what-they-really-use

Cube shows up as the number 9 most popular extension with about 1% of
databases having it installed (tied with pg_crypto and earthdistance).
That's a lot more than I would have expected actually.

Personally I would love to change the name because I always found the
name the most confusing thing about it. It took me forever to figure
out what on earth a "cube" was. It's actually a vector data type which
is actually a pretty useful idea.

-- 
greg


-- 
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] [GSoC2014] Patch ALTER TABLE ... SET LOGGED

[Fabrízio de Royes Mello]

On Fri, Aug 22, 2014 at 3:32 PM, Alvaro Herrera 
wrote:
>
> Fabrízio de Royes Mello wrote:
> > Em sexta-feira, 22 de agosto de 2014, Alvaro Herrera <
> > alvhe...@2ndquadrant.com> escreveu:
> >
> > > Fabrízio de Royes Mello wrote:
> > >
> > > > I forgot to mention... I did again a lot of tests using different
> > > > replication scenarios to make sure all is ok:
> > > > - slaves async
> > > > - slaves sync
> > > > - cascade slaves
> > >
> > > On v13 you mean?
> > >
> > Exactly!
>
> Great.  Pushed.  Thanks for the patch.
>

Awesome!!! Actually I should say thank you my friend!! See you in
Campinas...

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] Is this a bug?

[Bruce Momjian]

On Fri, Aug 22, 2014 at 12:53:30PM -0400, Bruce Momjian wrote:
> On Fri, Aug 22, 2014 at 10:27:02AM -0400, Robert Haas wrote:
> > On Thu, Aug 21, 2014 at 7:17 PM, Bruce Momjian  wrote:
> > > On Tue, Mar 18, 2014 at 09:11:46AM -0400, Robert Haas wrote:
> > >> On Mon, Mar 17, 2014 at 10:27 PM, Michael Paquier
> > >>  wrote:
> > >> > On Tue, Mar 18, 2014 at 10:24 AM, Fabrízio de Royes Mello
> > >> >  wrote:
> > >> >>
> > >> >> On Thu, Mar 13, 2014 at 10:22 AM, Robert Haas  
> > >> >> wrote:
> > >> >>> Well, it's fairly harmless, but it might not be a bad idea to 
> > >> >>> tighten that
> > >> >>> up.
> > >> >> The attached patch tighten that up.
> > >> > Hm... It might be interesting to include it in 9.4 IMO, somewhat
> > >> > grouping with what has been done in a6542a4 for SET and ABORT.
> > >>
> > >> Meh.  There will always be another thing we could squeeze in; I don't
> > >> think this is particularly urgent, and it's late to the party.
> > >
> > > Do we want this patch for 9.5?  It throws an error for invalid reloption
> > > specifications.
> > 
> > Fine with me.  But I have a vague recollection of seeing pg_upgrade
> > doing this on purpose to create TOAST tables or something... am I
> > misremembering?
> 
> Yes, you remember well.  I will have to find a different way for
> pg_upgrade to call a no-op ALTER TABLE, which is fine.

Looking at the ALTER TABLE options, I am going to put this check in a
!IsBinaryUpgrade block so pg_upgrade can still use its trick.

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

  + Everyone has their own god. +


-- 
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] [GSoC2014] Patch ALTER TABLE ... SET LOGGED

[Alvaro Herrera]

Fabrízio de Royes Mello wrote:
> Em sexta-feira, 22 de agosto de 2014, Alvaro Herrera <
> alvhe...@2ndquadrant.com> escreveu:
> 
> > Fabrízio de Royes Mello wrote:
> >
> > > I forgot to mention... I did again a lot of tests using different
> > > replication scenarios to make sure all is ok:
> > > - slaves async
> > > - slaves sync
> > > - cascade slaves
> >
> > On v13 you mean?
> >
> Exactly!

Great.  Pushed.  Thanks for the patch.

-- 
Álvaro Herrerahttp://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] 9.5: Better memory accounting, towards memory-bounded HashAgg

[Tomas Vondra]

On 20.8.2014 08:11, Jeff Davis wrote:
> On Tue, 2014-08-19 at 12:54 +0200, Tomas Vondra wrote:
>> The use-case for this is tracking a chosen subtree of contexts - e.g.
>> aggcontext and below, so I'd expect the tracked subtrees to be relatively
>> shallow. Am I right?
> 
> Right.
> 
>> My fear is that by removing the inheritance bit, we'll hurt cases with a
>> lot of child contexts. For example, array_agg currently creates a separate
>> context for each group - what happens if you have 100k groups and do
>> MemoryContextGetAllocated? I guess iterating over 100k groups is not free.
> 
> Good point. We don't want to make checking the allocated space into an
> expensive operation, because then we will be forced to call it less
> frequently, which implies that we'd be sloppier about actually fitting
> in work_mem.
> 
>> Wouldn't the solution with inheritance and propagating the accounting info
>> to the parent actually better? Or maybe allowing both, having two flags
>> when creating a context instead of one?
> 
> That seems overly complicated. We only have one driving use case, so two
> orthogonal options sounds excessive. Perhaps one option if we can't
> solve the performance problem and we need to isolate the changes to
> hashagg.
> 
>> Also, do we really need to track allocated bytes - couldn't we track
>> kilobytes or something and use smaller data types to get below the 64B?
> 
> Good idea.
> 
> I attached a patch that uses two uint32 fields so that it doesn't
> increase the size of MemoryContextData, and it tracks memory usage for
> all contexts. I was unable to detect any performance regression vs.
> master, but on my machine the results are noisy.

I don't think we really need to abandon the 'tracked' flag (or that we
should). I think it was useful, and removing it might be one of the
reasons why Robert now sees worse impact than before.

I assume you did that to get below 64B, right? What about to changing
'isReset' in MemoryContextData to a 'flags' field instead? It's a bool
now, i.e. 1B -> 8 bits.

I don't think it makes sense to abandon this only to get <= 64B, if it
forces you to touch multiple 64B structures unnecessarily.

Also, you're doing this for every block in AllocSetReset:

update_allocation(context, -(block->endptr - ((char*) block)));

So if there are 1000 blocks, you'll call that 1000x (and it will
propagate up to TopMemoryContext). Instead keep a local sum and only do
the call once at the end. Again, this might be one of the reasons why
Robet sees ~4% overhead.

BTW what happens when AllocSetContext is created with initBlockSize that
is not a multiple of 1kB? I see it's enforced to be at least 1024, but I
don't see anything forcing it to be a multiple of 1024?

What if I create a context with initBlockSize=1500? ISTM it'll break the
accounting, right? (Not that it would make sense to create such
contexts, but it's possible.)

Tomas


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

Re: [HACKERS] WIP Patch for GROUPING SETS phase 1

[Tom Lane]

Robert Haas  writes:
> On Thu, Aug 21, 2014 at 2:13 PM, Tom Lane  wrote:
>> Well, if there are any extant applications that use that exact phrasing,
>> they're going to be broken in any case.  That does not mean that we have
>> to break every other appearance of "cube".  I think that special-casing
>> appearances of cube(...) in GROUP BY lists might be a feasible approach.

> Not really.  As pointed out downthread, you can't distinguish "cube"
> from CUBE.  We could fix that with a big enough hammer, of course, but
> it would be a mighty big hammer.

I'm not convinced of that; I think some creative hackery in the grammar
might be able to deal with this.  It would be a bit ugly, for sure, but
if it works it would be a localized fix.  Meanwhile, I don't believe
that it's going to be possible to rename the cube extension in any way
that's even remotely acceptable for its users ("remotely acceptable"
here means "pg_upgrade works", never mind what's going to be needed
to fix their applications).  So the proposal you are pushing is going
to result in seriously teeing off some fraction of our userbase;
and the argument why that would be acceptable seems to boil down to
"I think there are few enough of them that we don't have to care"
(an opinion based on little evidence IMO).  I think it's worth investing
some work, and perhaps accepting some ugly code, to try to avoid that.

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] WIP Patch for GROUPING SETS phase 1

[Robert Haas]

On Thu, Aug 21, 2014 at 2:13 PM, Tom Lane  wrote:
> Andrew Gierth  writes:
>> "Tom" == Tom Lane  writes:
>>  Tom> I wonder if you've tried hard enough to avoid reserving the keyword.
>
>> GROUP BY cube(a,b)  is currently legal syntax and means something completely
>> incompatible to what the spec requires.
>
> Well, if there are any extant applications that use that exact phrasing,
> they're going to be broken in any case.  That does not mean that we have
> to break every other appearance of "cube".  I think that special-casing
> appearances of cube(...) in GROUP BY lists might be a feasible approach.

Not really.  As pointed out downthread, you can't distinguish "cube"
from CUBE.  We could fix that with a big enough hammer, of course, but
it would be a mighty big hammer.

More generally, I think it makes a lot of sense to "work harder" to
reserve keywords less when there's no fundamental semantic conflict,
but when there is, trying to do things like special case stuff
depending on context results in a situation where some keywords are a
little more reserved than others.  As you pointed out in discussions
of CREATE INDEX CONCURRENTLY, that's confusing:

http://www.postgresql.org/message-id/10769.1261775...@sss.pgh.pa.us
(refer second paragraph)

I think we should:

(1) Rename the cube extension.  With a bat.  I have yet to encounter a
single user who is using it, but there probably are some.  They'll
have to get over it; GROUPING SETS is roughly a hundred times more
important than the cube extension.  The most I'd do to cater to
existing users of the extension is provide an SQL script someplace
that renames the extension and all of its containing objects so that
you can do that before running pg_dump/pg_upgrade.

(2) Reserve CUBE to the extent necessary to implement this feature.
Some people won't like this, but that's always true when we reserve a
keyword, and I don't think refusing to implement an SQL-standard
feature for which there is considerable demand is the right way to fix
that.  Here are the last five keywords we partially or fully reserved:
LATERAL (fully reserved), COLLATION (type_func_name), XMLEXISTS
(col_name), BETWEEN (was type_func_name, became col_name),
CONCURRENTLY (type_func_name).  That takes us back to December 2009,
so the rate at which we do this is just under one a year, and there
haven't been many screams about it.  I grant you that "cube" is a
slightly more plausible identifier than any of those, but I don't
think we should let the fact that we happen to have an extension with
that name prejudice us too much about how common it really is.

Mind you, I'm not trying to say that we don't need to be judicious in
reserving keywords, or even adding them at all: I've argued against
those things on numerous occasions, and have done work to let us get
rid of keywords we've previously had.  I just think that this is a big
enough, important enough feature that we'll please more people than we
disappoint.  And I think trying to walk some middle way where we
distinguish on context is going to be a mess.

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


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

Re: [HACKERS] Is this a bug?

[Bruce Momjian]

On Fri, Aug 22, 2014 at 10:27:02AM -0400, Robert Haas wrote:
> On Thu, Aug 21, 2014 at 7:17 PM, Bruce Momjian  wrote:
> > On Tue, Mar 18, 2014 at 09:11:46AM -0400, Robert Haas wrote:
> >> On Mon, Mar 17, 2014 at 10:27 PM, Michael Paquier
> >>  wrote:
> >> > On Tue, Mar 18, 2014 at 10:24 AM, Fabrízio de Royes Mello
> >> >  wrote:
> >> >>
> >> >> On Thu, Mar 13, 2014 at 10:22 AM, Robert Haas  
> >> >> wrote:
> >> >>> Well, it's fairly harmless, but it might not be a bad idea to tighten 
> >> >>> that
> >> >>> up.
> >> >> The attached patch tighten that up.
> >> > Hm... It might be interesting to include it in 9.4 IMO, somewhat
> >> > grouping with what has been done in a6542a4 for SET and ABORT.
> >>
> >> Meh.  There will always be another thing we could squeeze in; I don't
> >> think this is particularly urgent, and it's late to the party.
> >
> > Do we want this patch for 9.5?  It throws an error for invalid reloption
> > specifications.
> 
> Fine with me.  But I have a vague recollection of seeing pg_upgrade
> doing this on purpose to create TOAST tables or something... am I
> misremembering?

Yes, you remember well.  I will have to find a different way for
pg_upgrade to call a no-op ALTER TABLE, which is fine.

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

  + Everyone has their own god. +


-- 
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] 9.5: Better memory accounting, towards memory-bounded HashAgg

[Robert Haas]

On Wed, Aug 20, 2014 at 2:11 AM, Jeff Davis  wrote:
> I attached a patch that uses two uint32 fields so that it doesn't
> increase the size of MemoryContextData, and it tracks memory usage for
> all contexts. I was unable to detect any performance regression vs.
> master, but on my machine the results are noisy.

Still doesn't look good here.  On the same PPC64 machine I've been
using for testing:

master:
2014-08-22 16:26:42 UTC [13153] LOG:  internal sort ended, 1723933 KB
used: CPU 2.19s/11.41u sec elapsed 16.40 sec
2014-08-22 16:28:18 UTC [13173] LOG:  internal sort ended, 1723933 KB
used: CPU 2.56s/11.37u sec elapsed 16.64 sec
2014-08-22 16:29:37 UTC [13194] LOG:  internal sort ended, 1723933 KB
used: CPU 2.57s/11.48u sec elapsed 16.75 sec

memory-accounting-v4:
2014-08-22 16:27:35 UTC [13163] LOG:  internal sort ended, 1723933 KB
used: CPU 2.72s/11.91u sec elapsed 17.43 sec
2014-08-22 16:29:10 UTC [13185] LOG:  internal sort ended, 1723933 KB
used: CPU 2.67s/12.03u sec elapsed 17.40 sec
2014-08-22 16:30:01 UTC [13203] LOG:  internal sort ended, 1723933 KB
used: CPU 2.65s/11.97u sec elapsed 17.45 sec

That's a 4.7% regression on the median results, worse than before.

> It would be easier to resolve the performance concern if I could
> reliably get the results Robert is getting. I think I was able to
> reproduce the regression with the old patch, but the results were still
> noisy.

If you send me an SSH key by private email I can set you an account up
on this machine, if that's helpful.

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


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

Re: [HACKERS] change alter user to be a true alias for alter role

[Jov]

I make the v2 of the patch,use Tom's advice.

But I can't make ROLE and USER in the keyword list,it is hard to solve the
conflict,or rewrite many gram rules.
the problem is :

role_or_user : ROLE | USER;
xx_keyword:...| ROLE|...|USER..;

this two rules produce conflict.So in v2,I remove ROLE and USER from the
xx_keyword rule now?
Any idea?

Jov
blog: http:amutu.com/blog 


2014-07-09 18:27 GMT+08:00 Jov :

> Sorry for the late resp,I will try to update the patch.
>
> Jov
> blog: http:amutu.com/blog 
>
>
> 2014-07-02 4:17 GMT+08:00 Abhijit Menon-Sen :
>
> At 2014-06-27 16:11:21 +0200, vik.fear...@dalibo.com wrote:
>> >
>> > After a week of silence from Jov, I decided to do this myself since it
>> > didn't seem very hard.
>> >
>> > Many frustrating hours of trying to understand why I'm getting
>> > shift/reduce conflicts by the hundreds later, I've decided to give up
>> > for now.
>>
>> Jov, do you expect to be able to work on the patch along the lines Tom
>> suggested and resubmit during this CF?
>>
>> If not, since Vik gave up on it, it seems to me that it would be best to
>> mark it returned with feedback (which I'll do in a couple of days if I
>> don't hear anything to the contrary).
>>
>> -- Abhijit
>>
>
>


full-imp-alter-user-v2.patch.gz
Description: GNU Zip compressed 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] [v9.5] Custom Plan API

[Tom Lane]

Robert Haas  writes:
> I think the threshold question for this incarnation of the patch is
> whether we're happy with new DDL (viz, CREATE CUSTOM PLAN PROVIDER) as
> a way of installing new plan providers into the database.

I tend to agree with your conclusion that that's a whole lot of
infrastructure with very little return.  I don't see anything here
we shouldn't do via function hooks instead, and/or a "register" callback
from a dynamically loaded library.

Also, we tend to think (for good reason) that once something is embedded
at the SQL level it's frozen; we are much more willing to redesign C-level
APIs.  There is no possible way that it's a good idea for this stuff to
get frozen in its first iteration.

regards, tom lane


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

Re: [HACKERS] [PATCH] Incremental backup: add backup profile to base backup

[Robert Haas]

On Wed, Aug 20, 2014 at 7:33 PM, Claudio Freire  wrote:
> On Wed, Aug 20, 2014 at 8:24 PM, Bruce Momjian  wrote:
>> On Mon, Aug 18, 2014 at 04:05:07PM +0300, Heikki Linnakangas wrote:
>>> But more to the point, I thought the consensus was to use the
>>> highest LSN of all the blocks in the file, no? That's essentially
>>> free to calculate (if you have to read all the data anyway), and
>>> isn't vulnerable to collisions.
>>
>> The highest-LSN approach allows you to read only the tail part of each
>> 8k block.  Assuming 512-byte storage sector sizes, you only have to read
>> 1/8 of the file.
>>
>> Now, the problem is that you lose kernel prefetch, but maybe
>> posix_fadvise() would fix that problem.
>
> Sequential read of 512-byte blocks or 8k blocks takes the same amount
> of time in rotating media (if they're scheduled right). Maybe not in
> SSD media.
>
> Not only, the kernel will read in 4k blocks, instead of 8k (at least in 
> linux).
>
> So, the benefit is dubious.

Agreed.  But, there could be a CPU benefit, too.  Pulling the LSN out
of a block is probably a lot cheaper than checksumming the whole
thing.

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


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

Re: [HACKERS] [PATCH] Incremental backup: add backup profile to base backup

[Robert Haas]

On Mon, Aug 18, 2014 at 4:55 PM, Heikki Linnakangas
 wrote:
> You're not thinking evil enough ;-). Let's say that you have a table that
> stores bank transfers. You can do a bank transfer to pay a merchant, get the
> goods delivered to you, and then a second transfer to yourself with a
> specially crafted message attached to it that makes the checksum match the
> state before the first transfer. If the backup is restored (e.g. by a daily
> batch job to a reporting system), it will appear as if neither transfer
> happened, and you get to keep your money.
>
> Far-fetched? Well, how about this scenario: a vandal just wants to cause
> damage. Creating a situation where a restore from backup causes the system
> to be inconsistent will certainly cause headaches to the admins, and leave
> them wondering what else is corrupt.
>
> Or how about this: you can do the trick to a system catalog, say
> pg_attribute, to make it look like a column is of type varlena, when it's
> actually since been ALTERed to be an integer. Now you can access arbitrary
> memory in the server, and take over the whole system.
>
> I'm sure any or all of those scenarios are highly inpractical when you
> actually sit down and try to do it, but you don't want to rely on that. You
> have to be able to trust your backups.

Yeah.  I agree that these scenarios are far-fetched; however, they're
also preventable, so we should.

Also, with respect to checksum collisions, you figure to have an
*accidental* checksum collision every so often as well.  For block
checksums, we're using a 16-bit value, which is OK because we'll still
detect 65535/65536 = 99.998% of corruption events.  The requirements
are much higher for incremental backup, because a checksum collision
here means automatic data corruption.  If we were crazy enough to use
a 16-bit block-level checksum in this context, about one
actually-modified block would fail to get copied out of every 8kB *
64k = 512MB of modified data, which would not make anybody very happy.
A 32-bit checksum would be much safer, and a 64-bit checksum would be
better still, but block LSNs seem better still.

Of course, there's one case where block LSNs aren't better, which is
where somebody goes backwards in time - i.e. back up the database, run
it for a while, take a base backup, shut down, restore from backup, do
stuff that's different from what you did the first time through, try
to take an incremental backup against your base backup.  LSNs won't
catch that; checksums will.  Do we want to allow incremental backup in
that kind of situation?  It seems like playing with fire, but it's
surely not useless.

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


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

Re: [HACKERS] [v9.5] Custom Plan API

[Robert Haas]

On Thu, Jul 17, 2014 at 3:38 PM, Tom Lane  wrote:
> Alvaro Herrera  writes:
>> I haven't followed this at all, but I just skimmed over it and noticed
>> the CustomPlanMarkPos thingy; apologies if this has been discussed
>> before.  It seems a bit odd to me; why isn't it sufficient to have a
>> boolean flag in regular CustomPlan to indicate that it supports
>> mark/restore?
>
> Yeah, I thought that was pretty bogus too, but it's well down the
> list of issues that were there last time I looked at this ...

I think the threshold question for this incarnation of the patch is
whether we're happy with new DDL (viz, CREATE CUSTOM PLAN PROVIDER) as
a way of installing new plan providers into the database.  If we are,
then I can go ahead and enumerate a long list of things that will need
to be fixed to make that code acceptable (such as adding pg_dump
support).  But if we're not, there's no point in spending any time on
that part of the patch.

I can see a couple of good reasons to think that this approach might
be reasonable:

- In some ways, a custom plan provider (really, at this point, a
custom scan provider) is very similar to a foreign data wrapper.  To
the guts of PostgreSQL, an FDW is a sort of black box that knows how
to scan some data not managed by PostgreSQL.  A custom plan provider
is similar, except that it scans data that *is* managed by PostgreSQL.

- There's also some passing resemblance between a custom plan provider
and an access method.  Access methods provide a set of APIs for fast
access to data via an index, while custom plan providers provide an
API for fast access to data via magic that the core system doesn't
(and need not) understand.  While access methods don't have associated
SQL syntax, they do include catalog structure, so perhaps this should
too, by analogy.

All that having been said, I'm having a hard time mustering up
enthusiasm for this way of doing things.  As currently constituted,
the pg_custom_plan_provider catalog contains only a name, a "class"
that is always 's' for scan, and a handler function OID.  Quite
frankly, that's a whole lot of nothing.  If we got rid of the
pg_catalog structure and just had something like
RegisterCustomPlanProvider(char *name, void (*)(customScanArg *),
which could be invoked from _PG_init(), hundreds and hundreds of lines
of code could go away and we wouldn't lose any actual functionality;
you'd just list your custom plan providers in shared_preload_libraries
or local_preload_libraries instead of listing them in a system
catalog.  In fact, you might even have more functionality, because you
could load providers into particular sessions rather than system-wide,
which isn't possible with this design.

I think the underlying issue here really has to do with when custom
plan providers get invoked - what triggers that?  For foreign data
wrappers, we have some relations that are plain tables (relkind = 'r')
and no foreign data wrapper code is invoked.  We have others that are
flagged as foreign tables (relkind = 'f') and for those we look up the
matching FDW (via ftserver) and run the code.  Similarly, for an index
AM, we notice that the relation is an index (relkind = 'r') and then
consult relam to figure out which index AM we should invoke.  But as
KaiGai is conceiving this feature, it's quite different.  Rather than
applying only to particular relations, and being mutually exclusive
with other options that might apply to those relations, it applies to
*everything* in the database in addition to whatever other options may
be present.  The included ctidscan implementation is just as good an
example as PG-Strom: you inspect the query and see, based on the
operators present, whether there's any hope of accelerating things.
In other words, there's no user configuration - and also, not
irrelevantly, no persistent on-disk state the way you have for an
index, or even an FDW, which has on disk state to the extent that
there have to be catalog entries tying a particular FDW to a
particular table.

A lot of the previous discussion of this topic revolves around the
question of whether we can unify the use case that this patch is
targeting with other things - e.g. Citus's desire to store its data
files within the data directory while retaining control over data
access, thus not a perfect fit for FDWs; the desire to push joins down
to foreign servers; more generally, the desire to replace a join with
a custom plan that may or may not use access paths for the underlying
relations as subpaths.  I confess I'm not seeing a whole lot of
commonality with anything other than the custom-join-path idea, which
probably shares many of what I believe to be the relevant
characteristics of a custom scan as conceived by KaiGai: namely, that
all of the decisions about whether to inject a custom path in
particular circumstances are left up to the provider itself based on
inspection of the specific query, rather than being a result of user
configuration.

So, I'm tentatively in 

Re: [HACKERS] WIP Patch for GROUPING SETS phase 1

[Andrew Gierth]

> "Alvaro" == Alvaro Herrera  writes:

 >> (This of course means that if someone has a cube() function call
 >> in a group by clause of a view, then upgrading will change the
 >> meaning of the view and possibly fail to create it; there seems to
 >> be no fix for this, not even using the latest pg_dump, since
 >> pg_dump relies on the old server's ruleutils)

 Alvaro> This sucks.  Can we tweak pg_dump to check for presence of
 Alvaro> the cube extension, and if found refuse to dump unless a
 Alvaro> minor version older than some hardcoded version (known to
 Alvaro> have fixed ruleutils) is used?

I honestly don't think it's worth it. cube() is not a function that
really makes any sense in a GROUP BY, though of course someone could
have written their own function called cube() that does something
else; while this case is a problem, it is also likely to be
vanishingly rare.

-- 
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] Proposal to add a QNX 6.5 port to PostgreSQL

[Andres Freund]

On 2014-08-22 10:41:55 -0400, Alvaro Herrera wrote:
> Andres Freund wrote:
> > Hi,
> > 
> > On 2014-08-21 15:25:44 +, Baker, Keith [OCDUS Non-J&J] wrote:
> > > About SA_RESTART:
> > > 
> > > I would like to offer you a different perspective which may alter your 
> > > current opinion.
> > > I believe the port.h QNX macro replacement for SA_RESTART is still a 
> > > reasonable solution on QNX for these reasons:
> > > 
> > > First, I think it is better to adapt PostgreSQL to suit the platform
> > > than to adapt the platform to suit PostgreSQL.
> > 
> > Well. That might be somewhat true for a popular platform. Which QNX
> > really isn't. I personally don't believe your approach to be likely to
> > end up with a correct and maintainable port.
> > 
> > > Changing default behavior of libc on QNX to suit PostgreSQL may break
> > > other applications which rely on the current behavior of libc.
> > 
> > I don't see how *adding* SA_RESTART support which would only be used
> > when SA_RESTART is being passed to sigaction(), would do that.
> 
> I guess the important question here is how much traction does Keith have
> with the QNX development group.

If you search for SA_RESTART and QNX there's a fair number of bugs
cropping up where it leads to problems... I think a large amount of open
source software essentially relies on it these days.

Note that it doesn't necessarily need to be implemented inside QNX. It
could very well be a wrapper library that you would optionally link
against. That'd benefit more users than just postgres on QNX.

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] potential bug in psql

[Szymon Guz]

On 22 August 2014 17:06, Tom Lane  wrote:

> Szymon Guz  writes:
> > when I run `\s` in psql, I get the nice list of queries with an error at
> > the end:
>
> > "\s
> > could not save history to file "/dev/tty": No such file or directory"
>
> Well, that's interesting ... what version of which readline library are
> you using?
>
>
Hi Tom,
that's libreadline6 from Ubuntu package 6.3-4ubuntu2


thanks,
Szymon

Re: [HACKERS] potential bug in psql

[Alvaro Herrera]

Tom Lane wrote:

> TBH, though, \s doesn't seem to me to be anywhere near worth the amount
> of work we've already put into it, let alone a major new implementation
> effort.  Who's for just removing the command altogether?

I use it every once in a while.  Is there a replacement?  There are
several things I hate about it, such as it filling up the terminal
scroll buffer with junk, so if there are workable alternatives I'm more
than willing to listen.

-- 
Álvaro Herrerahttp://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] Proposal to add a QNX 6.5 port to PostgreSQL

I am reaching out to our QNX support contacts today, I will let you know how 
they respond.

Keith Baker

> -Original Message-
> From: Alvaro Herrera [mailto:alvhe...@2ndquadrant.com]
> Sent: Friday, August 22, 2014 10:42 AM
> To: Andres Freund
> Cc: Baker, Keith [OCDUS Non-J&J]; Robert Haas; Tom Lane; pgsql-
> hack...@postgresql.org
> Subject: Re: [HACKERS] Proposal to add a QNX 6.5 port to PostgreSQL
> 
> Andres Freund wrote:
> > Hi,
> >
> > On 2014-08-21 15:25:44 +, Baker, Keith [OCDUS Non-J&J] wrote:
> > > About SA_RESTART:
> > > 
> > > I would like to offer you a different perspective which may alter your
> current opinion.
> > > I believe the port.h QNX macro replacement for SA_RESTART is still a
> reasonable solution on QNX for these reasons:
> > >
> > > First, I think it is better to adapt PostgreSQL to suit the platform
> > > than to adapt the platform to suit PostgreSQL.
> >
> > Well. That might be somewhat true for a popular platform. Which QNX
> > really isn't. I personally don't believe your approach to be likely to
> > end up with a correct and maintainable port.
> >
> > > Changing default behavior of libc on QNX to suit PostgreSQL may
> > > break other applications which rely on the current behavior of libc.
> >
> > I don't see how *adding* SA_RESTART support which would only be used
> > when SA_RESTART is being passed to sigaction(), would do that.
> 
> I guess the important question here is how much traction does Keith have
> with the QNX development group.
> 
> --
> Álvaro Herrerahttp://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] potential bug in psql

[Tom Lane]

Szymon Guz  writes:
> when I run `\s` in psql, I get the nice list of queries with an error at
> the end:

> "\s
> could not save history to file "/dev/tty": No such file or directory"

Well, that's interesting ... what version of which readline library are
you using?

Presumably what is happening is that write_history is writing out the data
okay (since you saw it) but then returning nonzero errno.  This is a bit
odd since we've not heard other complaints.  strace'ing psql while it does
this might be interesting.

We may have to re-revisit the question of how we detect errors in
write_history.  Some of the back story can be found here:
http://www.postgresql.org/message-id/flat/3b715ba0-cc81-4b3c-9174-5357129aa...@justatheory.com
and here:
http://www.postgresql.org/message-id/flat/ce92d7150608210927y44fb02bcw612dd75995e1d...@mail.gmail.com

Despite those efforts, I find that on OS X Mavericks, \s no longer works
at all with libedit:

regression=# \s
could not save history to file "/dev/tty": Operation not permitted

The reason becomes clear on comparing source for the two versions:
http://www.opensource.apple.com/source/libedit/libedit-3/libedit/history.c
http://www.opensource.apple.com/source/libedit/libedit-39/src/history.c

Somewhere in the last few releases, Apple "improved" history_save() so
that it not only does a fchmod() on the target file, but refuses to write
anything to the file if that fails.  I don't know if this was their own
idea or is something they got from upstream libedit development (though
most likely the latter).  It's unsurprising that an fchmod on /dev/tty
would fail --- you don't really own that file.

I wonder if we should just abandon the idea that \s to /dev/tty is
possible.  It's certainly never going to work anymore on libedit, and
I'd bet against getting the libedit developers to revert this change;
they probably think it's a security fix.

Another idea would be to reimplement \s as "do a normal history file save,
then cat the history file contents to /dev/tty".  This would have a couple
of benefits: we could interpose the pager program instead of just doing
"cat", and conceivably we could undo the encoding that libedit puts on the
file entries (though I'm not sure exactly how we'd figure out what it is).

TBH, though, \s doesn't seem to me to be anywhere near worth the amount
of work we've already put into it, let alone a major new implementation
effort.  Who's for just removing the command altogether?

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] delta relations in AFTER triggers

[Kevin Grittner]

Amit Khandekar  wrote:
> On 15 August 2014 04:04, Kevin Grittner  wrote:

>> The identifiers in the trigger function are not resolved to
>> particular objects until there is a request to fire the trigger.
> Ah ok, you are talking about changes specific to the PL language
> handlers. Yes, I agree that in the plpgsql parser (and in any PL
> handler), we need to parse such table references in the SQL construct,
> and transform it into something else.
>> At that time the parse analysis
>> needs to find the name defined somewhere.  It's not defined in the
>> catalogs like a table or view, and it's not defined in the query
>> itself like a CTE or VALUES clause.  The names specified in trigger
>> creation must be recognized as needing to resolve to the new
>> TuplestoreScan, and it needs to match those to the tuplestores
>> themselves.

For now I have added the capability to register named tuplestores
(with the associated TupleDesc) to SPI.  This seems like a pretty
useful place to be able to do this, although I tried to arrange for
it to be reasonably easy to add other registration mechanisms
later.

> One approach that comes to my mind is by transforming such transition
> table references into a RangeFunction reference while in plpgsql
> parser/lexer. This RangeFunction would point to a set-returning
> catalog function that would return rows from the delta tuplestore. So
> the tuplestore itself would remain a blackbox. Once we have such a
> function, any language handler can re-use the same interface.

plpgsql seems entirely the wrong place for that.  What I have done
is for trigger.c to add the tuplestores to the TriggerData
structure, and not worry about it beyond that.  I have provided a
way to register named tuplestores to SPI, and for the subsequent
steps to recognize those names as relation names.  The change to
plpgsql are minimal, since they only need to take the tuplestores
from TriggerData and register them with SPI before making the SPI
calls to plan and execute.

>> Row counts, costing, etc. needs to be provided so the
>> optimizer can pick a good plan in what might be a complex query
>> with many options.
> I am also not sure about the costing, but I guess it may be possible
> to supply some costs to the FunctionScan plan node.

I went with a bogus row estimate for now.  I think we can arrange 
for a tuplestore to keep a count of rows added, and a function to 
retrieve that, and thereby get a better estimate, but that is not 
yet done.

I think this is approaching a committable state, although I think 
it should probably be broken down to four separate patches.  Two of 
them are very small and easy to generate: the SPI changes and the 
plpgsql changes are in files that are distinct from all the other 
changes.  What remains is to separate the parsing of the CREATE 
TRIGGER statement and the trigger code that generates the 
tuplestores for the transition tables from the analysis, planning, 
and execution phases which deal with a more generic concept of 
named tuplestores.  Some of the changes for those two things both 
touch files related to parsing -- for different things, but in the 
same files.

To avoid polluting paring code with SPI and tuplestore includes, I 
created a very thin abstraction layer for parse analysis to use.  I 
didn't do the same for the executor yet, but it may be a good idea 
there, too.

It probably still needs more documentation and definitely needs 
more regression tests.  I have used these transition tables in 
plpgsql in simple tests, but there may be bugs still lurking.

New patch attached.

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

after-trigger-delta-relations-v3.patch.gz
Description: application/tgz

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

Re: [HACKERS] Support for N synchronous standby servers

[Michael Paquier]

On Fri, Aug 22, 2014 at 7:14 PM, Rajeev rastogi 
wrote:

> I have just started looking into this patch.
> Please find below my first level of observation from the patch:
>

Thanks! Updated patch attached.


> 1. Allocation of memory for sync_nodes in function
> SyncRepGetSynchronousNodes should be equivalent to allowed_sync_nodes
> instead of max_wal_senders. As anyway we are not going to store sync stdbys
> more   than allowed_sync_nodes.
> sync_nodes = (int *) palloc(allowed_sync_nodes *
> sizeof(int));
>

Fixed.

2. Logic of deciding the highest priority one seems to be in-correct.
> Assume, s_s_num = 3, s_s_names = 3,4,2,1
> standby nodes are in order as: 1,2,3,4,5,6,7
>
> As per the logic in patch, node 4 with priority 2 will not be
> added in the list whereas 1,2,3 will be added.
>
> The problem is because priority updated for next tracking is not
> the highest priority as of that iteration, it is just priority of last node
> added to the list. So it may happen that a node with   higher priority
> is still there in list but we are comparing with some other smaller
> priority.
>

Fixed. Nice catch!


> 3. Can we optimize the function SyncRepGetSynchronousNodes in such a way
> that it gets the number of standby nodes from s_s_names itself. With this
> it will be usful to stop scanning the moment we get first s_s_num potential
> standbys.
>

By doing so, we would need to scan the WAL sender array more than once (or
once if we can find N sync nodes with a name matching the first entry, smth
unlikely to happen). We would need as well to recalculate for a given item
in the list _names what is its priority and compare it with the existing
entries in the WAL sender list. So this is not worth the shot.
Also, using the priority instead of s_s_names is more solid as s_s_names is
now used only in SyncRepGetStandbyPriority to calculate the priority for a
given WAL sender, and is a function only called by a WAL sender itself when
it initializes.
Regards,
-- 
Michael
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***
*** 2586,2597  include_dir 'conf.d'
  Specifies a comma-separated list of standby names that can support
  synchronous replication, as described in
  .
! At any one time there will be at most one active synchronous standby;
! transactions waiting for commit will be allowed to proceed after
! this standby server confirms receipt of their data.
! The synchronous standby will be the first standby named in this list
! that is both currently connected and streaming data in real-time
! (as shown by a state of streaming in the
  
  pg_stat_replication view).
  Other standby servers appearing later in this list represent potential
--- 2586,2598 
  Specifies a comma-separated list of standby names that can support
  synchronous replication, as described in
  .
! At any one time there will be at a number of active synchronous standbys
! defined by , transactions
! waiting for commit will be allowed to proceed after those standby
! servers confirm receipt of their data. The synchronous standbys will be
! the first entries named in this list that are both currently connected
! and streaming data in real-time (as shown by a state of
! streaming in the
  
  pg_stat_replication view).
  Other standby servers appearing later in this list represent potential
***
*** 2627,2632  include_dir 'conf.d'
--- 2628,2685 

   
  
+  
+   synchronous_standby_num (integer)
+   
+synchronous_standby_num configuration parameter
+   
+   
+   
+
+ Specifies the number of standbys that support
+ synchronous replication.
+
+
+ Default value is -1. In this case, if
+  is empty all the
+ standby nodes are considered asynchronous. If there is at least
+ one node name defined, process will wait for one synchronous
+ standby listed.
+
+
+ When this parameter is set to 0, all the standby
+ nodes will be considered as asynchronous.
+
+
+This parameter value cannot be higher than
+ .
+
+
+ Are considered as synchronous the first elements of
+  in number of
+  that are
+ connected. If there are more elements than the number of stansbys
+ required, all the additional standbys are potential synchronous
+ candidates. If  is
+ empty, all the standbys are asynchronous. If it is set to the
+ special entry *, a number of standbys equal to
+  with the highest
+ pritority are elected as being synchronous.
+
+
+ Server will wait for commit confirmatio

Re: [HACKERS] Proposal to add a QNX 6.5 port to PostgreSQL

[Alvaro Herrera]

Andres Freund wrote:
> Hi,
> 
> On 2014-08-21 15:25:44 +, Baker, Keith [OCDUS Non-J&J] wrote:
> > About SA_RESTART:
> > 
> > I would like to offer you a different perspective which may alter your 
> > current opinion.
> > I believe the port.h QNX macro replacement for SA_RESTART is still a 
> > reasonable solution on QNX for these reasons:
> > 
> > First, I think it is better to adapt PostgreSQL to suit the platform
> > than to adapt the platform to suit PostgreSQL.
> 
> Well. That might be somewhat true for a popular platform. Which QNX
> really isn't. I personally don't believe your approach to be likely to
> end up with a correct and maintainable port.
> 
> > Changing default behavior of libc on QNX to suit PostgreSQL may break
> > other applications which rely on the current behavior of libc.
> 
> I don't see how *adding* SA_RESTART support which would only be used
> when SA_RESTART is being passed to sigaction(), would do that.

I guess the important question here is how much traction does Keith have
with the QNX development group.

-- 
Álvaro Herrerahttp://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] WIP Patch for GROUPING SETS phase 1

[Alvaro Herrera]

Andrew Gierth wrote:

> (This of course means that if someone has a cube() function call in
> a group by clause of a view, then upgrading will change the meaning
> of the view and possibly fail to create it; there seems to be no fix
> for this, not even using the latest pg_dump, since pg_dump relies on
> the old server's ruleutils)

This sucks.  Can we tweak pg_dump to check for presence of the cube
extension, and if found refuse to dump unless a minor version older than
some hardcoded version (known to have fixed ruleutils) is used?

-- 
Álvaro Herrerahttp://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] After switching primary server while using replication slot.

[Andres Freund]

Hi,

On 2014-08-20 13:14:30 -0400, Robert Haas wrote:
> On Tue, Aug 19, 2014 at 6:25 AM, Fujii Masao  wrote:
> > On Mon, Aug 18, 2014 at 11:16 PM, Sawada Masahiko  
> > wrote:
> >> Hi all,
> >> After switching primary serer while using repliaction slot, the
> >> standby server will not able to connect new primary server.
> >> Imagine this situation, if primary server has two ASYNC standby
> >> servers, also use each replication slots.
> >> And the one standby(A) apply WAL without problems. But another one
> >> standby(B) has stopped after connected to primary server.
> >> (or sending WAL is too delayed)
> >>
> >> In this situation, the standby(B) has not received WAL segment file
> >> while stopping itself.
> >> And the primary server can not remove WAL segments which has not been
> >> received to all standby.
> >> Therefore the primary server have to keep the WAL segment file which
> >> has not been received to all standby.
> >> But standby(A) can do checkpoint itself, and then it's possible to
> >> recycle WAL segments.
> >> The number of WAL segment of each server are different.
> >> ( The number of WAL files of standby(A) having smaller than primary 
> >> server.)
> >> After the primary server is crashed, the standby(A) promote to primary,
> >> we can try to connect standby(B) to standby(A) as new standby server.
> >> But it will be failed because the standby(A) server might not have WAL
> >> segment files that standby(B) required.
> >
> > This sounds valid concern.
> >
> >> To resolve this situation, I think that we should make master server
> >> to notify about removal of WAL segment to all standby servers.
> >> And the standby servers recycle WAL segments files base on that 
> >> information.

I think that'll end up being really horrible, at least if done in an
obligatory fashion. In a cascaded setup it's really sensible to only
retain WAL on the intermediate nodes. Consider e.g. a setup - rather
common these days actually - where there's a master somewhere and then a
cascading standby on each continent feeding off to further nodes on that
continent. You don't want to retain nodes on each continent (or on the
primary) just because one node somewhere is down for maintenance.


If you really want something like this we should probably add the
infrastructure for one standby to maintain a replication slot on another
standby server. So, if you have a setup like:

A
   / \
 /\
B  C
   / \ /\
.... ..  ..

B and C can coordinate that they keep enough WAL for each other. You can
actually easily write a external tool for that today. Just create a
replication slot oin B for C and the other way round and have a tool
update them once a minute or so.

I'm not sure if we want that builtin.

> >> Thought?
> >
> > How does the server recycle WAL files after it's promoted from the
> > standby to master?
> > It does that as it likes? If yes, your approach would not be enough.
> >
> > The approach prevents unexpected removal of WAL files while the standby
> > is running. But after the standby is promoted to master, it might recycle
> > needed WAL files immediately. So another standby may still fail to retrieve
> > the required WAL file after the promotion.
> >
> > ISTM that, in order to address this, we might need to log all the 
> > replication
> > slot activities and replicate them to the standby. I'm not sure if this
> > breaks the design of replication slot at all, though.

Yes, that'd break it. You can't WAL log anything on a standby, and
replication slots can be modified on standbys.

> I believe that the reason why replication slots are not currently
> replicated is because we had the idea that the standby could have
> slots that don't exist on the master, for cascading replication.  I'm
> not sure that works yet, but I think Andres definitely had it in mind
> in the original design.

That works. And it's absolutely required for adding logical decoding on
standbys (I've a prototype patch for it...).

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] Proposal to add a QNX 6.5 port to PostgreSQL

[Andres Freund]

Hi,

On 2014-08-21 15:25:44 +, Baker, Keith [OCDUS Non-J&J] wrote:
> About SA_RESTART:
> 
> I would like to offer you a different perspective which may alter your 
> current opinion.
> I believe the port.h QNX macro replacement for SA_RESTART is still a 
> reasonable solution on QNX for these reasons:
> 
> First, I think it is better to adapt PostgreSQL to suit the platform
> than to adapt the platform to suit PostgreSQL.

Well. That might be somewhat true for a popular platform. Which QNX
really isn't. I personally don't believe your approach to be likely to
end up with a correct and maintainable port.

> Changing default behavior of libc on QNX to suit PostgreSQL may break
> other applications which rely on the current behavior of libc.

I don't see how *adding* SA_RESTART support which would only be used
when SA_RESTART is being passed to sigaction(), would do that.

> Yes, I could forget to add a port.h macro for a given interruptible
> primitive, but I could likewise forget to update the wrapper for that
> call in a custom libc.

> I requested that QNX support provide me a list of interruptible
> primitives, but I was able to identify many by searching through the
> QNX help.  Definition of a new interruptible primitive is a rare
> event, so once a solid list of macros is in place for QNX, it should
> need very little maintenance.  If you have any specific calls you
> believe are missing from my list of macros, I would be happy to add
> them.

I have no idea whether there are any other ones - I don't have access to
a QNX machine, and I don't personally wan't any. The problem is that we
might want to start using new syscalls or QNX might introduce new
interruptible signals. Problems caused by missed interruptible syscalls
won't show during low-load testing like pg_regress. They'll show up
during production usage.

> port.h is included in c.h, which is in postgres.h, so the QNX macros
> should be effective for all QNX PostgreSQL compiles.  If it were not,
> no one could reply on any port.h features on any platform.

Yea, that's not a concern I have.

> The first release on any platform has risk of defects, which can be
> corrected once identified.  I would expect that a first release on any
> platform would include a warning or disclaimer stating that it is new
> port.
> 
> Lastly, the QNX-specific section added to port.h appears to solve the
> SA_RESTART issue for QNX, while having no impact on compiles of
> existing platforms.

My problem is that it's ugly hack for a niche paltform that will need to
be maintained for a long while into the future. I don't have a problem
adding support for not that frequently used platforms if the support is
very localized, but that's definitely not the case here.

> About configure:
> 
> "./configure" barked at 2 things on QNX, and it advised using both
> "--without-readline --disable-thread-safety".  I can investigate
> further, but I have been focusing on the bigger issues first.

Yea, those aren't really critical. It'd be interesting to know why the
the thread safety test fails - quite possibly it's just the configure
test for pthreads not being very good.

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] Is this a bug?

[Robert Haas]

On Thu, Aug 21, 2014 at 7:17 PM, Bruce Momjian  wrote:
> On Tue, Mar 18, 2014 at 09:11:46AM -0400, Robert Haas wrote:
>> On Mon, Mar 17, 2014 at 10:27 PM, Michael Paquier
>>  wrote:
>> > On Tue, Mar 18, 2014 at 10:24 AM, Fabrízio de Royes Mello
>> >  wrote:
>> >>
>> >> On Thu, Mar 13, 2014 at 10:22 AM, Robert Haas  
>> >> wrote:
>> >>> Well, it's fairly harmless, but it might not be a bad idea to tighten 
>> >>> that
>> >>> up.
>> >> The attached patch tighten that up.
>> > Hm... It might be interesting to include it in 9.4 IMO, somewhat
>> > grouping with what has been done in a6542a4 for SET and ABORT.
>>
>> Meh.  There will always be another thing we could squeeze in; I don't
>> think this is particularly urgent, and it's late to the party.
>
> Do we want this patch for 9.5?  It throws an error for invalid reloption
> specifications.

Fine with me.  But I have a vague recollection of seeing pg_upgrade
doing this on purpose to create TOAST tables or something... am I
misremembering?

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


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

Re: [HACKERS] B-Tree support function number 3 (strxfrm() optimization)

[Robert Haas]

On Thu, Aug 14, 2014 at 1:24 PM, Peter Geoghegan  wrote:
> On Thu, Aug 14, 2014 at 9:13 AM, Robert Haas  wrote:
>> Committed that way.  As the patch is by and large the same as what I
>> submitted for this originally, I credited myself as first author and
>> you as second author.  I hope that seems fair.
>
> I think that's more than fair. Thanks!

Patch 0002 no longer applies; please rebase.

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


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

Re: [HACKERS] pg_upgrade: allow multiple -o/-O options

[Bruce Momjian]

On Fri, Aug 22, 2014 at 10:52:12AM +0200, Pavel Raiskup wrote:
> On Thursday 21 of August 2014 18:26:37 Bruce Momjian wrote:
> > On Tue, Mar  4, 2014 at 04:52:56PM +0100, Pavel Raiskup wrote:
> > > RFE:  Consider that you want to run pg_upgrade via some script with some
> > > default '-o' option.  But then you also want to give the script's user a
> > > chance to specify the old-server's options according user's needs.
> > > Then something like the following is not possible:
> > > 
> > >   $ cat script
> > >   ...
> > >   pg_upgrade ... -o 'sth' $PG_UPGRADE_OPT ...
> > >   ...
> > > 
> > > I know that this problem is still script-able, but the fix should be
> > > innocent and it would simplify things.  Thanks for considering,
> > 
> > Attached is a patch that makes multiple -o options append their
> > arguments for pg_upgrade and pg_ctl, and documents this and the append
> > behavior of postmaster/postgres.  This covers all the -o behaviors.
> 
> Thanks!  Seems to be OK to me, one nit  - why you did not go the
> append_optiton way (there could be probably better name like arg_cat)?
> Because this is just about few lines, it is probably OK from PostgreSQL
> policy POV, so "review?  ~> review+", thanks again!

Well, I found append_optiton() to be an extra function that wasn't
necessary --- the psprintf() use was short enough not to need a separate
function.

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

  + Everyone has their own god. +


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

Re: [HACKERS] Proposal to add a QNX 6.5 port to PostgreSQL

[Noah Misch]

On Fri, Aug 22, 2014 at 09:34:42AM +0200, Andres Freund wrote:
> On 2014-08-22 01:36:37 -0400, Noah Misch wrote:
> > On Thu, Aug 21, 2014 at 01:33:38AM +0200, Andres Freund wrote:
> > > On 2014-07-25 18:29:53 -0400, Tom Lane wrote:
> > > > > * QNX lacks sigaction SA_RESTART: I modified 
> > > > > "src/include/port.h" to define macros to retry system calls upon 
> > > > > EINTR (open,read,write,...) when compiled on QNX
> > > > 
> > > > That's pretty scary too.  For one thing, such macros would affect every
> > > > call site whether it's running with SA_RESTART or not.  Do you really
> > > > need it?  It looks to me like we just turn off HAVE_POSIX_SIGNALS if
> > > > you don't have SA_RESTART.  Maybe that code has bit-rotted by now, but
> > > > it did work at one time.
> > > 
> > > I have pretty much no trust that we're maintaining
> > > !HAVE_POSIX_SIGNAL. And none that we have that capability of doing so. I
> > > seriously doubt there's any !HAVE_POSIX_SIGNAL animals and
> > > 873ab97219caabeb2f7b390268a4fe01e2b7518c makes it pretty darn unlikely
> > > that we have much chance of finding such mistakes during development.
> > 
> > I bet it's fine for its intended target, namely BSD-style signal() in which
> > SA_RESTART-like behavior is implicit.  See the src/port/pqsignal.c header
> > comment.  PostgreSQL has no support for V7-style/QNX-style signal().
> 
> That might be true - although I'm not sure it actually still works - but
> my point is that I can't see Tom's suggestion on relying on
> !HAVE_POSIX_SIGNALS for QNX work out.

True.


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

Re: [HACKERS] WIP Patch for GROUPING SETS phase 1

[Stephen Frost]

* Andrew Gierth (and...@tao11.riddles.org.uk) wrote:
> Having now spent some more time looking, I believe there is a solution
> which makes it unreserved which does not require any significant pain
> in the code.  I'm not entirely convinced that this is the right
> approach in the long term, but it might allow for a more planned
> transition.
> 
> The absolute minimum seems to be:
> 
>  GROUPING as a col_name_keyword (since GROUPING(x,y,...) in the select
>  list as a  looks like a function call for any
>  argument types)
> 
>  CUBE, ROLLUP, SETS as unreserved_keyword

This means

GROUP BY cube(x,y)
GROUP BY (cube(x,y))
GROUP BY cube(x)

all end up with different meanings though, right?

I'm not sure that's really a better situation.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] add line number as prompt option to psql

[Jeevan Chalke]

> I would like to ignore this as UINTMAX lines are too much for a input
> > buffer to hold. It is almost NIL chances to hit this.
>
> Yeah, most likely you will run out of memory before reaching that point,
> or out of patience.
>
> Yep.

BTW, I have marked this as "waiting for committer".

Thanks
-- 
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Re: [HACKERS] option -T in pg_basebackup doesn't work on windows

[Heikki Linnakangas]

On 08/22/2014 11:35 AM, Amit Kapila wrote:

On Fri, Aug 22, 2014 at 1:00 PM, Heikki Linnakangas 
wrote:

On 08/22/2014 07:08 AM, Amit Kapila wrote:

Today morning, I realised that there is one problem with the
patch I sent yesterday and the problem is that incase user
has not given -T option, it will not be able to create the symlink
for appropriate path.  Attached patch fix this issue.



Thanks, committed with minor changes:


Thank you.  One minor point I observed is that in comments below,
you have used --tablespace as option name rather than
--tablespace-mapping, is it intentional?

+* the location of a tablespace. Apply any tablespace
+* mapping given on the command line (--tablespace).


Sorry, my mistake. Fixed.

- Heikki



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

Re: [HACKERS] Re: proposal: ignore null fields in not relation type composite type based constructors

[Jeevan Chalke]

Hi Pavel,

You have said that XMLFOREST has something which ignores nulls, what's that?
Will you please provide an example ?

I am NOT sure, but here you are trying to omit entire field from the output
when its value is NULL. But that will add an extra efforts at other end
which is using output of this. That application need to know all fields and
then need to replace NULL values for missing fields. However we have an
optional argument for ignoring nulls and thus we are safe. Application will
use as per its choice.

Well, apart from that, tried reviewing the patch. Patch was applied but make
failed with following error.

make[3]: Entering directory `/home/jeevan/pg_master/src/backend/catalog'
cd ../../../src/include/catalog && '/usr/bin/perl' ./duplicate_oids
3255
make[3]: *** [postgres.bki] Error 1

Please run unused_oids script to find unused oid.

However, I had a quick look over code changes. At first glance it looks
good. But still need to check on SQL level and then code walk-through.

Waiting for updated patch.

Thanks

-- 
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Re: [HACKERS] Support for N synchronous standby servers

[Rajeev rastogi]

On 09 August 2014 11:33, Michael Paquier Wrote:

> Please find attached a patch to add support of synchronous replication
> for multiple standby servers. This is controlled by the addition of a
> new GUC parameter called synchronous_standby_num, that makes server
> wait for transaction commit on the first N standbys defined in
> synchronous_standby_names. The implementation is really straight-
> forward, and has just needed a couple of modifications in walsender.c
> for pg_stat_get_wal_senders and syncrep.c.

I have just started looking into this patch. 
Please find below my first level of observation from the patch:

1. Allocation of memory for sync_nodes in function SyncRepGetSynchronousNodes 
should be equivalent to allowed_sync_nodes instead of max_wal_senders. As 
anyway we are not going to store sync stdbys more   than allowed_sync_nodes.
sync_nodes = (int *) palloc(allowed_sync_nodes * sizeof(int));

2. Logic of deciding the highest priority one seems to be in-correct.
Assume, s_s_num = 3, s_s_names = 3,4,2,1 
standby nodes are in order as: 1,2,3,4,5,6,7

As per the logic in patch, node 4 with priority 2 will not be added in 
the list whereas 1,2,3 will be added.

The problem is because priority updated for next tracking is not the 
highest priority as of that iteration, it is just priority of last node added 
to the list. So it may happen that a node with   higher priority is still 
there in list but we are comparing with some other smaller priority. 

3. Can we optimize the function SyncRepGetSynchronousNodes in such a way that 
it gets the number of standby nodes from s_s_names itself. With this it will be 
usful to stop scanning the moment we get first s_s_num potential standbys.

Thanks and Regards,
Kumar Rajeev Rastogi



-- 
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] inherit support for foreign tables

[Etsuro Fujita]

(2014/08/22 12:58), Alvaro Herrera wrote:

Noah Misch wrote:


I'm anticipating a bug report along these lines:

   I saw poor estimates involving a child foreign table, so I ran "ANALYZE
   VERBOSE", which reported 'INFO:  analyzing "public.parent" inheritance
   tree'.  Estimates remained poor, so I scratched my head for awhile before
   noticing that pg_stats didn't report such statistics.  I then ran "ANALYZE
   VERBOSE parent", saw the same 'INFO:  analyzing "public.parent" inheritance
   tree', but this time saw relevant rows in pg_stats.  The message doesn't
   reflect the actual behavior.

I'll sympathize with that complaint.


Agreed on that.


I've got the point.  Will fix.

Thanks for the comment!

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] Are postgresql-9.4 binaries available on Windows XP?

[Dave Page]

Hi

On Fri, Aug 22, 2014 at 4:45 AM, Hiroshi Inoue  wrote:
> Hi Dave and Andrew,
>
> I recently noticed the thread
>   [BUGS] BUG #11039: installation fails when trying to install C++
> redistributable .
>
> Unfortunately I have no XP machine at hand and can't test the
> installer by myself.
>
> Looking at the binaries in the package, they seem to be built using
> Visual Studio 2013. I'm suspicious if the binaries are available on
> Windows XP.
>
> If I recognize correctly, Visual Studio 2012 or later doesn't support
> Windows XP by default and Platform Toolset v120_xp (or v110_xp) must
> be specified so as to build binaries guaranteed to be avaiable on
> Windows XP. However MSBuildProject.pm seems to specify v120 (or v110) as
> the PlarformToolset property. Is it intentional?

I can't say whether that was an intentional change or not as I wasn't
involved in writing that patch, but I can say that EDB will not be
supporting Windows XP for our future builds now that it's been
(finally) made obsolete by Microsoft when extended support ended in
April.

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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] pg_upgrade: allow multiple -o/-O options

[Pavel Raiskup]

On Thursday 21 of August 2014 18:26:37 Bruce Momjian wrote:
> On Tue, Mar  4, 2014 at 04:52:56PM +0100, Pavel Raiskup wrote:
> > RFE:  Consider that you want to run pg_upgrade via some script with some
> > default '-o' option.  But then you also want to give the script's user a
> > chance to specify the old-server's options according user's needs.
> > Then something like the following is not possible:
> > 
> >   $ cat script
> >   ...
> >   pg_upgrade ... -o 'sth' $PG_UPGRADE_OPT ...
> >   ...
> > 
> > I know that this problem is still script-able, but the fix should be
> > innocent and it would simplify things.  Thanks for considering,
> 
> Attached is a patch that makes multiple -o options append their
> arguments for pg_upgrade and pg_ctl, and documents this and the append
> behavior of postmaster/postgres.  This covers all the -o behaviors.

Thanks!  Seems to be OK to me, one nit  - why you did not go the
append_optiton way (there could be probably better name like arg_cat)?
Because this is just about few lines, it is probably OK from PostgreSQL
policy POV, so "review?  ~> review+", thanks again!

Pavel



-- 
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] option -T in pg_basebackup doesn't work on windows

[Amit Kapila]

On Fri, Aug 22, 2014 at 1:00 PM, Heikki Linnakangas 
wrote:
> On 08/22/2014 07:08 AM, Amit Kapila wrote:
>> Today morning, I realised that there is one problem with the
>> patch I sent yesterday and the problem is that incase user
>> has not given -T option, it will not be able to create the symlink
>> for appropriate path.  Attached patch fix this issue.
>
>
> Thanks, committed with minor changes:

Thank you.  One minor point I observed is that in comments below,
you have used --tablespace as option name rather than
--tablespace-mapping, is it intentional?

+* the location of a tablespace. Apply any tablespace
+* mapping given on the command line (--tablespace).




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

Re: [HACKERS] Removing dependency to wsock32.lib when compiling code on WIndows

[Michael Paquier]

On Fri, Aug 15, 2014 at 8:00 PM, Noah Misch  wrote:
>
> On Fri, Aug 15, 2014 at 12:49:36AM -0700, Michael Paquier wrote:
> > Btw, how do you determine if MSVC is using HAVE_GETADDRINFO? Is it
> > decided by the inclusion of getaddrinfo.c in @pgportfiles of
> > Mkvdbuild.pm?
>
> src/include/pg_config.h.win32 dictates it, and we must keep @pgportfiles
> synchronized with the former's verdict.
Thanks.

Looking more into that, I am seeing that MinGW-32 is failing to find socket
at configure, contrary to MinGW-64.

Here is what happens for MinGW-64 at configure:
configure:7638: checking for library containing socket
[...]
configure:7669: x86_64-w64-mingw32-gcc -o conftest.exe  -Wall
-Wmissing-prototypes -Wpointer-arith \
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
-Wformat-security -fno-stri\
ct-aliasing -fwrapv -fexcess-precision=standard -g
 -I./src/include/port/win32 -DEXEC_BACKEND   -Wl\
,--allow-multiple-definition -Wl,--disable-auto-import  conftest.c -lws2_32
 -lm  >&5
configure:7669: $? = 0
configure:7686: result: -lws2_32

And for MinGW-32:
configure:7638: checking for library containing socket
[...]
configure:7669: gcc -o conftest.exe  -Wall -Wmissing-prototypes
-Wpointer-arith -Wdeclaration-after\
-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security
-fno-strict-aliasing -fwrapv\
 -fexcess-precision=standard -g  -I./src/include/port/win32
-DEXEC_BACKEND   -Wl,--allow-multiple-d\
efinition -Wl,--disable-auto-import  conftest.c -lws2_32  -lm  >&5
C:\Users\ioltas\AppData\Local\Temp\cciNV1Y8.o: In function `main':
c:\Users\ioltas\git\postgres/conftest.c:33: undefined reference to `socket'
collect2.exe: error: ld returned 1 exit status
configure:7669: $? = 1
[...]
configure:7686: result: no

Now, what happens is that if configure finds socket within ws2_32 it adds
it to LIBS, making this variable look like that:
- MinGW-32: LIBS="-lm "
- MinGW-64: LIBS="-lm -lws2_32"
And as LIBS is used afterwards to check the presence of several functions,
including getaddrinfo, those different values of LIBS make HAVE_GETADDRINFO
set to different values in MinGW-32 and 64, respectively undefined and
defined.

I am not sure which way is better, do we want HAVE_GETADDRINFO or
!HAVE_GETADDRINFO in all Windows builds? If we choose the former, we'll
need to understand why -lws2_32 is not added to LIBS for MinGW-32. If we
choose the latter, we would need to remove -lws2_32 from LIBS with MinGW-64
for consistency with MinGW-32 I think.

Either way, currently MSVC uses !HAVE_GETADDRINFO. So in bonus to this
anamysis, the patch attached makes possible the use of HAVE_GETADDRINFO.
This could be needed for the final solution we seek.

Note that the undef gai_strerror in src/include/port/win32/sys/socket.h has
been added in 2005 by this commit:
commit: a310a1d80c6535774115838010f9c337d08d45cc
author: Tom Lane 
date: Fri, 26 Aug 2005 03:15:12 +
Some more mop-up for Windows IPv6 support.  Andrew Dunstan

I don't know the opinions of the others, but removing a tweak like this one
may not be a bad thing to simplify code.

Regards,
-- 
Michael
diff --git a/src/include/pg_config.h.win32 b/src/include/pg_config.h.win32
index bce67a2..903246d 100644
--- a/src/include/pg_config.h.win32
+++ b/src/include/pg_config.h.win32
@@ -143,7 +143,7 @@
 #define HAVE_FUNCNAME__FUNCTION 1
 
 /* Define to 1 if you have getaddrinfo(). */
-/* #undef HAVE_GETADDRINFO */
+#define HAVE_GETADDRINFO 1
 
 /* Define to 1 if you have the `gethostbyname_r' function. */
 /* #undef HAVE_GETHOSTBYNAME_R */
diff --git a/src/include/port/win32/sys/socket.h b/src/include/port/win32/sys/socket.h
index edaee6a..4bd08f0 100644
--- a/src/include/port/win32/sys/socket.h
+++ b/src/include/port/win32/sys/socket.h
@@ -23,11 +23,4 @@
 #define ERROR PGERROR
 #endif
 
-/*
- * we can't use the windows gai_strerror{AW} functions because
- * they are defined inline in the MS header files. So we'll use our
- * own
- */
-#undef gai_strerror
-
 #endif   /* WIN32_SYS_SOCKET_H */
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 004942c..177879e 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -66,7 +66,7 @@ sub mkvcbuild
 
 	our @pgportfiles = qw(
 	  chklocale.c crypt.c fls.c fseeko.c getrusage.c inet_aton.c random.c
-	  srandom.c getaddrinfo.c gettimeofday.c inet_net_ntop.c kill.c open.c
+	  srandom.c gettimeofday.c inet_net_ntop.c kill.c open.c
 	  erand48.c snprintf.c strlcat.c strlcpy.c dirmod.c noblock.c path.c
 	  pgcheckdir.c pg_crc.c pgmkdirp.c pgsleep.c pgstrcasecmp.c pqsignal.c
 	  mkdtemp.c qsort.c qsort_arg.c quotes.c system.c

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

Re: [HACKERS] Proposal to add a QNX 6.5 port to PostgreSQL

[Andres Freund]

On 2014-08-22 01:36:37 -0400, Noah Misch wrote:
> On Thu, Aug 21, 2014 at 01:33:38AM +0200, Andres Freund wrote:
> > On 2014-07-25 18:29:53 -0400, Tom Lane wrote:
> > > > * QNX lacks sigaction SA_RESTART: I modified 
> > > > "src/include/port.h" to define macros to retry system calls upon EINTR 
> > > > (open,read,write,...) when compiled on QNX
> > > 
> > > That's pretty scary too.  For one thing, such macros would affect every
> > > call site whether it's running with SA_RESTART or not.  Do you really
> > > need it?  It looks to me like we just turn off HAVE_POSIX_SIGNALS if
> > > you don't have SA_RESTART.  Maybe that code has bit-rotted by now, but
> > > it did work at one time.
> > 
> > I have pretty much no trust that we're maintaining
> > !HAVE_POSIX_SIGNAL. And none that we have that capability of doing so. I
> > seriously doubt there's any !HAVE_POSIX_SIGNAL animals and
> > 873ab97219caabeb2f7b390268a4fe01e2b7518c makes it pretty darn unlikely
> > that we have much chance of finding such mistakes during development.
> 
> I bet it's fine for its intended target, namely BSD-style signal() in which
> SA_RESTART-like behavior is implicit.  See the src/port/pqsignal.c header
> comment.  PostgreSQL has no support for V7-style/QNX-style signal().

That might be true - although I'm not sure it actually still works - but
my point is that I can't see Tom's suggestion on relying on
!HAVE_POSIX_SIGNALS for QNX work out.

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

[HACKERS] potential bug in psql

[Szymon Guz]

Hi,
when I run `\s` in psql, I get the nice list of queries with an error at
the end:

"\s
could not save history to file "/dev/tty": No such file or directory"


Newest ubuntu from trunk

 PostgreSQL 9.5devel on x86_64-unknown-linux-gnu, compiled by Ubuntu clang
version 3.4-1ubuntu3 (tags/RELEASE_34/final) (based on LLVM 3.4), 64-bit

and zsh as shell

thanks,
Szymon

Re: [HACKERS] option -T in pg_basebackup doesn't work on windows

[Heikki Linnakangas]

On 08/22/2014 07:08 AM, Amit Kapila wrote:

On Thu, Aug 21, 2014 at 3:44 PM, Amit Kapila 
wrote:


On Tue, Aug 19, 2014 at 9:51 AM, Amit Kapila 

wrote:

On Mon, Aug 18, 2014 at 7:50 PM, Heikki Linnakangas <

hlinnakan...@vmware.com> wrote:

Wouldn't it make a lot more sense to create it correctly in the first

place?


Looking at the code, I think it is very well possible to create
it correctly in the first place without much extra work.  I will
send a patch if nobody sees any problem with this change.


Attached patch implements the above suggested fix.
I have removed the earlier code which was used to update the
symlink path.


Today morning, I realised that there is one problem with the
patch I sent yesterday and the problem is that incase user
has not given -T option, it will not be able to create the symlink
for appropriate path.  Attached patch fix this issue.


Thanks, committed with minor changes:

* fixed the error message to print the mapped path that it actually 
tried to create, instead of the original.

* there's no need to copy the mapped path string, so I just used a pointer
* made the code to do the basetablespace mapping in top of the function 
a little bit tidier (IMHO anyway), although it wasn't really this patch's.
* I noticed that the mappings now apply to any symlinks in the data 
directory. I think that's OK, we don't expect there to be any other 
symlinks, especially not pointing to a tablespace location, but if there 
are, it's arguably a good thing that they are mapped too.


- Heikki


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

Re: [HACKERS] WIP Patch for GROUPING SETS phase 1

[Andrew Gierth]

> "Tom" == Tom Lane  writes:

 Tom> Perhaps so.  I would really prefer not to have to get into
 Tom> estimating how many people will be inconvenienced how badly.
 Tom> It's clear to me that not a lot of sweat has been put into
 Tom> seeing if we can avoid reserving the keyword, and I think we
 Tom> need to put in that effort.

So, first nontrivial issue that crops up is this: if CUBE is
unreserved, then ruleutils will output the string "cube(a,b)" for a
function call to a function named "cube", on the assumption that it
will parse back as a single unit (which inside a GROUP BY is not
true).

Options:

1) when outputting GROUP BY clauses (and nothing else), put parens
around anything that isn't provably atomic; or put parens around
anything that might look like a function call; or put parens around
anything that looks like a function call with a keyword name

2) when outputting any function call, add parens if the name is an
unreserved keyword

3) when outputting any function call, quote the name if it is an
unreserved keyword

4) something else?

(This of course means that if someone has a cube() function call in
a group by clause of a view, then upgrading will change the meaning
of the view and possibly fail to create it; there seems to be no fix
for this, not even using the latest pg_dump, since pg_dump relies on
the old server's ruleutils)

-- 
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] pgcrypto: PGP signatures

[Marko Tiikkaja]

On 8/22/14, 2:57 AM, Thomas Munro wrote:

I took a quick look at your patch at
http://www.postgresql.org/message-id/53edbcf0.9070...@joh.to (sorry I
didn't reply directly as I didn't have the message).  It applies
cleanly, builds, and the tests pass.  I will hopefully have more to
say after I've poked at it and understood it better, but in the
meantime a couple of trivial things I spotted:


Thanks!


In pgp-pgsql.c, function pgp_sym_decrypt_verify_bytea:

I think the first 'arg' should be 'psw'.

I think the same mistake appears in pgp_sym_decrypt_verify_text.


Yeah, these look like copypaste-os.  Will fix.


Should t be of type pg_time_t rather than time_t?  Would it be better
if PGP_Signature's member creation_time were of type uint32_t and you
could use ntohl(sig->creation_time) instead of the bitshifting?


I tried to make the code look like the existing code in 
init_litdata_packet().  I don't oppose to writing it this way, but I 
think we should change both instances if so (or perhaps move the code 
into a new function).



In pgp.h:

+#define PGP_MAX_KEY(256/8)
+#define PGP_MAX_BLOCK  (256/8)
+#define PGP_MAX_DIGEST (512/8)
+#define PGP_MAX_DIGEST_ASN1_PREFIX 20
+#define PGP_S2K_SALT   8

The PGP_MAX_DIGEST_ASN1_PREFIX line is new but the others just have
whitespace changes, and I gather the done thing is not to reformat
existing lines like that to distract from the changes in a patch.


Right.  Sorry about that.  I can revert the whitespace fixes.


(Just curious, why do you use while (1) for an infinite loop in
extract_signatures, but for (;;) in pullf_discard?  It looks like the
latter is much more common in the source tree.)


In the postgres source tree?  Yeah.  But while(1) is all over pgcrypto, 
so I've tried to make the new code match that.  If there are any 
instances of "for (;;)" in the patch, those must be because of me typing 
without thinking.  I could search-and-replace those to "while (1)" to 
make it consistent.



.marko


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