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

[Noah Misch]

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


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

Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

[Ashutosh Bapat]

On Thu, Aug 21, 2014 at 3:00 PM, Etsuro Fujita 
wrote:

> (2014/08/21 13:21), Ashutosh Bapat wrote:
>
>> On Wed, Aug 20, 2014 at 3:25 PM, Etsuro Fujita
>> mailto:fujita.ets...@lab.ntt.co.jp>> wrote:
>>
>
>  Hi Ashutish,
>>
>
> I am sorry that I mistook your name's spelling.
>
>  (2014/08/14 22:30), Ashutosh Bapat wrote:
>>
>> On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita
>> > 
>> >
>> >> wrote:
>>
>>  (2014/08/08 18:51), Etsuro Fujita wrote:
>>   >>> (2014/06/30 22:48), Tom Lane wrote:
>>    I wonder whether it isn't time to change that.  It
>> was coded
>>  like that
>>    originally only because calculating the values
>> would've been a
>>  waste of
>>    cycles at the time.  But this is at least the third
>> place
>>  where it'd be
>>    useful to have attr_needed for child rels.
>>
>
>   There was a problem with the previous patch, which will be
>> described
>>  below.  Attached is the updated version of the patch
>> addressing that.
>>
>
>  Here are some more comments
>>
>
>  attr_needed also has the attributes used in the restriction
>> clauses as
>> seen in distribute_qual_to_rels(), so, it looks unnecessary to
>> call
>> pull_varattnos() on the clauses in baserestrictinfo in functions
>> check_selective_binary___conversion(),
>> remove_unused_subquery___outputs(),
>> check_index_only().
>>
>
>  IIUC, I think it's *necessary* to do that in those functions since
>> the attributes used in the restriction clauses in baserestrictinfo
>> are not added to attr_needed due the following code in
>> distribute_qual_to_rels.
>>
>
>  That's right. Thanks for pointing that out.
>>
>
>  Although in case of RTE_RELATION, the 0th entry in attr_needed
>> corresponds to FirstLowInvalidHeapAttributeNu__mber + 1, it's
>>
>> always safer
>> to use it is RelOptInfo::min_attr, in case get_relation_info()
>> wants to
>> change assumption or somewhere down the line some other part of
>> code
>> wants to change attr_needed information. It may be unlikely, that
>> it
>> would change, but it will be better to stick to comments in
>> RelOptInfo
>>443 AttrNumber  min_attr;   /* smallest attrno of rel
>> (often
>> <0) */
>>444 AttrNumber  max_attr;   /* largest attrno of rel */
>>445 Relids *attr_needed;/* array indexed [min_attr
>> ..
>> max_attr] */
>>
>
>  Good point!  Attached is the revised version of the patch.
>>
>
>  If the patch is not in the commit-fest, can you please add it there?
>>
>
> I've already done that:
>
> https://commitfest.postgresql.org/action/patch_view?id=1529
>
>
>   From my side, the review is done, it should be marked "ready for
>> committer", unless somebody else wants to review.
>>
>
> Many thanks!
>
>
Thanks. Since, I haven't seen anybody else commenting here and I do not
have any further comments to make, I have marked it as "ready for
committer".


> Best regards,
> Etsuro Fujita
>



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

Re: [HACKERS] pg_receivexlog --status-interval add fsync feedback

[furuyao]

> Thank you for updating the patch.
> I reviewed the patch.
> 
> First of all, I think that we should not append the above message to
> section of '-r' option.
> (Or these message might not be needed at all) Whether flush location in
> feedback message is valid,  is not depend on '-r' option.
> 
> If we use '-r' option and 'S' option (i.g., replication slot) then
> pg_receivexlog informs valid flush location to primary server at the same
> time as doing fsync.
> But,  if we don't specify replication slot then the flush location in
> feedback message always invalid.
> So I think Fujii-san pointed out that sending of invalid flush location
> is not needed if pg_receivexlog does not use replication slot.

Thanks for the review!

I understand the attention message wasn't appropriate.

To report the write location, even If you do not specify a replication slot.
So the fix only appended messages.

There was a description of the flush location section of '-S' option, 
but I intended to catch eye more and added a message.

Is it better to make specification of the -S option indispensable?

Regards,

--
Furuya Osamu

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


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


pg_basebackup_relocate_tablespace_v4.patch
Description: Binary data

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

Re: [HACKERS] inherit support for foreign tables

[Alvaro Herrera]

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.

> A credible alternative is to emit a second message indicating that we
> skipped the inheritance tree statistics after all, and why we skipped
> them.

That'd be similar to Xorg emitting messages such as

[53.772] (II) intel(0): RandR 1.2 enabled, ignore the following RandR 
disabled message.
[53.800] (--) RandR disabled

I find this very poor.

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

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!

Fabrízio Mello




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

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

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

[HACKERS] Are postgresql-9.4 binaries available on Windows XP?

[Hiroshi Inoue]

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?

regards,
Hiroshi Inoue


-- 
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] Removing dependency to wsock32.lib when compiling code on WIndows

[Noah Misch]

On Thu, Aug 21, 2014 at 09:18:22AM -0400, Andrew Dunstan wrote:
> 
> On 08/15/2014 11: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.
> >
> 
> 
> What's happening about this? Buildfarm animal jacana is consistently
> red because of this.

If nobody plans to do the aforementioned analysis in the next 4-7 days, I
suggest we adopt one of Michael's suggestions: force "configure" to reach its
old conclusion about getaddrinfo() on Windows.  Then the analysis can proceed
on an unhurried schedule.


-- 
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 Thu, Aug 21, 2014 at 10:26 PM, Fabrízio de Royes Mello <
fabriziome...@gmail.com> wrote:
>
>
> On Thu, Aug 21, 2014 at 8:04 PM, Alvaro Herrera 
wrote:
> > Andres Freund wrote:
> >
> > > Have you looked at the correctness of the patch itself? Last time I'd
> > > looked it has fundamental correctness issues. I'd outlined a possible
> > > solution, but I haven't looked since.
> >
> > Yeah, Fabrizio had it passing the relpersistence down to make_new_heap,
> > so the transient table is created with the right setting.  AFAICS it's
> > good now.  I'm a bit uneasy about the way it changes indexes: it just
> > updates pg_class for them just before invoking the reindex in
> > finish_heap_swap.  I think it's correct as it stands though; at least,
> > the rewrite phase happens with the right setting, so that if there are
> > constraints being checked and these invoke index scans, such accesses
> > would not leave buffers with the wrong setting in shared_buffers.
> >
>
> Ok.
>
>
> > Another option would be to pass the new relpersistence down to
> > finish_heap_swap, but that would be hugely complicated AFAICS.
> >
>
> I think isn't so complicated to do it, but will this improve something ?
> Maybe I didn't understand it very well. IMHO it just complicate a
> simple thing.
>
>
>
> > Here's the updated patch.
> >
>
> Thanks Alvaro!
>

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

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

[Noah Misch]

On Wed, Aug 20, 2014 at 08:11:01PM +0900, Etsuro Fujita wrote:
> (2014/07/02 11:23), Noah Misch wrote:
> >Your chosen ANALYZE behavior is fair, but the messaging from a database-wide
> >ANALYZE VERBOSE needs work:
> >
> >INFO:  analyzing "test_foreign_inherit.parent"
> >INFO:  "parent": scanned 1 of 1 pages, containing 1 live rows and 0 dead 
> >rows; 1 rows in sample, 1 estimated total rows
> >INFO:  analyzing "test_foreign_inherit.parent" inheritance tree
> >WARNING:  relcache reference leak: relation "child" not closed
> >WARNING:  relcache reference leak: relation "tchild" not closed
> >WARNING:  relcache reference leak: relation "parent" not closed
> >
> >Please arrange to omit the 'analyzing "tablename" inheritance tree' message,
> >since this ANALYZE actually skipped that task.  (The warnings obviously need 
> >a
> >fix, too.)  I do find it awkward that adding a foreign table to an 
> >inheritance
> >tree will make autovacuum stop collecting statistics on that inheritance 
> >tree,
> >but I can't think of a better policy.
> 
> I think it would be better that this is handled in the same way as
> an inheritance tree that turns out to be a singe table that doesn't
> have any descendants in acquire_inherited_sample_rows().  That would
> still output the message as shown below, but I think that that would
> be more consistent with the existing code.  The patch works so.
> (The warnings are also fixed.)
> 
> INFO:  analyzing "public.parent"
> INFO:  "parent": scanned 0 of 0 pages, containing 0 live rows and 0
> dead rows; 0 rows in sample, 0 estimated total rows
> INFO:  analyzing "public.parent" inheritance tree
> INFO:  analyzing "pg_catalog.pg_authid"
> INFO:  "pg_authid": scanned 1 of 1 pages, containing 1 live rows and
> 0 dead rows; 1 rows in sample, 1 estimated total rows

Today's ANALYZE VERBOSE messaging for former inheritance parents (tables with
relhassubclass = true but no pg_inherits.inhparent links) is deceptive, and I
welcome a fix to omit the spurious message.  As defects go, this is quite
minor.  There's fundamentally no value in collecting inheritance tree
statistics for such a parent, and no PostgreSQL command will do so.

Your proposed behavior for inheritance parents having at least one foreign
table child is more likely to mislead DBAs in practice.  An inheritance tree
genuinely exists, and a different ANALYZE command is quite capable of
collecting statistics on that inheritance tree.  Messaging should reflect the
difference between ANALYZE invocations that do such work and ANALYZE
invocations that skip it.  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.  It's a minor point overall, but the code
impact is, I predict, small enough that we may as well get it right.  A
credible alternative is to emit a second message indicating that we skipped
the inheritance tree statistics after all, and why we skipped them.


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

Re: [HACKERS] New Model For Role Attributes and Fine Grained Permssions

[Stephen Frost]

Heikki,

* Heikki Linnakangas (hlinnakan...@vmware.com) wrote:
> On 08/19/2014 04:27 AM, Brightwell, Adam wrote:
> >This is a "proof-of-concept" patch for a new model around role attributes
> >and fine grained permissions meant to alleviate the current over dependence
> >on superuser.
> 
> Hmm. How does this get us any closer to fine-grained permissions? 

This patch, by itself, does not- but it adds the structure to allow us
to add more permissions without having to add more fields to pg_authid,
and we could build into pg_permission the "WITH ADMIN OPTION" and
"grantor" bits that the normal GRANT syntax already supports- but
without having to chew up additional bits for these granted permissions
which are applied to roles instead of objects in the system.

As for specific examples where this could be used beyond those
presented, consider things like:

CREATE EXTENSION
CREATE TABLESPACE
COPY (server-side)

The question which I've been kicking around is the possible additional
constraints which we may want/need for these- a list of extensions which
the role is allowed to create, a set of directories which the role is
allowed to create tablespaces within, similairly a set of directories
the role is allowed to use server-side COPY with (and perhaps a
distinction between COPY FROM and COPY TO).

> I
> guess we need this, so that you can grant/revoke the permissions,
> but I thought the hard part is defining what the fine-grained
> permissions are, in a way that you can't escalate yourself to full
> superuser through any of them.

This is definitely a concern- which is why I mention the specific items
above as needing to be constrained in some fashion.  CREATE EXTENSION
and CREATE TABLESPACE are, in a way, "naturally" constrained if you
imagine an environment where the user with those permissions doesn't
have direct access to modify the filesystem.  COPY, on the other hand,
would allow the user to gain access to pg_authid through indirect means
and therefore gain access to an actual superuser role on the system,
potentially.

(Ok- it might be possible to do that with CREATE TABLESPACE too, but
it'd be a bit more interesting to work through that than being able to
just COPY to a bytea and download the result)

> The syntax for GRANT/REVOKE is quite complicated already. Do we want
> to overload it even more? Also keep in mind that the SQL standard
> specifies GRANT/REVOKE, so we have to be careful to not clash with
> the SQL standard syntax, or any conceivable future syntax the SQL
> committee might add to it (although we have plenty of PostgreSQL
> extensions in it already). For example, this is currently legal:

I agree that there are concerns in this area and I've not got any great
solutions.  There are certainly pros and cons to using GRANT.  It's
familiar and natural to DBAs, but it runs the risk of conflicting with
future SQL syntax, or even our own GRANT role.  We can avoid the latter
by keeping to reserved keywords only, but that may lead to some rather
odd syntax (how would the "GRANT COPY ON (dir1, dir2, dir3)" work?).

Is there a good alternative to consider though..?

Thanks!

Stephen


signature.asc
Description: Digital signature

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

[Andrew Gierth]

> "Stephen" == Stephen Frost  writes:

 >>> I'm inclined to think that the audience for this is far larger
 >>> than the audience for the cube extension, which I have not once
 >>> encountered in the field.

 Stephen> +1

Most of my encounters with cube have been me suggesting it to people
on IRC as a possible approach for solving certain kinds of performance
problems by converting them to N-dimensional spatial containment
queries. Sometimes this works well, but it doesn't seem to be an
approach that many people discover on their own.

 >> We've jumped through some pretty high hoops to avoid reserving
 >> keywords in the past, so I don't think this patch should get a
 >> free pass on the issue.

 Stephen> This doesn't feel like an attempt to get a free pass on
 Stephen> anything- it's not being proposed as fully reserved and
 Stephen> there is spec-defined syntax which needs to be supported.
 Stephen> If we can get away with keeping it unreserved while not
 Stephen> making it utterly confusing for users and convoluting the
 Stephen> code, great, but that doesn't seem likely to pan out.

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

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

[Fabrízio de Royes Mello]

On Thu, Aug 21, 2014 at 8:04 PM, Alvaro Herrera 
wrote:
> Andres Freund wrote:
>
> > Have you looked at the correctness of the patch itself? Last time I'd
> > looked it has fundamental correctness issues. I'd outlined a possible
> > solution, but I haven't looked since.
>
> Yeah, Fabrizio had it passing the relpersistence down to make_new_heap,
> so the transient table is created with the right setting.  AFAICS it's
> good now.  I'm a bit uneasy about the way it changes indexes: it just
> updates pg_class for them just before invoking the reindex in
> finish_heap_swap.  I think it's correct as it stands though; at least,
> the rewrite phase happens with the right setting, so that if there are
> constraints being checked and these invoke index scans, such accesses
> would not leave buffers with the wrong setting in shared_buffers.
>

Ok.

> Another option would be to pass the new relpersistence down to
> finish_heap_swap, but that would be hugely complicated AFAICS.
>

I think isn't so complicated to do it, but will this improve something ?
Maybe I didn't understand it very well. IMHO it just complicate a
simple thing.


> Here's the updated patch.
>

Thanks Alvaro!

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

[Thomas Munro]

Hi Marko,

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:

In pgp-pgsql.c, function pgp_sym_decrypt_verify_bytea:

+   if (PG_NARGS() > 3)
+   PG_FREE_IF_COPY(arg, 3);
+   if (PG_NARGS() > 4)
+   PG_FREE_IF_COPY(arg, 4);

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

I think the same mistake appears in pgp_sym_decrypt_verify_text.

+   if (!sig->onepass)
+   {
+   time_t t;
+
+   isnull[3] = false;
+   /* unsigned big endian */
+   t = sig->creation_time[0] << 24;
+   t += sig->creation_time[1] << 16;
+   t += sig->creation_time[2] << 8;
+   t += sig->creation_time[3];
+   values[3] = time_t_to_timestamptz(t);
+   }

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?

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.

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

Best regards,

Thomas Munro


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

-- 
  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] ALTER TABLESPACE MOVE command tag tweak

[Stephen Frost]

Alvaro,

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> ALTER TABLE ALL IN TABLESPACE xyz
> which AFAICS should work since ALL is already a reserved keyword.

Pushed to master and REL9_4_STABLE.  Apologies on it taking so long-
things have a bit "interesting" for me over the past month or two. :)

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Should extension--unpackaged-$ver.sql's \echo use FROM unpackaged;?

[Andres Freund]

On 2014-07-06 16:58:33 -0400, Robert Haas wrote:
> On Thu, Jul 3, 2014 at 9:03 AM, Fujii Masao  wrote:
> > On Thu, Jul 3, 2014 at 4:26 AM, Andres Freund  
> > wrote:
> >> Hi,
> >> Fujii noticed that I'd used
> >> \echo Use "CREATE EXTENSION pg_buffercache" to load this file. \quit
> >> in an extension upgrade script. That's obviously wrong, because it
> >> should say ALTER EXTENSION. The reason it's that way is that I copied
> >> the line from the unpackaged--1.0.sql file.
> >> I noticed all unpackaged--$version transition files miss the "FROM
> >> unpackaged" in that echo.
> >> I guess that's just a oversight which we should correct?
> >
> > Maybe for user-friendness. But I think that's not so important fix enough
> > to backpatch.
> 
> Seems like a clear mistake to me.  +1 for fixing it and back-patching.

I think so as well - and I now have a patch for it. Fujii, do you mind
if I backpatch 6f9e39bc9993c1 to 9.1, 9.2 and 9.3 for you?

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

[David Fetter]

On Thu, Aug 21, 2014 at 06:15:33PM -0400, Stephen Frost wrote:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
> > Andrew Dunstan  writes:
> > > I'm inclined to think that the audience for this is far larger than the 
> > > audience for the cube extension, which I have not once encountered in 
> > > the field.
> 
> +1

I haven't seen it in the field either.

I'd also like to mention that the mere presence of a module in our
contrib/ directory can reflect bad decisions that need reversing just
as easily as it can the presence of vitally important utilities that
need to be preserved.  I'm pretty sure the cube extension arrived
after the CUBE keyword in SQL, which makes that an error on our part
if true.

> > Especially considering that renaming the cube extension isn't
> > exactly going to be zero work: there is no infrastructure for such
> > a thing.  A patch consisting merely of s/cube/foobar/g isn't going
> > to cut it.
> 
> This is a much more interesting challenge to deal with, but perhaps
> we could include a perl script or pg_upgrade snippet for users to
> run to see if they have the extension installed and to do some magic
> before the actual upgrade to handle the rename..?

+1 for doing this.  Do we want to make some kind of generator for such
things?  It doesn't seem hard in principle, but I haven't tried coding
it up yet.

Cheers,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

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


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

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

[Alvaro Herrera]

Tom Lane wrote:
> Alvaro Herrera  writes:
> > Agreed.  I am going over this patch, and the last bit I need to sort out
> > is the wording of these messages.  I have temporarily settled on this:
> 
> > ereport(ERROR,
> > (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
> >  errmsg("cannot change logged status of table %s to 
> > logged",
> > RelationGetRelationName(rel)),
> >  errdetail("Table %s references unlogged table %s.",
> >RelationGetRelationName(rel),
> >RelationGetRelationName(relfk;
> 
> > Note the term "logged status" to talk about whether a table is logged or
> > not.  I thought about "loggedness" but I'm not sure english speakers are
> > going to love me for that.  Any other ideas there?
> 
> Just say "cannot change status of table %s to logged".

I like this one, thanks.

> > Yeah, there is precedent for silently doing nothing.  We previously
> > threw warnings or notices, but nowadays even that is gone.  Throwing an
> > error definitely seems the wrong thing.
> 
> Agreed, just do nothing if it's already the right setting.

Done that way.

Andres Freund wrote:

> Have you looked at the correctness of the patch itself? Last time I'd
> looked it has fundamental correctness issues. I'd outlined a possible
> solution, but I haven't looked since.

Yeah, Fabrizio had it passing the relpersistence down to make_new_heap,
so the transient table is created with the right setting.  AFAICS it's
good now.  I'm a bit uneasy about the way it changes indexes: it just
updates pg_class for them just before invoking the reindex in
finish_heap_swap.  I think it's correct as it stands though; at least,
the rewrite phase happens with the right setting, so that if there are
constraints being checked and these invoke index scans, such accesses
would not leave buffers with the wrong setting in shared_buffers.

Another option would be to pass the new relpersistence down to
finish_heap_swap, but that would be hugely complicated AFAICS.

Here's the updated patch.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
*** a/doc/src/sgml/ref/alter_table.sgml
--- b/doc/src/sgml/ref/alter_table.sgml
***
*** 61,66  ALTER TABLE [ IF EXISTS ] name
--- 61,68 
  SET WITHOUT CLUSTER
  SET WITH OIDS
  SET WITHOUT OIDS
+ SET TABLESPACE new_tablespace
+ SET {LOGGED | UNLOGGED}
  SET ( storage_parameter = value [, ... ] )
  RESET ( storage_parameter [, ... ] )
  INHERIT parent_table
***
*** 68,74  ALTER TABLE [ IF EXISTS ] name
  OF type_name
  NOT OF
  OWNER TO new_owner
- SET TABLESPACE new_tablespace
  REPLICA IDENTITY {DEFAULT | USING INDEX index_name | FULL | NOTHING}
  
  and table_constraint_using_index is:
--- 70,75 
***
*** 477,482  ALTER TABLE [ IF EXISTS ] name
--- 478,508 
 
  
 
+ SET TABLESPACE
+ 
+  
+   This form changes the table's tablespace to the specified tablespace and
+   moves the data file(s) associated with the table to the new tablespace.
+   Indexes on the table, if any, are not moved; but they can be moved
+   separately with additional SET TABLESPACE commands.
+   See also
+   .
+  
+ 
+
+ 
+
+ SET {LOGGED | UNLOGGED}
+ 
+  
+   This form changes the table from unlogged to logged or vice-versa
+   (see ).  It cannot be applied
+   to a temporary table.
+  
+ 
+
+ 
+
  SET ( storage_parameter = value [, ... ] )
  
   
***
*** 589,608  ALTER TABLE [ IF EXISTS ] name
  
 
  
-
- SET TABLESPACE
- 
-  
-   This form changes the table's tablespace to the specified tablespace and
-   moves the data file(s) associated with the table to the new tablespace.
-   Indexes on the table, if any, are not moved; but they can be moved
-   separately with additional SET TABLESPACE commands.
-   See also
-   .
-  
- 
-
- 
 
  REPLICA IDENTITY
  
--- 615,620 
*** a/src/backend/commands/cluster.c
--- b/src/backend/commands/cluster.c
***
*** 574,580  rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
  	heap_close(OldHeap, NoLock);
  
  	/* Create the transient table that will receive the re-ordered data */
! 	OIDNewHeap = make_new_heap(tableOid, tableSpace, false,
  			   AccessExclusiveLock);
  
  	/* Copy the heap data into the new table in the desired order */
--- 574,581 
  	heap_close(OldHeap, NoLock);
  
  	/* Create the transient table that will receive the re-ordered data */
! 	OIDNewHeap = make_new_heap(tableOid, tableSpace,
! 			   OldHeap->rd_rel->relpersistence,
  			   AccessExclusiveLock);
  
  	/* Copy the heap data i

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

[Bruce Momjian]

On Tue, Mar  4, 2014 at 04:52:56PM +0100, Pavel Raiskup wrote:
> Hello,
> 
> 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.

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

  + Everyone has their own god. +
diff --git a/contrib/pg_upgrade/option.c b/contrib/pg_upgrade/option.c
new file mode 100644
index bb594dd..cfc88ec
*** a/contrib/pg_upgrade/option.c
--- b/contrib/pg_upgrade/option.c
*** parseCommandLine(int argc, char *argv[])
*** 137,153 
  break;
  
  			case 'o':
! old_cluster.pgopts = pg_strdup(optarg);
  break;
  
  			case 'O':
! new_cluster.pgopts = pg_strdup(optarg);
  break;
  
  /*
   * Someday, the port number option could be removed and passed
   * using -o/-O, but that requires postmaster -C to be
!  * supported on all old/new versions.
   */
  			case 'p':
  if ((old_cluster.port = atoi(optarg)) <= 0)
--- 137,171 
  break;
  
  			case 'o':
! /* append option? */
! if (!old_cluster.pgopts)
! 	old_cluster.pgopts = pg_strdup(optarg);
! else
! {
! 	char *old_pgopts = old_cluster.pgopts;
! 
! 	old_cluster.pgopts = psprintf("%s %s", old_pgopts, optarg);
! 	free(old_pgopts);
! }
  break;
  
  			case 'O':
! /* append option? */
! if (!new_cluster.pgopts)
! 	new_cluster.pgopts = pg_strdup(optarg);
! else
! {
! 	char *new_pgopts = new_cluster.pgopts;
! 
! 	new_cluster.pgopts = psprintf("%s %s", new_pgopts, optarg);
! 	free(new_pgopts);
! }
  break;
  
  /*
   * Someday, the port number option could be removed and passed
   * using -o/-O, but that requires postmaster -C to be
!  * supported on all old/new versions (added in PG 9.2).
   */
  			case 'p':
  if ((old_cluster.port = atoi(optarg)) <= 0)
diff --git a/doc/src/sgml/pgupgrade.sgml b/doc/src/sgml/pgupgrade.sgml
new file mode 100644
index b79f0db..dd57c5c
*** a/doc/src/sgml/pgupgrade.sgml
--- b/doc/src/sgml/pgupgrade.sgml
***
*** 130,143 
-o options
--old-options options
options to be passed directly to the
!   old postgres command
   
  
   
-O options
--new-options options
options to be passed directly to the
!   new postgres command
   
  
   
--- 130,145 
-o options
--old-options options
options to be passed directly to the
!   old postgres command;  multiple
!   option invocations are appended
   
  
   
-O options
--new-options options
options to be passed directly to the
!   new postgres command;  multiple
!   option invocations are appended
   
  
   
diff --git a/doc/src/sgml/ref/pg_ctl-ref.sgml b/doc/src/sgml/ref/pg_ctl-ref.sgml
new file mode 100644
index 2368129..29f882b
*** a/doc/src/sgml/ref/pg_ctl-ref.sgml
--- b/doc/src/sgml/ref/pg_ctl-ref.sgml
*** PostgreSQL documentation
*** 302,308 

 
  Specifies options to be passed directly to the
! postgres command.
 
 
  The options should usually be surrounded by single or double
--- 302,309 

 
  Specifies options to be passed directly to the
! postgres command;  multiple
! option invocations are appended.
 
 
  The options should usually be surrounded by single or double
diff --git a/doc/src/sgml/ref/postgres-ref.sgml b/doc/src/sgml/ref/postgres-ref.sgml
new file mode 100644
index cdfdaa0..845d969
*** a/doc/src/sgml/ref/postgres-ref.sgml
--- b/doc/src/sgml/ref/postgres-ref.sgml
*** PostgreSQL documentation
*** 288,294 
  class="parameter">extra-options are passed to
  all server processes started by this
  postgres process.  If the option string contains
! any spaces, the entire string must be quoted.
 
  
 
--- 288,295 
  class="parameter">extra-options are passed to
  all server processes started by this
  postgres process.  If the option string contains
! any spaces, the entire string must be quoted;  multiple
! option invocations are appended.
 
  
 
diff --git a/src/bin/pg_ctl/p

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

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Andrew Dunstan  writes:
> > I'm inclined to think that the audience for this is far larger than the 
> > audience for the cube extension, which I have not once encountered in 
> > the field.

+1

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

I'm with Merlin on this one, it's going to end up happening and I don't
know that 9.5 is any worse than post-9.5 to make this change.

> We've jumped through some pretty high hoops to avoid reserving keywords in
> the past, so I don't think this patch should get a free pass on the issue.

This doesn't feel like an attempt to get a free pass on anything- it's
not being proposed as fully reserved and there is spec-defined syntax
which needs to be supported.  If we can get away with keeping it
unreserved while not making it utterly confusing for users and
convoluting the code, great, but that doesn't seem likely to pan out.

> Especially considering that renaming the cube extension isn't exactly
> going to be zero work: there is no infrastructure for such a thing.
> A patch consisting merely of s/cube/foobar/g isn't going to cut it.

This is a much more interesting challenge to deal with, but perhaps we
could include a perl script or pg_upgrade snippet for users to run to
see if they have the extension installed and to do some magic before the
actual upgrade to handle the rename..?

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Add the number of pinning backends to pg_buffercache's output

[Andres Freund]

On 2014-07-02 08:55:05 +, Rajeev rastogi wrote:
> Also can we change the description of function "pg_buffercache_pages" to 
> include pinning_backend also in the description.

I'm not sure what you mean here - I've adjusted the docs to include the
column? Or do you mean that it errorneously was named pincount as Fujii
noticed?

Thanks for the review,

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] jsonb format is pessimal for toast compression

[Josh Berkus]

On 08/20/2014 03:42 PM, Arthur Silva wrote:
> What data are you using right now Josh?

The same data as upthread.

Can you test the three patches (9.4 head, 9.4 with Tom's cleanup of
Heikki's patch, and 9.4 with Tom's latest lengths-only) on your workload?

I'm concerned that my workload is unusual and don't want us to make this
decision based entirely on it.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


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

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

[Andres Freund]

On 2014-08-21 16:59:17 -0400, Alvaro Herrera wrote:
> Heikki Linnakangas wrote:
> 
> > In Postgres internals slang, non-permanent means temporary or
> > unlogged. But I agree we shouldn't expose users to that term; we use
> > it in the docs, and it's not used in command names either.
> 
> Agreed.  I am going over this patch, and the last bit I need to sort out
> is the wording of these messages.  I have temporarily settled on this:
> 
>   ereport(ERROR,
>   (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
>errmsg("cannot change logged status of table %s to 
> logged",
>   RelationGetRelationName(rel)),
>errdetail("Table %s references unlogged table %s.",
>  RelationGetRelationName(rel),
>  RelationGetRelationName(relfk;

Maybe 'cannot change persistency of table .. from unlogged to logged'; possibly 
with
s/persistency/durability/?


Have you looked at the correctness of the patch itself? Last time I'd
looked it has fundamental correctness issues. I'd outlined a possible
solution, but I haven't looked since.

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

[Tom Lane]

Alvaro Herrera  writes:
> Agreed.  I am going over this patch, and the last bit I need to sort out
> is the wording of these messages.  I have temporarily settled on this:

>   ereport(ERROR,
>   (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
>errmsg("cannot change logged status of table %s to 
> logged",
>   RelationGetRelationName(rel)),
>errdetail("Table %s references unlogged table %s.",
>  RelationGetRelationName(rel),
>  RelationGetRelationName(relfk;

> Note the term "logged status" to talk about whether a table is logged or
> not.  I thought about "loggedness" but I'm not sure english speakers are
> going to love me for that.  Any other ideas there?

Just say "cannot change status of table %s to logged".

> Yeah, there is precedent for silently doing nothing.  We previously
> threw warnings or notices, but nowadays even that is gone.  Throwing an
> error definitely seems the wrong thing.

Agreed, just do nothing if it's already the right setting.

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

[Tom Lane]

Andrew Dunstan  writes:
> I'm inclined to think that the audience for this is far larger than the 
> audience for the cube extension, which I have not once encountered in 
> the field.

Perhaps so.  I would really prefer not to have to get into estimating
how many people will be inconvenienced how badly.  It's clear to me
that not a lot of sweat has been put into seeing if we can avoid
reserving the keyword, and I think we need to put in that effort.
We've jumped through some pretty high hoops to avoid reserving keywords in
the past, so I don't think this patch should get a free pass on the issue.

Especially considering that renaming the cube extension isn't exactly
going to be zero work: there is no infrastructure for such a thing.
A patch consisting merely of s/cube/foobar/g isn't going to cut it.

regards, tom lane


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

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

[Alvaro Herrera]

Heikki Linnakangas wrote:

> In Postgres internals slang, non-permanent means temporary or
> unlogged. But I agree we shouldn't expose users to that term; we use
> it in the docs, and it's not used in command names either.

Agreed.  I am going over this patch, and the last bit I need to sort out
is the wording of these messages.  I have temporarily settled on this:

ereport(ERROR,
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
 errmsg("cannot change logged status of table %s to 
logged",
RelationGetRelationName(rel)),
 errdetail("Table %s references unlogged table %s.",
   RelationGetRelationName(rel),
   RelationGetRelationName(relfk;

Note the term "logged status" to talk about whether a table is logged or
not.  I thought about "loggedness" but I'm not sure english speakers are
going to love me for that.  Any other ideas there?

> I wonder if throwing an error is correct behavior anyway. Other
> ALTER TABLE commands just silently do nothing in similar situations,
> e.g:
> 
> lowerdb=# CREATE TABLE foo () WITH OIDS;
> CREATE TABLE
> lowerdb=# ALTER TABLE foo SET WITH OIDS;
> ALTER TABLE
> 
> But if we want to throw an error anyway, I'd suggest phrasing it
> "table foo is already unlogged"

Yeah, there is precedent for silently doing nothing.  We previously
threw warnings or notices, but nowadays even that is gone.  Throwing an
error definitely seems the wrong thing.  In the patch I have it's like
this:

ereport(ERROR,
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
 errmsg("cannot change logged status of table %s to 
unlogged",
RelationGetRelationName(rel)),
 errdetail("Table %s is already unlogged.",
   RelationGetRelationName(rel;

but I think I will just take that out as a whole and set a flag to
indicate nothing is to be done.

(This also means that setting tab->rewrite while analyzing the command
is the wrong thing to do.  Instead, the test for tab->rewrite should
have an || tab->chgLoggedness test, and we set chgLoggedness off if we
see that it's a no-op.  That way we avoid a pointless table rewrite and
a pointless error in a multi-command ALTER TABLE that has a no-op SET
LOGGED subcommand among other things.)


I changed the doc item in ALTER TABLE,

   
SET {LOGGED | UNLOGGED}

 
  This form changes the table from unlogged to logged or vice-versa
  (see ).  It cannot be applied
  to a temporary table.
 

   

I removed the fact that it needs ACCESS EXCLUSIVE because that's already
mentioned in the introductory paragraph.  I also removed the phrase that
it requires a table rewrite, because on reading existing text I noticed
that we don't document which forms cause rewrites.  Perhaps we should
document that somehow, but adding it to only one item seems wrong.

I will post an updated patch as soon as I fix a bug I introduced in the
check for FKs.

-- 
Á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: rounding up time value less than its unit.

[David G Johnston]

Peter Eisentraut-2 wrote
> On 8/21/14 11:16 AM, Tom Lane wrote:
>> Heikki Linnakangas <

> hlinnakangas@

> > writes:
>>> 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?
>> 
>> That sounds like a dealbreaker to me.  There are enough places where zero
>> has special meaning that we should not *ever* change zero to non-zero
>> silently.
> 
> I don't think I like this idea anyway.  If something has units of an
> hour and the user (perhaps misunderstanding the setting) sets it to one
> second, then we shouldn't silently change that to one hour.
> 
> If there is a problem with rounding it to zero, then we should perhaps
> raise an error.  (And stop treating zero specially.  It's a terrible
> idea.)

I'm on board, from the original thread, for errors if the input cannot be
converted to the parameter measurement unit cleanly.  By which I mean the
specified value should result in an integer being recorded without rounding. 
Specifying a precision less than the default unit thus becomes impossible.

I don't have a problem with zero meaning disabled when appropriate since it
avoids having a separate on/off GUC.

That said the complaint here just seems like a bug in the supplied patch -
zero is zero regardless of whether a unit is specified.  The only obvious
exception would be temperature but that isn't relevant here.

David J.



--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/proposal-rounding-up-time-value-less-than-its-unit-tp5811102p5815770.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.


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

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

[Andrew Dunstan]

On 08/21/2014 02:48 PM, Merlin Moncure wrote:


Basically, I'm afraid that unilaterally renaming cube is going to break
enough applications that there will be more people who flat out don't
want this patch than there will be who get benefit from it, and we end
up voting to revert the feature altogether.  If you'd like to take that
risk then feel free to charge full steam ahead, but don't say you were
not warned.  And don't bother arguing that CUBE is reserved according to
the standard, because that will not make one damn bit of difference
to the people who will be unhappy.

I have to respectfully disagree.  Certainly, if there is some
reasonable way to not have to change 'cube' then great.  But the
tonnage rule applies here: even considering compatibility issues, when
considering the importance of standard SQL (and, I might add,
exceptionally useful) syntax and a niche extension, 'cube' is going to
have to get out of the way.  There are view valid reasons to break
compatibility but blocking standard syntax is definitely one of them.




I'm inclined to think that the audience for this is far larger than the 
audience for the cube extension, which I have not once encountered in 
the field.


But I guess we all have different experiences.

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] run xmllint during build (was Re: need xmllint on borka)

[Fabien COELHO]

ISTM that it may be more helful to do:

  ifndef JADE
  #error "no jade found on your system, cannot generate the documention"
  endif


We could use $(missing) for that, which is already used for bison, flex,
and perl.


I'm fine with "$(missing)" or whatever else that provides a clear message.
Oops, not "#error", but "$(error  ...)", I was mixing cpp & make above...

However the example in the doc Makefile for "collageindex.pl" is on the 
heavy side, as it suggests that every use of such commands should be put 
in ifdef/else/endif. ISTM that a dependence-based solution would be 
simpler.


--
Fabien.


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

Re: [HACKERS] run xmllint during build (was Re: need xmllint on borka)

[Fabien COELHO]

It is only run when the build is via XML/XSLT, not via SGML/DSSSL
(because the SGML/DSSSL build already checks the validity anyway).


I'm confused.  I thought the point of this change was mostly that the
SGML toolchain is less strict than the XML toolchain, and you wanted
to have the more-strict checks applied whenever possible.


The SGML tool chain is less strict about what it considers valid, but
the XML toolchain doesn't check at all unless we run xmllint, it just
produces garbage when the input is invalid.


This suggests that "xmllint" should always be run, because it is more 
strict than the other, so it is safer if the source has passed it and is 
validated, whatever the tool chain used afterwards?


--
Fabien.


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

Re: [HACKERS] run xmllint during build (was Re: need xmllint on borka)

[Peter Eisentraut]

On 8/21/14 4:00 PM, Tom Lane wrote:
>> It is only run when the build is via XML/XSLT, not via SGML/DSSSL
>> (because the SGML/DSSSL build already checks the validity anyway).
> 
> I'm confused.  I thought the point of this change was mostly that the
> SGML toolchain is less strict than the XML toolchain, and you wanted
> to have the more-strict checks applied whenever possible.

The SGML tool chain is less strict about what it considers valid, but
the XML toolchain doesn't check at all unless we run xmllint, it just
produces garbage when the input is invalid.



-- 
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] Hardening pg_upgrade

[Magnus Hagander]

On Thu, Aug 21, 2014 at 10:07 PM, Tom Lane  wrote:
> Bruce Momjian  writes:
>> I tried writing the
>> query to use CTEs (second patch), but I would then have to have one
>> query for 8.3, which doesn't support CTEs, and another for 8.4+, plus
>> the CTE query was more complex than I liked.  Another idea would be to
>> drop 8.3 support (and remove lots of code to support that), but the
>> recent large increase in the number of people upgrading from 8.4 makes
>> that unattractive.  (8.3 did use a different timestamp storage format
>> though.)
>
> I vote for discarding 8.3 support in pg_upgrade.  There are already enough
> limitations on pg_upgrade from pre-8.4 to make it of questionable value;
> if it's going to create problems like this, it's time to cut the rope.

+1. 8.3 has been unsupported for a fairly long time now, and you can
still do a two-step upgrade if you're on that old a version.


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


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

Re: [HACKERS] Hardening pg_upgrade

[Tom Lane]

Bruce Momjian  writes:
> Specifically, the first attached patch causes pg_upgrade_support
> functions to throw errors when called by a backend that is not in binary
> upgrade mode.  (This seems like a good safety measure.)

Agreed about that part.

> Second, and
> more importantly, the patch prevents automatic oid assignment when in
> binary upgrade mode, except for temporary objects.  This is to help
> guarantee that system-assigned oids do not conflict with preserved oids.

> I had to make an exception for temporary tables because pg_upgrade uses
> temporary tables to collect schema information.

This seems like a bad idea.  If you are going to have such an off-switch
at all (which I'm not sure I buy the need for), it should not have holes
in it.

> I tried writing the
> query to use CTEs (second patch), but I would then have to have one
> query for 8.3, which doesn't support CTEs, and another for 8.4+, plus
> the CTE query was more complex than I liked.  Another idea would be to
> drop 8.3 support (and remove lots of code to support that), but the
> recent large increase in the number of people upgrading from 8.4 makes
> that unattractive.  (8.3 did use a different timestamp storage format
> though.)

I vote for discarding 8.3 support in pg_upgrade.  There are already enough
limitations on pg_upgrade from pre-8.4 to make it of questionable value;
if it's going to create problems like this, it's time to cut the rope.

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] run xmllint during build (was Re: need xmllint on borka)

[Tom Lane]

Peter Eisentraut  writes:
> We could use $(missing) for that, which is already used for bison, flex,
> and perl.

+1 ... I'm surprised it's not like that already.  Fabien's quite right to
complain that the Makefile has no business inserting defaults for things
configure couldn't find.

regards, tom lane


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

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

[Peter Eisentraut]

On 8/21/14 11:16 AM, Tom Lane wrote:
> Heikki Linnakangas  writes:
>> 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?
> 
> That sounds like a dealbreaker to me.  There are enough places where zero
> has special meaning that we should not *ever* change zero to non-zero
> silently.

I don't think I like this idea anyway.  If something has units of an
hour and the user (perhaps misunderstanding the setting) sets it to one
second, then we shouldn't silently change that to one hour.

If there is a problem with rounding it to zero, then we should perhaps
raise an error.  (And stop treating zero specially.  It's a terrible idea.)




-- 
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] run xmllint during build (was Re: need xmllint on borka)

[Tom Lane]

Peter Eisentraut  writes:
> On 8/21/14 5:12 AM, Fabien COELHO wrote:
>> However, it seems that it is not run for target "html", nor for pdf/ps
>> targets. I'm wondering whether it is an oversight or if there is a
>> reason for that. Maybe the intention is that an explicit "htmlhelp" is
>> expected to do the checking, but then why force it for man and epub? It
>> seems to me that it is not consistent.

> It is only run when the build is via XML/XSLT, not via SGML/DSSSL
> (because the SGML/DSSSL build already checks the validity anyway).

I'm confused.  I thought the point of this change was mostly that the
SGML toolchain is less strict than the XML toolchain, and you wanted
to have the more-strict checks applied whenever possible.

regards, tom lane


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

Re: [HACKERS] Enable WAL archiving even in standby

[Robert Haas]

On Tue, Aug 19, 2014 at 7:33 AM, Fujii Masao  wrote:
> On Fri, Aug 15, 2014 at 4:30 AM, Robert Haas  wrote:
>> On Wed, Aug 13, 2014 at 6:42 AM, Fujii Masao  wrote:
>>> I'd propose the attached WIP patch which allows us to enable WAL archiving
>>> even in standby. The patch adds "always" as the valid value of archive_mode.
>>> If it's set to "always", the archiver is started when the server is in 
>>> standby
>>> mode and all the WAL files that walreceiver wrote to the disk are archived 
>>> by
>>> using archive_command. Then, even after the server is promoted to master,
>>> the archiver keeps archiving WAL files. The patch doesn't change the 
>>> meanings
>>> of the setting values "on" and "off" of archive_mode.
>>
>> I like the feature, but I don't much like this as a control mechanism.
>> Having archive_command and standby_archive_command, as you propose
>> further down, seems saner.
>
> Okay, that's fine. One question is; Which WAL files should be archived by
> standby_archive_command? There are following kinds of WAL files.
>
> (1) WAL files which were fully written and closed by walreceiver
>  Curently they are not archived at all.
>
> (2) WAL file which is being written by walreceiver
>  This file will be closed before it's fully written because of,
>  for example, standby promotion.
>  Currently this is archived by archive_command.
>
> (3) WAL file with new timeline, which is copied from (2)
>   At the end of recovery, after new timeline is assigned,
>   this latest WAL file with new timeline is created by being copied
>   from (2) (i.e., latest WAL file with old timeline). WAL data of
>   end-of-recovery checkpoint is written to this latest WAL file.
>   Currently this is archived by archive_command.
>
> (4) Timeline history files
>  When standby is promoted to the master, the imeline is incremented
>  and the timeline history file is created.
>  Currently the timeline history files are archived by archive_command.
>
> (5) WAL files generated in normal processing mode
>   Currently they are archived by archive_command.
>
> I'm thinking to use standby_archive_command only for (1) because
> the others are currently archived by archive_command. That means
> that even if there are type (1) WAL files which have not been archived
> yet after the standby promotion (i.e., the situation where WAL archiving
> was delayed for some reasons in the standby), they are archived by
> standby_archive_command. IOW, the archiver uses both archive commands
> as the situation demands.
>
> OTOH, maybe there are people who want to use standby_archive_command
> for all the WAL files with old timeline, i.e., (1) and (2). Thought?

Boy, that's quite confusing.  I didn't think we ever ran
archive_command on the standby right now, so then it would make sense
to have a way to do that.  And it makes sense for it to be separate
from the mode used on the master to avoid breaking existing
configurations, so that a user assuming that a certain setting will
only take effect after promotion will not be disappointed.  However,
if what you're saying is that we do archiving on the standby sometimes
but not others, I'm not quite sure what the best way forward is.  It
seems rather inconsistent to do it for some types of WAL files but not
others.

-- 
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] run xmllint during build (was Re: need xmllint on borka)

[Peter Eisentraut]

On 8/21/14 5:12 AM, Fabien COELHO wrote:
> Also, a general comment, which is independent of this patch: I found the
> documentation build especially not resilient, with a lack of clear error
> messages when something is broken. Basically, if configure does not
> found something for the doc (openjade, osx, xmllint, ...) it does not
> complain. That is fine with me, people would not always want to build
> the doc anyway as it is available online. However, the Makefile in
> doc/src/sgml overrides the not found commands (ifndef JADE JADE=...,
> etc), and proceed to unhelpful and unclear errors later on. ISTM that it
> may be more helful to do:
> 
>   ifndef JADE
>   #error "no jade found on your system, cannot generate the documention"
>   endif

We could use $(missing) for that, which is already used for bison, flex,
and perl.


-- 
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] run xmllint during build (was Re: need xmllint on borka)

[Peter Eisentraut]

On 8/21/14 5:12 AM, Fabien COELHO wrote:
> 
> However, it seems that it is not run for target "html", nor for pdf/ps
> targets. I'm wondering whether it is an oversight or if there is a
> reason for that. Maybe the intention is that an explicit "htmlhelp" is
> expected to do the checking, but then why force it for man and epub? It
> seems to me that it is not consistent.

It is only run when the build is via XML/XSLT, not via SGML/DSSSL
(because the SGML/DSSSL build already checks the validity anyway).


-- 
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] Hardening pg_upgrade

[Bruce Momjian]

On Thu, Aug 21, 2014 at 11:43:42AM -0700, David G Johnston wrote:
> Bruce Momjian wrote
> > I had to make an exception for temporary tables because pg_upgrade uses
> > temporary tables to collect schema information.  I tried writing the
> > query to use CTEs (second patch), but I would then have to have one
> > query for 8.3, which doesn't support CTEs, and another for 8.4+, plus
> > the CTE query was more complex than I liked.  Another idea would be to
> > drop 8.3 support (and remove lots of code to support that), but the
> > recent large increase in the number of people upgrading from 8.4 makes
> > that unattractive.  (8.3 did use a different timestamp storage format
> > though.)
> 
> Why not tell people on 8.3- that a direct upgrade is not supported but that
> an indirect upgrade to 9.4 or earlier has to be performed first and then
> that can be upgraded to 9.5+ ?

Yes, we could easily do that, and trim down pg_upgrade in the process. 
Are people OK with that?

> I'm not clear on how the 8.4 upgrades volume impacts a decision to support
> 8.3- upgrades?

My point is that people aren't doing upgrades just from 9.1 and 9.2, but
often from very old releases. and the end-of-lifed of 8.4 prompted a lot
of people to upgrade.  Now, since 8.3 has been end-of-lifed since
February, 2013, we might be able to argue that 8.3 already had a year to
upgrade, so if they now want to upgrade, they have to do it in two
steps.

Anyway, I think we need more opinions on this.

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

[Heikki Linnakangas]

On 08/21/2014 05:04 PM, Thom Brown wrote:

On 21 August 2014 14:45, Fabrízio de Royes Mello 
wrote:


On Thu, Aug 21, 2014 at 5:23 AM, Christoph Berg  wrote:


Re: Thom Brown 2014-08-20 
7u7tsgl4s5jh1a+shq_ny7gorzc_g_yj7...@mail.gmail.com>

"ERROR:  table test is not permanent"

Perhaps this would be better as "table test is unlogged" as "permanent"
doesn't match the term used in the DDL syntax.


I was also wondering that, but then figured that when ALTER TABLE SET
UNLOGGED is invoked on temp tables, the error message "is not
permanent" was correct while the apparent opposite "is unlogged" is
wrong.


Thom,

Christoph is right... make no sense the message... see the example:

fabrizio=# create temp table foo();
CREATE TABLE
fabrizio=# alter table foo set unlogged;
ERROR:  table foo is unlogged

The previous message is better:

fabrizio=# create temp table foo();
CREATE TABLE
fabrizio=# alter table foo set unlogged;
ERROR:  table foo is not permanent
fabrizio=#
fabrizio=# create unlogged table foo2();
CREATE TABLE
fabrizio=# alter table foo2 set unlogged;
ERROR:  table foo2 is not permanent



To me, that's even more confusing:

CREATE TEMP TABLE test();
CREATE UNLOGGED TABLE test2();

# ALTER TABLE test SET LOGGED;
ERROR:  table test is not unlogged

# ALTER TABLE test SET UNLOGGED;
ERROR:  table test is not permanent

# ALTER TABLE test2 SET UNLOGGED;
ERROR:  table test2 is not permanent

They're being rejected for different reasons but the error message is
identical.  Permanent suggests the opposite of temporary, and unlogged
tables aren't temporary.


In Postgres internals slang, non-permanent means temporary or unlogged. 
But I agree we shouldn't expose users to that term; we use it in the 
docs, and it's not used in command names either.


I wonder if throwing an error is correct behavior anyway. Other ALTER 
TABLE commands just silently do nothing in similar situations, e.g:


lowerdb=# CREATE TABLE foo () WITH OIDS;
CREATE TABLE
lowerdb=# ALTER TABLE foo SET WITH OIDS;
ALTER TABLE

But if we want to throw an error anyway, I'd suggest phrasing it "table 
foo is already unlogged"


- 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

[Merlin Moncure]

On Thu, Aug 21, 2014 at 1: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.
>
> Basically, I'm afraid that unilaterally renaming cube is going to break
> enough applications that there will be more people who flat out don't
> want this patch than there will be who get benefit from it, and we end
> up voting to revert the feature altogether.  If you'd like to take that
> risk then feel free to charge full steam ahead, but don't say you were
> not warned.  And don't bother arguing that CUBE is reserved according to
> the standard, because that will not make one damn bit of difference
> to the people who will be unhappy.

I have to respectfully disagree.  Certainly, if there is some
reasonable way to not have to change 'cube' then great.  But the
tonnage rule applies here: even considering compatibility issues, when
considering the importance of standard SQL (and, I might add,
exceptionally useful) syntax and a niche extension, 'cube' is going to
have to get out of the way.  There are view valid reasons to break
compatibility but blocking standard syntax is definitely one of them.

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] Hardening pg_upgrade

[David G Johnston]

Bruce Momjian wrote
> I had to make an exception for temporary tables because pg_upgrade uses
> temporary tables to collect schema information.  I tried writing the
> query to use CTEs (second patch), but I would then have to have one
> query for 8.3, which doesn't support CTEs, and another for 8.4+, plus
> the CTE query was more complex than I liked.  Another idea would be to
> drop 8.3 support (and remove lots of code to support that), but the
> recent large increase in the number of people upgrading from 8.4 makes
> that unattractive.  (8.3 did use a different timestamp storage format
> though.)

Why not tell people on 8.3- that a direct upgrade is not supported but that
an indirect upgrade to 9.4 or earlier has to be performed first and then
that can be upgraded to 9.5+ ?

I'm not clear on how the 8.4 upgrades volume impacts a decision to support
8.3- upgrades?

David J.





--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/Hardening-pg-upgrade-tp5815735p5815748.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.


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

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

[Tom Lane]

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.

Basically, I'm afraid that unilaterally renaming cube is going to break
enough applications that there will be more people who flat out don't
want this patch than there will be who get benefit from it, and we end
up voting to revert the feature altogether.  If you'd like to take that
risk then feel free to charge full steam ahead, but don't say you were
not warned.  And don't bother arguing that CUBE is reserved according to
the standard, because that will not make one damn bit of difference
to the people who will be unhappy.

regards, tom lane


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

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

[Alvaro Herrera]

Baker, Keith [OCDUS Non-J&J] wrote:

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

I don't think thread-safety is of great concern.  The backend is not
multithreaded, and neither are the utilities (I think the only exception
is pgbench, and even there it is optional).  The only problem, as I
recall, would be that libpq would not lock things correctly when used in
a multithreaded program.  I think you will need to solve this
eventually, but it doesn't look as critical as the others.

I was asking specifically about spinlocks because if you have to use
that switch, it means our spinlock implementation doesn't cover your
platform, and you would need to add something to support native
spinlocks.  Since you're using gcc on x86, I assume your port is
choosing an already existing, working implementation.

-- 
Á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] pg_dumpall reccomendation in release notes

[Bruce Momjian]

On Tue, Feb 25, 2014 at 05:05:09PM -0800, Josh Berkus wrote:
> On 02/25/2014 04:42 PM, Bruce Momjian wrote:
> > On Tue, Feb 25, 2014 at 06:41:26PM -0500, Tom Lane wrote:
> >> I'm not sure what "many limitations" you think pg_dumpall has that pg_dump
> >> doesn't.
> >>
> >> I do think that it might be time to reword this to recommend pg_upgrade
> >> first, though.  ISTM that the current wording dates from when pg_upgrade
> >> could charitably be described as experimental.
> > 
> > Wow, so pg_upgrade takes the lead!  And from Tom too!  :-)
> > 
> > I agree with Tom that mentioning pg_dump/restore is going to lead to
> > global object data loss, and throwing the users to a URL with no
> > explanation isn't going to help either.  What we could do is to
> > restructure the existing text and add a link to the upgrade URL for more
> > details.
> 
> What I was suggesting was something like:
> 
> "Users upgrading from earlier versions will need to go through the
> entire upgrade procedure, as described on our upgrade page: "
> 
> The problem is that anything we say about "how to upgrade" in one short
> sentence is going to confuse some people.  BTW, the reason I got that
> question about pg_dump was that 9.2's release notes say "pg_dump" and
> 9.3's say "pg_dumpall", causing users to think there's been some kind of
> change.
> 
> Of course, this means I need to fix the upgrade page, and I need to
> write backported versions of that fix for at least 9.1 and 9.2.

I have developed the attached patch to address the issues raised above:

o  non-text output of pg_dump is mentioned
o  mentions of using OID for keys is removed
o  the necessity of pg_dumpall --globals-only is mentioned
o  using pg_dump parallel mode rather than pg_dumpall for upgrades is mentioned
o  pg_upgrade is mentioned more prominently for upgrades
o  replication upgrades are in their own section

I don't think we want to mention pg_upgrade as the _primary_
major-version upgrade method.  While the pg_dump upgrade section is
long, it is mostly about starting/stoping the server, moving
directories, etc, the same things you have to do for pg_upgrade, so I
just mentioned that int the pg_upgrade section.  Other ideas?

I plan to apply this to head and 9.4.


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

  + Everyone has their own god. +
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
new file mode 100644
index 06f064e..07ca0dc
*** a/doc/src/sgml/backup.sgml
--- b/doc/src/sgml/backup.sgml
***
*** 28,34 
SQL Dump
  

!The idea behind this dump method is to generate a text file with SQL
 commands that, when fed back to the server, will recreate the
 database in the same state as it was at the time of the dump.
 PostgreSQL provides the utility program
--- 28,34 
SQL Dump
  

!The idea behind this dump method is to generate a file with SQL
 commands that, when fed back to the server, will recreate the
 database in the same state as it was at the time of the dump.
 PostgreSQL provides the utility program
*** pg_dump d
*** 39,44 
--- 39,47 
  
 As you see, pg_dump writes its result to the
 standard output. We will see below how this can be useful.
+While the above command creates a text file, pg_dump
+can create files in other formats that allow for parallism and more
+fine-grained control of object restoration.

  

*** pg_dump d
*** 98,117 
 exclusive lock, such as most forms of ALTER TABLE.)

  
-   
-
- If your database schema relies on OIDs (for instance, as foreign
- keys) you must instruct pg_dump to dump the OIDs
- as well. To do this, use the -o command-line
- option.
-
-   
- 

 Restoring the Dump
  
 
! The text files created by pg_dump are intended to
  be read in by the psql program. The
  general command form to restore a dump is
  
--- 101,111 
 exclusive lock, such as most forms of ALTER TABLE.)

  

 Restoring the Dump
  
 
! Text files created by pg_dump are intended to
  be read in by the psql program. The
  general command form to restore a dump is
  
*** psql dbna
*** 127,132 
--- 121,128 
  supports options similar to pg_dump for specifying
  the database server to connect to and the user name to use. See
  the  reference page for more information.
+ Non-text file dumps are restored using the  utility.
 
  
 
*** psql -f i
*** 225,231 
  roles, tablespaces, and empty databases, then invoking
  pg_dump for each database.  This means that while
  each database will be internally consistent, the snapshots of
! different databases might not be exactly in-sync.
 

  
--- 221,234 
  roles, tablespaces, and empty databases, then invoking
  pg_dump fo

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

[Pavel Stehule]

2014-08-21 17:58 GMT+02:00 Andrew Gierth :

> > "Tom" == Tom Lane  writes:
>
>  >> I agree, the contrib/cube patch as posted is purely so we could test
>  >> everything without having to argue over the new name first.
>
>  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.
>
> GROUP BY GROUPING SETS (cube(a,b), c)  -- is that cube(a,b) an expression
> to group on, or a list of grouping sets to expand?
>
> GROUP BY (cube(a,b)) -- should that be an error, or silently treat it
> as a function call rather than a grouping set? What about GROUP BY
> GROUPING SETS ((cube(a,b)) ? (both are errors in our patch)
>
> Accepting those as valid implies a degree of possible confusion that I
> personally regard as quite questionable.  Previous discussion seemed to
> have accepted that contrib/cube was going to have to be renamed.
>
>  Tom> I think that the cube extension is not going to be the only
>  Tom> casualty if "cube" becomes a reserved word --- that seems like a
>  Tom> name that could be in use in lots of applications.  ("What do
>  Tom> you mean, 9.5 breaks our database for tracking office space?")
>  Tom> It would be worth quite a bit of effort to avoid that.
>
> It has been a reserved word in the spec since, what, 1999? and it is a
> reserved word in mssql, oracle, db2, etc.?
>
> It only needs to be a col_name_keyword, so it still works as a table
> or column name (as usual we are less strict than the spec in that
> respect).  I'm looking into whether it can be made unreserved, but I
> have serious doubts about this being a good idea.
>

+1

contrib module should be renamed - more - current name is confusing against
usual functionality related to words CUBE and ROLLUP

Pavel


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

[Andrew Gierth]

> "Tom" == Tom Lane  writes:

 >> I agree, the contrib/cube patch as posted is purely so we could test
 >> everything without having to argue over the new name first.

 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.

GROUP BY GROUPING SETS (cube(a,b), c)  -- is that cube(a,b) an expression
to group on, or a list of grouping sets to expand?

GROUP BY (cube(a,b)) -- should that be an error, or silently treat it
as a function call rather than a grouping set? What about GROUP BY
GROUPING SETS ((cube(a,b)) ? (both are errors in our patch)

Accepting those as valid implies a degree of possible confusion that I
personally regard as quite questionable.  Previous discussion seemed to
have accepted that contrib/cube was going to have to be renamed.

 Tom> I think that the cube extension is not going to be the only
 Tom> casualty if "cube" becomes a reserved word --- that seems like a
 Tom> name that could be in use in lots of applications.  ("What do
 Tom> you mean, 9.5 breaks our database for tracking office space?")
 Tom> It would be worth quite a bit of effort to avoid that.

It has been a reserved word in the spec since, what, 1999? and it is a
reserved word in mssql, oracle, db2, etc.?

It only needs to be a col_name_keyword, so it still works as a table
or column name (as usual we are less strict than the spec in that
respect).  I'm looking into whether it can be made unreserved, but I
have serious doubts about this being a good idea.

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

Hello Andres,

Thanks for your response.

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.
Changing default behavior of libc on QNX to suit PostgreSQL may break other 
applications which rely on the current behavior of libc.

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.

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.

Testing so far has demonstrated that the macro fixes are effective on QNX.  
Repeated runs of the regression tests run cleanly.
More testing will be required to boost the confidence and expose any gaps, but 
the foundation appears to be solid.

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.


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.

I hope the explanations above address your main concerns.
Again, thanks for your response!

Keith Baker

> -Original Message-
> From: Andres Freund [mailto:and...@2ndquadrant.com]
> Sent: Wednesday, August 20, 2014 7:25 PM
> To: Baker, Keith [OCDUS Non-J&J]
> Cc: Alvaro Herrera; Robert Haas; Tom Lane; pgsql-hackers@postgresql.org
> Subject: Re: [HACKERS] Proposal to add a QNX 6.5 port to PostgreSQL
> 
> Hi,
> 
> On 2014-08-20 21:21:41 +, Baker, Keith [OCDUS Non-J&J] wrote:
> > To work around lack of SA_RESTART, I added QNX-specific retry macros
> > to port.h With these macros in place "make check" runs cleanly (fails in
> many place without them).
> >
> > +#if defined(__QNX__)
> > +/* QNX does not support sigaction SA_RESTART. We must retry
> interrupted calls (EINTR) */
> > +/* Helper macros, used to build our retry macros */
> > +#define PG_RETRY_EINTR3(exp,val,type) ({ type _tmp_rc; do _tmp_rc =
> (exp); while (_tmp_rc == (val) && errno == EINTR); _tmp_rc; })
> > +#define PG_RETRY_EINTR(exp) PG_RETRY_EINTR3(exp,-1L,long int)
> > +#define PG_RETRY_EINTR_FILE(exp) PG_RETRY_EINTR3(exp,NULL,FILE
> *)
> > +/* override calls known to return EINTR when interrupted */
> > +#define close(a) PG_RETRY_EINTR(close(a))
> > +#define fclose(a) PG_RETRY_EINTR(fclose(a))
> > +#define fdopen(a,b) PG_RETRY_EINTR_FILE(fdopen(a,b))
> > +#define fopen(a,b) PG_RETRY_EINTR_FILE(fopen(a,b))
> > +#define freopen(a,b,c) PG_RETRY_EINTR_FILE(freopen(a,b,c))
> > +#define fseek(a,b,c) PG_RETRY_EINTR(fseek(a,b,c))
> > +#define fseeko(a,b,c) PG_RETRY_EINTR(fseeko(a,b,c))
> > +#define ftruncate(a,b) PG_RETRY_EINTR(ftruncate(a,b))
> > +#define lseek(a,b,c) PG_RETRY_EINTR(lseek(a,b,c))
> > +#define open(a,b,...) ({ int _tmp_rc; do _tmp_rc =
> open(a,b,##__VA_ARGS__); while (_tmp_rc == (-1) && errno == EINTR);
> _tmp_rc; })
> > +#define shm_open(a,b,c) PG_RETRY_EINTR(shm_open(a,b,c))
> > +#define stat(a,b) PG_RETRY_EINTR(stat(a,b))
> > +#define unlink(a) PG_RETRY_EINTR(unlink(a))
> > ... (Macros for read and write are similar but slightly longer, so I 
> > omit
> them here)...
> > +#endif /* __QNX__ */
> 
> I think this is a horrible way to go and unlikely to succeed. You're surely 
> going
> to miss calls and it's going to need to be maintained continuously. We'll miss
> adding things which will then only break under load. Which most poeple
> won't be able to generate under qnx.
> 
> The only reasonably way to fake kernel SA_RESTART support is doing so is in
> $platform's libc. In the syscall wrapper.
> 
> > Here is what I used for configure, I am open to suggestions:
> > ./configure --without-readline --disable-thread-safety
>   
> Why is the --disable-thread-safety needed?
> 
> Greetings,
> 
> Andres Freun

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

[Tom Lane]

Heikki Linnakangas  writes:
> 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?

That sounds like a dealbreaker to me.  There are enough places where zero
has special meaning that we should not *ever* change zero to non-zero
silently.

regards, tom lane


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

[HACKERS] Hardening pg_upgrade

[Bruce Momjian]

Now that everyone is happy with how pg_upgrade_support uses global
variables to set preserved oids (or at least has no better ideas), I
think it is time to lock down this usage to prevent future breakage.

Specifically, the first attached patch causes pg_upgrade_support
functions to throw errors when called by a backend that is not in binary
upgrade mode.  (This seems like a good safety measure.)  Second, and
more importantly, the patch prevents automatic oid assignment when in
binary upgrade mode, except for temporary objects.  This is to help
guarantee that system-assigned oids do not conflict with preserved oids.

I had to make an exception for temporary tables because pg_upgrade uses
temporary tables to collect schema information.  I tried writing the
query to use CTEs (second patch), but I would then have to have one
query for 8.3, which doesn't support CTEs, and another for 8.4+, plus
the CTE query was more complex than I liked.  Another idea would be to
drop 8.3 support (and remove lots of code to support that), but the
recent large increase in the number of people upgrading from 8.4 makes
that unattractive.  (8.3 did use a different timestamp storage format
though.)

(Of course, the assumption is that temporary tables will not exist at
the time you are assigning preserved oids.)

Barring objections, I plan to apply the first patch to head, and discard
the second patch.

FYI, I think the macro isTempOrToastNamespace() is misnamed --- it is
testing for temporary tables or temporary toast tables, not for any
toast table.  Seems it should be called isTempOrToastTempNamespace(). 
Should I rename it in a separate commit?

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

  + Everyone has their own god. +
diff --git a/contrib/pg_upgrade_support/pg_upgrade_support.c b/contrib/pg_upgrade_support/pg_upgrade_support.c
new file mode 100644
index edd41d0..beaee76
*** a/contrib/pg_upgrade_support/pg_upgrade_support.c
--- b/contrib/pg_upgrade_support/pg_upgrade_support.c
*** PG_FUNCTION_INFO_V1(set_next_pg_authid_o
*** 38,49 
--- 38,57 
  
  PG_FUNCTION_INFO_V1(create_empty_extension);
  
+ #define CHECK_IS_BINARY_UPGRADE \
+ do { 			\
+ 	if (!IsBinaryUpgrade)		\
+ 		ereport(ERROR,			\
+ (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),	\
+  (errmsg("function can only be called when server is in binary upgrade mode"; \
+ } while (0)
  
  Datum
  set_next_pg_type_oid(PG_FUNCTION_ARGS)
  {
  	Oid			typoid = PG_GETARG_OID(0);
  
+ 	CHECK_IS_BINARY_UPGRADE;
  	binary_upgrade_next_pg_type_oid = typoid;
  
  	PG_RETURN_VOID();
*** set_next_array_pg_type_oid(PG_FUNCTION_A
*** 54,59 
--- 62,68 
  {
  	Oid			typoid = PG_GETARG_OID(0);
  
+ 	CHECK_IS_BINARY_UPGRADE;
  	binary_upgrade_next_array_pg_type_oid = typoid;
  
  	PG_RETURN_VOID();
*** set_next_toast_pg_type_oid(PG_FUNCTION_A
*** 64,69 
--- 73,79 
  {
  	Oid			typoid = PG_GETARG_OID(0);
  
+ 	CHECK_IS_BINARY_UPGRADE;
  	binary_upgrade_next_toast_pg_type_oid = typoid;
  
  	PG_RETURN_VOID();
*** set_next_heap_pg_class_oid(PG_FUNCTION_A
*** 74,79 
--- 84,90 
  {
  	Oid			reloid = PG_GETARG_OID(0);
  
+ 	CHECK_IS_BINARY_UPGRADE;
  	binary_upgrade_next_heap_pg_class_oid = reloid;
  
  	PG_RETURN_VOID();
*** set_next_index_pg_class_oid(PG_FUNCTION_
*** 84,89 
--- 95,101 
  {
  	Oid			reloid = PG_GETARG_OID(0);
  
+ 	CHECK_IS_BINARY_UPGRADE;
  	binary_upgrade_next_index_pg_class_oid = reloid;
  
  	PG_RETURN_VOID();
*** set_next_toast_pg_class_oid(PG_FUNCTION_
*** 94,99 
--- 106,112 
  {
  	Oid			reloid = PG_GETARG_OID(0);
  
+ 	CHECK_IS_BINARY_UPGRADE;
  	binary_upgrade_next_toast_pg_class_oid = reloid;
  
  	PG_RETURN_VOID();
*** set_next_pg_enum_oid(PG_FUNCTION_ARGS)
*** 104,109 
--- 117,123 
  {
  	Oid			enumoid = PG_GETARG_OID(0);
  
+ 	CHECK_IS_BINARY_UPGRADE;
  	binary_upgrade_next_pg_enum_oid = enumoid;
  
  	PG_RETURN_VOID();
*** set_next_pg_authid_oid(PG_FUNCTION_ARGS)
*** 114,119 
--- 128,134 
  {
  	Oid			authoid = PG_GETARG_OID(0);
  
+ 	CHECK_IS_BINARY_UPGRADE;
  	binary_upgrade_next_pg_authid_oid = authoid;
  	PG_RETURN_VOID();
  }
*** create_empty_extension(PG_FUNCTION_ARGS)
*** 129,134 
--- 144,151 
  	Datum		extCondition;
  	List	   *requiredExtensions;
  
+ 	CHECK_IS_BINARY_UPGRADE;
+ 
  	if (PG_ARGISNULL(4))
  		extConfig = PointerGetDatum(NULL);
  	else
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
new file mode 100644
index 33eef9f..d810fe9
*** a/src/backend/catalog/heap.c
--- b/src/backend/catalog/heap.c
***
*** 39,44 
--- 39,45 
  #include "catalog/dependency.h"
  #include "catalog/heap.h"
  #include "catalog/index.h"
+ #include "catalog/namespace.h"
  #include "catalog/objectaccess.h"
  #include "catalog/pg_attrdef.h"
  #

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

[Pavel Stehule]

2014-08-21 17:00 GMT+02:00 Tom Lane :

> Andrew Gierth  writes:
> > "Heikki" == Heikki Linnakangas  writes:
> >  Heikki> I think we should bite the bullet and rename the extension,
>
> > I agree, the contrib/cube patch as posted is purely so we could test
> > everything without having to argue over the new name first.
>
> I wonder if you've tried hard enough to avoid reserving the keyword.
>
> I think that the cube extension is not going to be the only casualty
> if "cube" becomes a reserved word --- that seems like a name that
> could be in use in lots of applications.  ("What do you mean, 9.5
> breaks our database for tracking office space?")  It would be worth
> quite a bit of effort to avoid that.
>

My prototypes worked without reserved keywords if I remember well

but analyzer is relatively complex

Pavel




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

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

[Andrew Gierth]

> "Heikki" == Heikki Linnakangas  writes:

 Heikki> Uh, that's ugly. The EXPLAIN out I mean; as an implementation
 Heikki> detail chaining the nodes might be reasonable. But the above
 Heikki> gets unreadable if you have more than a few grouping sets.

It's good for highlighting performance issues in EXPLAIN, too.

4096 grouping sets takes about a third of a second to plan and execute,
but something like a minute to generate the EXPLAIN output. However,
for more realistic sizes, plan time is not significant and explain
takes only about 40ms for 256 grouping sets.

(To avoid resource exhaustion issues, we have set a limit of,
currently, 4096 grouping sets per query level. Without such a limit,
it is easy to write queries that would take TBs of memory to parse or
plan. MSSQL and DB2 have similar limits, I'm told.)

 >> The ChainAggregate nodes use a tuplestore to communicate with the
 >> GroupAggregate node at the top of the chain; they pass through input
 >> tuples unchanged, and write aggregated result rows to the tuplestore,
 >> which the top node then returns once it has finished its own result.

 Heikki> Hmm, so there's a "magic link" between the GroupAggregate at
 Heikki> the top and all the ChainAggregates, via the tuplestore. That
 Heikki> may be fine, we have special rules in passing information
 Heikki> between bitmap scan nodes too.

Eh. It's far from a perfect solution, but the planner doesn't lend itself
to perfect solutions.

 Heikki> But rather than chain multiple ChainAggregate nodes, how
 Heikki> about just doing all the work in the top GroupAggregate node?

It was easier this way. (How would you expect to do it all in the top
node when each subset of the grouping sets list needs to see the data
in a different order?)

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

[Tom Lane]

Andrew Gierth  writes:
> "Heikki" == Heikki Linnakangas  writes:
>  Heikki> I think we should bite the bullet and rename the extension,

> I agree, the contrib/cube patch as posted is purely so we could test
> everything without having to argue over the new name first.

I wonder if you've tried hard enough to avoid reserving the keyword.

I think that the cube extension is not going to be the only casualty
if "cube" becomes a reserved word --- that seems like a name that
could be in use in lots of applications.  ("What do you mean, 9.5
breaks our database for tracking office space?")  It would be worth
quite a bit of effort 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] add line number as prompt option to psql

[Alvaro Herrera]

Jeevan Chalke wrote:

> I have initialize cur_lineno to UINTMAX - 2. And then observed following
> behaviour to check wrap-around.
> 
> postgres=# \set PROMPT1 '%/[%l]%R%# '
> postgres[18446744073709551613]=# \set PROMPT2 '%/[%l]%R%# '
> postgres[18446744073709551613]=# select
> postgres[18446744073709551614]-# a
> postgres[18446744073709551615]-# ,
> postgres[0]-# b
> postgres[1]-# from
> postgres[2]-# dual;
> 
> It is wrapping to 0, where as line number always start with 1. Any issues?
> 
> 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.

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

[Thom Brown]

On 21 August 2014 14:45, Fabrízio de Royes Mello 
wrote:

>
> On Thu, Aug 21, 2014 at 5:23 AM, Christoph Berg  wrote:
> >
> > Re: Thom Brown 2014-08-20  7u7tsgl4s5jh1a+shq_ny7gorzc_g_yj7...@mail.gmail.com>
> > > "ERROR:  table test is not permanent"
> > >
> > > Perhaps this would be better as "table test is unlogged" as "permanent"
> > > doesn't match the term used in the DDL syntax.
> >
> > I was also wondering that, but then figured that when ALTER TABLE SET
> > UNLOGGED is invoked on temp tables, the error message "is not
> > permanent" was correct while the apparent opposite "is unlogged" is
> > wrong.
> >
> > Christoph
> > --
> > c...@df7cb.de | http://www.df7cb.de/
>
> Thom,
>
> Christoph is right... make no sense the message... see the example:
>
> fabrizio=# create temp table foo();
> CREATE TABLE
> fabrizio=# alter table foo set unlogged;
> ERROR:  table foo is unlogged
>
> The previous message is better:
>
> fabrizio=# create temp table foo();
> CREATE TABLE
> fabrizio=# alter table foo set unlogged;
> ERROR:  table foo is not permanent
> fabrizio=#
> fabrizio=# create unlogged table foo2();
> CREATE TABLE
> fabrizio=# alter table foo2 set unlogged;
> ERROR:  table foo2 is not permanent
>

To me, that's even more confusing:

CREATE TEMP TABLE test();
CREATE UNLOGGED TABLE test2();

# ALTER TABLE test SET LOGGED;
ERROR:  table test is not unlogged

# ALTER TABLE test SET UNLOGGED;
ERROR:  table test is not permanent

# ALTER TABLE test2 SET UNLOGGED;
ERROR:  table test2 is not permanent

They're being rejected for different reasons but the error message is
identical.  Permanent suggests the opposite of temporary, and unlogged
tables aren't temporary.

-- 
Thom

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

[Fabrízio de Royes Mello]

On Thu, Aug 21, 2014 at 5:23 AM, Christoph Berg  wrote:
>
> Re: Thom Brown 2014-08-20 
> > "ERROR:  table test is not permanent"
> >
> > Perhaps this would be better as "table test is unlogged" as "permanent"
> > doesn't match the term used in the DDL syntax.
>
> I was also wondering that, but then figured that when ALTER TABLE SET
> UNLOGGED is invoked on temp tables, the error message "is not
> permanent" was correct while the apparent opposite "is unlogged" is
> wrong.
>
> Christoph
> --
> c...@df7cb.de | http://www.df7cb.de/

Thom,

Christoph is right... make no sense the message... see the example:

fabrizio=# create temp table foo();
CREATE TABLE
fabrizio=# alter table foo set unlogged;
ERROR:  table foo is unlogged

The previous message is better:

fabrizio=# create temp table foo();
CREATE TABLE
fabrizio=# alter table foo set unlogged;
ERROR:  table foo is not permanent
fabrizio=#
fabrizio=# create unlogged table foo2();
CREATE TABLE
fabrizio=# alter table foo2 set unlogged;
ERROR:  table foo2 is not permanent

Patch attached.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 69a1e14..397b4e6 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -59,16 +59,17 @@ ALTER TABLE [ IF EXISTS ] name
 ENABLE ALWAYS RULE rewrite_rule_name
 CLUSTER ON index_name
 SET WITHOUT CLUSTER
+SET {LOGGED | UNLOGGED}
 SET WITH OIDS
 SET WITHOUT OIDS
 SET ( storage_parameter = value [, ... ] )
+SET TABLESPACE new_tablespace
 RESET ( storage_parameter [, ... ] )
 INHERIT parent_table
 NO INHERIT parent_table
 OF type_name
 NOT OF
 OWNER TO new_owner
-SET TABLESPACE new_tablespace
 REPLICA IDENTITY {DEFAULT | USING INDEX index_name | FULL | NOTHING}
 
 and table_constraint_using_index is:
@@ -447,6 +448,20 @@ ALTER TABLE [ IF EXISTS ] name

 

+SET {LOGGED | UNLOGGED}
+
+ 
+  This form changes the table persistence type from unlogged to permanent or
+  from permanent to unlogged (see ).
+ 
+ 
+  Changing the table persistence type acquires an ACCESS EXCLUSIVE lock
+  and rewrites the table contents and associated indexes into new disk files.
+ 
+
+   
+
+   
 SET WITH OIDS
 
  
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index b1c411a..7f497c7 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -574,7 +574,8 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
 	heap_close(OldHeap, NoLock);
 
 	/* Create the transient table that will receive the re-ordered data */
-	OIDNewHeap = make_new_heap(tableOid, tableSpace, false,
+	OIDNewHeap = make_new_heap(tableOid, tableSpace,
+			   OldHeap->rd_rel->relpersistence,
 			   AccessExclusiveLock);
 
 	/* Copy the heap data into the new table in the desired order */
@@ -601,7 +602,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
  * data, then call finish_heap_swap to complete the operation.
  */
 Oid
-make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp,
+make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence,
 			  LOCKMODE lockmode)
 {
 	TupleDesc	OldHeapDesc;
@@ -613,7 +614,6 @@ make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp,
 	Datum		reloptions;
 	bool		isNull;
 	Oid			namespaceid;
-	char		relpersistence;
 
 	OldHeap = heap_open(OIDOldHeap, lockmode);
 	OldHeapDesc = RelationGetDescr(OldHeap);
@@ -636,16 +636,10 @@ make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp,
 	if (isNull)
 		reloptions = (Datum) 0;
 
-	if (forcetemp)
-	{
+	if (relpersistence == RELPERSISTENCE_TEMP)
 		namespaceid = LookupCreationNamespace("pg_temp");
-		relpersistence = RELPERSISTENCE_TEMP;
-	}
 	else
-	{
 		namespaceid = RelationGetNamespace(OldHeap);
-		relpersistence = OldHeap->rd_rel->relpersistence;
-	}
 
 	/*
 	 * Create the new heap, using a temporary name in the same namespace as
@@ -1146,6 +1140,7 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class,
 	Oid			relfilenode1,
 relfilenode2;
 	Oid			swaptemp;
+	char		swaprelpersistence;
 	CatalogIndexState indstate;
 
 	/* We need writable copies of both pg_class tuples. */
@@ -1177,6 +1172,10 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class,
 		relform1->reltablespace = relform2->reltablespace;
 		relform2->reltablespace = swaptemp;
 
+		swaprelpersistence = relform1->relpersistence;
+		relform1->relpersistence = relform2->relpersistence;
+		relform2->relpersistence = swaprelpersistence;
+
 		/* Also swap toast links, if we're swapping by links */
 		if (!swap_toast_by_content)
 

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

[Andrew Dunstan]

On 08/15/2014 11: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.




What's happening about this? Buildfarm animal jacana is consistently red 
because of this.


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] proposal: rounding up time value less than its unit.

[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_ageminutes, 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

Re: [HACKERS] plpgsql.extra_warnings='num_into_expressions'

[Heikki Linnakangas]

On 08/21/2014 02:09 PM, Marko Tiikkaja wrote:

On 8/21/14, 1:19 PM, Heikki Linnakangas wrote:

On 08/07/2014 01:11 AM, Marko Tiikkaja wrote:

On 7/21/14, 10:56 PM, I wrote:

Here's a patch which allows you to notice those annoying bugs with INTO
slightly more quickly.  Adding to the next commit phest.


New version, fixed bugs with set operations and VALUES lists.


Looks good.

There's probably more checking like that that you could add, but that
can be done as add-on patches, if ever. The INTO mistake happens a lot
more easily.


Yeah, I think the mistake is at least as easy to do in "FOR .. IN
", and I'm planning to add checks for that as well.  But I
haven't found the time to look at it amongst all the other patches and
projects I have going


Ok.


(and also, unfortunately, I'm on vacation right now).


Oh, have fun!


It seems weird to pass a PLpgSQL_row struct to check_sql_expr.
check_sql_expr only needs to know how many attributes is expected to be
in the target list, so it would be more natural to just pass an "int
expected_natts".


I'm not sure about this, though.  AFAICT all the interesting cases are
already holding a PLpgSQL_row, and in that case it seems easier to just
pass that in to check_sql_expr() without making the callers worry about
extracting the expected_natts from the row.


Hmm. The integer FOR syntax I used in my example does not, it always 
expects 1 output column.



 And we can always change
the interface should such a case come up, since the interface is
completely internal.  Just my 0.02EUR, of course.


You might want to add a helper function to count the number of 
attributes in a PLpgSQL_row. Then the check_sql_expr call would be 
almost as simple:  check_sql_expr(..., get_row_natts(row)).


- 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] plpgsql.extra_warnings='num_into_expressions'

[Marko Tiikkaja]

On 8/21/14, 1:19 PM, Heikki Linnakangas wrote:

On 08/07/2014 01:11 AM, Marko Tiikkaja wrote:

On 7/21/14, 10:56 PM, I wrote:

Here's a patch which allows you to notice those annoying bugs with INTO
slightly more quickly.  Adding to the next commit phest.


New version, fixed bugs with set operations and VALUES lists.


Looks good.

There's probably more checking like that that you could add, but that
can be done as add-on patches, if ever. The INTO mistake happens a lot
more easily.


Yeah, I think the mistake is at least as easy to do in "FOR .. IN 
", and I'm planning to add checks for that as well.  But I 
haven't found the time to look at it amongst all the other patches and 
projects I have going (and also, unfortunately, I'm on vacation right now).



It seems weird to pass a PLpgSQL_row struct to check_sql_expr.
check_sql_expr only needs to know how many attributes is expected to be
in the target list, so it would be more natural to just pass an "int
expected_natts".


I'm not sure about this, though.  AFAICT all the interesting cases are 
already holding a PLpgSQL_row, and in that case it seems easier to just 
pass that in to check_sql_expr() without making the callers worry about 
extracting the expected_natts from the row.  And we can always change 
the interface should such a case come up, since the interface is 
completely internal.  Just my 0.02EUR, of course.



.marko


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

[Heikki Linnakangas]

On 08/21/2014 01:28 PM, Andrew Gierth wrote:


A progress update:

  Atri>  We envisage that handling of arbitrary grouping sets will be
  Atri> best done by having the planner generating an Append of
  Atri> multiple aggregation paths, presumably with some way of moving
  Atri> the original input path to a CTE. We have not really explored
  Atri> yet how hard this will be; suggestions are welcome.

This idea was abandoned.

Instead, we have implemented full support for arbitrary grouping sets
by means of a chaining system:

explain (verbose, costs off) select four, ten, hundred, count(*) from onek 
group by cube(four,ten,hundred);

  QUERY PLAN
-
  GroupAggregate
Output: four, ten, hundred, count(*)
Grouping Sets: (onek.hundred, onek.four, onek.ten), (onek.hundred, 
onek.four), (onek.hundred), ()
->  Sort
  Output: four, ten, hundred
  Sort Key: onek.hundred, onek.four, onek.ten
  ->  ChainAggregate
Output: four, ten, hundred
Grouping Sets: (onek.ten, onek.hundred), (onek.ten)
->  Sort
  Output: four, ten, hundred
  Sort Key: onek.ten, onek.hundred
  ->  ChainAggregate
Output: four, ten, hundred
Grouping Sets: (onek.four, onek.ten), (onek.four)
->  Sort
  Output: four, ten, hundred
  Sort Key: onek.four, onek.ten
  ->  Seq Scan on public.onek
Output: four, ten, hundred
(20 rows)


Uh, that's ugly. The EXPLAIN out I mean; as an implementation detail 
chaining the nodes might be reasonable. But the above gets unreadable if 
you have more than a few grouping sets.



The ChainAggregate nodes use a tuplestore to communicate with the
GroupAggregate node at the top of the chain; they pass through input
tuples unchanged, and write aggregated result rows to the tuplestore,
which the top node then returns once it has finished its own result.


Hmm, so there's a "magic link" between the GroupAggregate at the top and 
all the ChainAggregates, via the tuplestore. That may be fine, we have 
special rules in passing information between bitmap scan nodes too.


But rather than chain multiple ChainAggregate nodes, how about just 
doing all the work in the top GroupAggregate node?



  Atri> At this point we are more interested in design review rather
  Atri> than necessarily committing this patch in its current state.

This no longer applies; we expect to post within a day or two an
updated patch with full functionality.


Ok, cool

- 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] documentation update for doc/src/sgml/func.sgml

[Fabien COELHO]

I do not understand why the last sentence in the first paragraph about 
bitwise ops is put there with rounding issues, which seem unrelated. It 
seems to me that it belongs to the second paragraph which is about 
bitwise operators.


That's the part which came from Josh Berkus. We discussed this patch on IRC.


Hmmm. I do think the last sentence belongs to the next paragraph. The 
identity of the author does not change my opinion on that point:-) If you 
have another argument, maybe.



The wikipedia link can be simplified to a much cleaner:

 http://en.wikipedia.org/wiki/IEEE_floating_point#Rounding_rules


It can, but then you always refer to the latest version of the Wikipedia 
page, which might or might not be a good idea. The link in the patch points 
to the current version from yesterday, no matter how many changes are 
introduced afterwards.


I doubt that IEEE floating point rounding rules are likely to change much, 
so referencing the latest version is both safe & cleaner. Also, wikipedia 
would change its implementation from php to something else (well, 
unlikely, probably as unlikely as a change in IEEE fp rounding rules:-).


--
Fabien.


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

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

[Andrew Gierth]

A progress update:

 Atri>  We envisage that handling of arbitrary grouping sets will be
 Atri> best done by having the planner generating an Append of
 Atri> multiple aggregation paths, presumably with some way of moving
 Atri> the original input path to a CTE. We have not really explored
 Atri> yet how hard this will be; suggestions are welcome.

This idea was abandoned.

Instead, we have implemented full support for arbitrary grouping sets
by means of a chaining system:

explain (verbose, costs off) select four, ten, hundred, count(*) from onek 
group by cube(four,ten,hundred);

 QUERY PLAN 
 
-
 GroupAggregate
   Output: four, ten, hundred, count(*)
   Grouping Sets: (onek.hundred, onek.four, onek.ten), (onek.hundred, 
onek.four), (onek.hundred), ()
   ->  Sort
 Output: four, ten, hundred
 Sort Key: onek.hundred, onek.four, onek.ten
 ->  ChainAggregate
   Output: four, ten, hundred
   Grouping Sets: (onek.ten, onek.hundred), (onek.ten)
   ->  Sort
 Output: four, ten, hundred
 Sort Key: onek.ten, onek.hundred
 ->  ChainAggregate
   Output: four, ten, hundred
   Grouping Sets: (onek.four, onek.ten), (onek.four)
   ->  Sort
 Output: four, ten, hundred
 Sort Key: onek.four, onek.ten
 ->  Seq Scan on public.onek
   Output: four, ten, hundred
(20 rows)

The ChainAggregate nodes use a tuplestore to communicate with the
GroupAggregate node at the top of the chain; they pass through input
tuples unchanged, and write aggregated result rows to the tuplestore,
which the top node then returns once it has finished its own result.

The organization of the planner code seems to be actively hostile to
any attempt to break out new CTEs on the fly, or to plan parts of the
query more than once; the method above seems to be the easiest way to
avoid those issues.

 Atri> At this point we are more interested in design review rather
 Atri> than necessarily committing this patch in its current state.

This no longer applies; we expect to post within a day or two an
updated patch with full functionality.

-- 
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] documentation update for doc/src/sgml/func.sgml

[Andreas 'ads' Scherbaum]

On 08/21/2014 11:53 AM, Fabien COELHO wrote:



attached is a small patch which updates doc/src/sgml/func.sgml. The
change explains that functions like round() and others might behave
different depending on your operating system (because of rint(3)) and
that this is according to an IEEE standard. It also points out that
#.5 is not always rounded up, as expected from a mathematical point of
view.


Applied on head & read. I'm not a native English speaker, but the
English looked right to me.


Thanks.



Comments:

I'm not sure that the "note that" on the third line is useful.

I do not understand why the last sentence in the first paragraph about
bitwise ops is put there with rounding issues, which seem unrelated. It
seems to me that it belongs to the second paragraph which is about
bitwise operators.


That's the part which came from Josh Berkus. We discussed this patch on IRC.




The wikipedia link can be simplified to a much cleaner:

 http://en.wikipedia.org/wiki/IEEE_floating_point#Rounding_rules


It can, but then you always refer to the latest version of the Wikipedia 
page, which might or might not be a good idea. The link in the patch 
points to the current version from yesterday, no matter how many changes 
are introduced afterwards.


But yes:


Also, I submitted docs with relevant wikipedia links which was stripped
of these before committing. I'm wondering whether there is a general
policy not to put external links from within the text in the
documentation. There are very few of them, most in "acronym.sgml".


It would be nice to have a general rule how to handle external links.



I would suggest to put relevant tags around functions and types, like:
"round()" and "NUMERIC".


Can do.


Thanks,

--
Andreas 'ads' Scherbaum
German PostgreSQL User Group
European PostgreSQL User Group - Board of Directors
Volunteer Regional Contact, Germany - PostgreSQL Project


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

Re: [HACKERS] plpgsql.extra_warnings='num_into_expressions'

[Heikki Linnakangas]

On 08/07/2014 01:11 AM, Marko Tiikkaja wrote:

On 7/21/14, 10:56 PM, I wrote:

Here's a patch which allows you to notice those annoying bugs with INTO
slightly more quickly.  Adding to the next commit phest.


New version, fixed bugs with set operations and VALUES lists.


Looks good.

It seems weird to pass a PLpgSQL_row struct to check_sql_expr. 
check_sql_expr only needs to know how many attributes is expected to be 
in the target list, so it would be more natural to just pass an "int 
expected_natts".


Once you do that, you could trivially also add checking for other cases, 
e.g:


do $$
declare
  i int4;
begin
  -- fails at runtime, because "SELECT 1,3" returns two attributes,
  -- but FOR expects 1
  for i in 1,3..(2) loop
raise notice 'foo %', i;
  end loop;
end;
$$;

There's probably more checking like that that you could add, but that 
can be done as add-on patches, if ever. The INTO mistake happens a lot 
more easily.


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

[Amit Kapila]

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.

Do you see any need to change below line in docs:
"If a tablespace is relocated in this way, the symbolic links inside the
main data directory are updated to point to the new location."
Refer link:
http://www.postgresql.org/docs/devel/static/app-pgbasebackup.html

I was not sure whether docs need any change, so kept them
intact.

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


pg_basebackup_relocate_tablespace_v3.patch
Description: Binary data

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

Re: [HACKERS] documentation update for doc/src/sgml/func.sgml

[Fabien COELHO]

attached is a small patch which updates doc/src/sgml/func.sgml. The change 
explains that functions like round() and others might behave different 
depending on your operating system (because of rint(3)) and that this is 
according to an IEEE standard. It also points out that #.5 is not always 
rounded up, as expected from a mathematical point of view.


Applied on head & read. I'm not a native English speaker, but the English 
looked right to me.


Comments:

I'm not sure that the "note that" on the third line is useful.

I do not understand why the last sentence in the first paragraph about 
bitwise ops is put there with rounding issues, which seem unrelated. It 
seems to me that it belongs to the second paragraph which is about bitwise 
operators.


The wikipedia link can be simplified to a much cleaner:

http://en.wikipedia.org/wiki/IEEE_floating_point#Rounding_rules

Also, I submitted docs with relevant wikipedia links which was stripped of 
these before committing. I'm wondering whether there is a general policy 
not to put external links from within the text in the documentation. There 
are very few of them, most in "acronym.sgml".


I would suggest to put relevant tags around functions and types, like: 
"round()" and "NUMERIC".


--
Fabien.


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

Re: [HACKERS] inherit support for foreign tables

[Etsuro Fujita]

(2014/07/02 11:23), Noah Misch wrote:

On Fri, Jun 20, 2014 at 05:04:06PM +0900, Kyotaro HORIGUCHI wrote:

Attached is the rebased patch of v11 up to the current master.



The rest of these review comments are strictly observations; they're not
requests for you to change the patch, but they may deserve more discussion.

We use the term "child table" in many messages.  Should that change to
something more inclusive, now that the child may be a foreign table?  Perhaps
one of "child relation", plain "child", or "child foreign table"/"child table"
depending on the actual object?  "child relation" is robust technically, but
it might add more confusion than it removes.  Varying the message depending on
the actual object feels like a waste.  Opinions?


IMHO, I think that "child table" would not confusing users terribly.


LOCK TABLE on the inheritance parent locks child foreign tables, but LOCK
TABLE fails when given a foreign table directly.  That's odd, but I see no
cause to change it.


I agree wth that.


The longstanding behavior of CREATE TABLE INHERITS is to reorder local columns
to match the order found in parents.  That is, both of these tables actually
have columns in the order (a,b):

create table parent (a int, b int);
create table child (b int, a int) inherits (parent);

Ordinary dump/reload always uses CREATE TABLE INHERITS, thereby changing
column order in this way.  (pg_dump --binary-upgrade uses ALTER TABLE INHERIT
and some catalog hacks to avoid doing so.)  I've never liked that dump/reload
can change column order, but it's tolerable if you follow the best practice of
always writing out column lists.  The stakes rise for foreign tables, because
column order is inherently significant to file_fdw and probably to certain
other non-RDBMS FDWs.  If pg_dump changes column order in your file_fdw
foreign table, the table breaks.  I would heartily support making pg_dump
preserve column order for all inheritance children.  That doesn't rise to the
level of being a blocker for this patch, though.


I agree with that, too.  I think it would be better to add docs for now.

Thanks,

Best regards,
Etsuro Fujita


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

Re: [HACKERS] run xmllint during build (was Re: need xmllint on borka)

[Fabien COELHO]

Also, a general comment, which is independent of this patch: I found the 
documentation build especially not resilient, with a lack of clear error 
messages when something is broken. Basically, if configure does not found 
something for the doc (openjade, osx, xmllint, ...) it does not complain. 
That is fine with me, people would not always want to build the doc anyway as 
it is available online. However, the Makefile in doc/src/sgml overrides the 
not found commands (ifndef JADE JADE=..., etc), and proceed to unhelpful and 
unclear errors later on. ISTM that it may be more helful to do:


To be more constructive:

Maybe all commands could have a check counterpart added to the 
dependencies, so as to fail gracefully only if needed, something like:


  .check_XXX:
if type -p $(XXX) > /dev/null ; then touch $@ ; else \
  echo "command $(XXX) not found"; exit 1 ; \
fi

  foo: .check_XXX
$(XXX) ...

I'm not sure how to check for the docbook style availability though.

--
Fabien.


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

Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

[Etsuro Fujita]

(2014/08/21 13:21), Ashutosh Bapat wrote:

On Wed, Aug 20, 2014 at 3:25 PM, Etsuro Fujita
mailto:fujita.ets...@lab.ntt.co.jp>> wrote:



Hi Ashutish,


I am sorry that I mistook your name's spelling.


(2014/08/14 22:30), Ashutosh Bapat wrote:

On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita
mailto:fujita.ets...@lab.ntt.co.jp>
>> wrote:

 (2014/08/08 18:51), Etsuro Fujita wrote:
  >>> (2014/06/30 22:48), Tom Lane wrote:
   I wonder whether it isn't time to change that.  It
was coded
 like that
   originally only because calculating the values
would've been a
 waste of
   cycles at the time.  But this is at least the third place
 where it'd be
   useful to have attr_needed for child rels.



 There was a problem with the previous patch, which will be
described
 below.  Attached is the updated version of the patch
addressing that.



Here are some more comments



attr_needed also has the attributes used in the restriction
clauses as
seen in distribute_qual_to_rels(), so, it looks unnecessary to call
pull_varattnos() on the clauses in baserestrictinfo in functions
check_selective_binary___conversion(),
remove_unused_subquery___outputs(),
check_index_only().



IIUC, I think it's *necessary* to do that in those functions since
the attributes used in the restriction clauses in baserestrictinfo
are not added to attr_needed due the following code in
distribute_qual_to_rels.



That's right. Thanks for pointing that out.



Although in case of RTE_RELATION, the 0th entry in attr_needed
corresponds to FirstLowInvalidHeapAttributeNu__mber + 1, it's
always safer
to use it is RelOptInfo::min_attr, in case get_relation_info()
wants to
change assumption or somewhere down the line some other part of code
wants to change attr_needed information. It may be unlikely, that it
would change, but it will be better to stick to comments in
RelOptInfo
   443 AttrNumber  min_attr;   /* smallest attrno of rel
(often
<0) */
   444 AttrNumber  max_attr;   /* largest attrno of rel */
   445 Relids *attr_needed;/* array indexed [min_attr ..
max_attr] */



Good point!  Attached is the revised version of the patch.



If the patch is not in the commit-fest, can you please add it there?


I've already done that:

https://commitfest.postgresql.org/action/patch_view?id=1529


 From my side, the review is done, it should be marked "ready for
committer", unless somebody else wants to review.


Many thanks!

Best regards,
Etsuro Fujita


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

Re: [HACKERS] pg_receivexlog --status-interval add fsync feedback

[Sawada Masahiko]

On Thu, Aug 21, 2014 at 2:54 PM,   wrote:
>> When replication slot is not specified in pg_receivexlog, the flush
>> location in the feedback message always indicates invalid. So there seems
>> to be no need to send the feedback as soon as fsync is issued, in that
>> case.
>> How should this option work when replication slot is not specified?
>
> Thanks for the review!
>
> The present is not checking the existence of specification of -S.
>
> The use case when replication slot is not specified.
>
> Because it does fsync, it isn't an original intention.
> remote_write is set in synchronous_commit.
>
> To call attention to the user, append following documents.
> "If you want to report the flush position to the server, should use -S 
> option."
>

Thank you for updating the patch.
I reviewed the patch.

First of all, I think that we should not append the above message to
section of '-r' option.
(Or these message might not be needed at all)
Whether flush location in feedback message is valid,  is not depend on
'-r' option.

If we use '-r' option and 'S' option (i.g., replication slot) then
pg_receivexlog informs valid flush
location to primary server at the same time as doing fsync.
But,  if we don't specify replication slot then the flush location in
feedback message always invalid.
So I think Fujii-san pointed out that sending of invalid flush
location is not needed
if pg_receivexlog does not use replication slot.

Regards,

---
Sawada Masahiko


-- 
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] run xmllint during build (was Re: need xmllint on borka)

[Fabien COELHO]

Hello Peter,


Agreed.  I have committed the SGML changes that make things valid now,
but I will postpone the xmllint addition until the 9.5 branch, complete
with more documentation.


Per the above announcement, here is an updated patch, also with more
documentation and explanations.

It would especially be valuable if someone with a different-than-mine OS
would verify whether they can install xmllint according to the
documentation, or update the documentation if not.


Tested on Ubuntu trusty.

Patched applies with some minor warning on current head, and works, that 
is "xmllint" is run for some of the targets (epub, man).


However, it seems that it is not run for target "html", nor for pdf/ps 
targets. I'm wondering whether it is an oversight or if there is a reason 
for that. Maybe the intention is that an explicit "htmlhelp" is expected 
to do the checking, but then why force it for man and epub? It seems to me 
that it is not consistent.



Also, a general comment, which is independent of this patch: I found the 
documentation build especially not resilient, with a lack of clear error 
messages when something is broken. Basically, if configure does not found 
something for the doc (openjade, osx, xmllint, ...) it does not complain. 
That is fine with me, people would not always want to build the doc anyway 
as it is available online. However, the Makefile in doc/src/sgml overrides 
the not found commands (ifndef JADE JADE=..., etc), and proceed to 
unhelpful and unclear errors later on. ISTM that it may be more helful to 
do:


  ifndef JADE
  #error "no jade found on your system, cannot generate the documention"
  endif

Rather than overriding with "JADE=jade" if jade was not there when 
configuring.


This xmllint addition is done in the same spirit. I would suggest at the 
minimum to check that xmllint is available before running it, or to ignore 
the call if not available, something like:


type -p $(XMLLINT) || { echo error no $(XMLLINT)... ; exit 1 ; }
$(XMLLINT) ...

or

-type -p $(XMLLINT) && $(XMLLINT) ...

And I would prefer that a straightforward error is generated when
commands or styles are not found, in general.

Also, a detail, the Makefile style is not homogeneous:

ifndef XSLTPROC
XSLTPROC = xsltproc
endif

DBTOEPUB ?= dbtoepub

Why not XSLTPROC ?= xsltproc and the like?

--
Fabien.


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

Re: [HACKERS] [TODO] Process pg_hba.conf keywords as case-insensitive

[Christoph Berg]

Re: Heikki Linnakangas 2014-08-21 <53f5a2d6.2050...@vmware.com>
> >1) database and role names behave similar to SQL identifiers (case-sensitive 
> >/ case-folding).
> >
> >2) users and user-groups only requires special handling and behavior as 
> >follows
> > Normal user :
> >   A. unquoted ( USER ) will be treated as user ( downcase ).
> >   B. quoted  ( "USeR" )  will be treated as USeR (case-sensitive).

> With this patch, database (and role?) names are compared case-insensitively.
> For example:
> 
> local  MixedDB all trust
> local  mixedDB all reject
> 
> psql -d "mixedDB"
> psql (9.5devel)
> Type "help" for help.
> 
> mixedDB=#
> 
> That connection should've matched that 2nd line, and be rejected.

Actually it should have matched neither, as both lines will get folded
downcase:

local  mixeddb all trust
local  mixeddb all reject

Christoph
-- 
c...@df7cb.de | http://www.df7cb.de/


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

[Christoph Berg]

Re: Thom Brown 2014-08-20 

> "ERROR:  table test is not permanent"
> 
> Perhaps this would be better as "table test is unlogged" as "permanent"
> doesn't match the term used in the DDL syntax.

I was also wondering that, but then figured that when ALTER TABLE SET
UNLOGGED is invoked on temp tables, the error message "is not
permanent" was correct while the apparent opposite "is unlogged" is
wrong.

Christoph
-- 
c...@df7cb.de | http://www.df7cb.de/


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

[HACKERS] [TODO] Track number of files ready to be archived in pg_stat_archiver

[Julien Rouhaud]

Hello,

Attached patch implements the following TODO item :

Track number of WAL files ready to be archived in pg_stat_archiver

However, it will track the total number of any file ready to be
archived, not only WAL files.

Please let me know what you think about it.

Regards.
-- 
Julien Rouhaud
http://dalibo.com - http://dalibo.org
*** a/doc/src/sgml/monitoring.sgml
--- b/doc/src/sgml/monitoring.sgml
***
*** 728,733  postgres   27093  0.0  0.0  30096  2752 ?Ss   11:34   0:00 postgres: ser
--- 728,738 
Time of the last failed archival operation
   
   
+   ready_count
+   bigint
+   Number of files waiting to be archived
+  
+  
stats_reset
timestamp with time zone
Time at which these statistics were last reset
*** a/src/backend/access/transam/xlogarchive.c
--- b/src/backend/access/transam/xlogarchive.c
***
*** 24,29 
--- 24,30 
  #include "access/xlog_internal.h"
  #include "miscadmin.h"
  #include "postmaster/startup.h"
+ #include "pgstat.h"
  #include "replication/walsender.h"
  #include "storage/fd.h"
  #include "storage/ipc.h"
***
*** 539,544  XLogArchiveNotify(const char *xlog)
--- 540,548 
  	/* Notify archiver that it's got something to do */
  	if (IsUnderPostmaster)
  		SendPostmasterSignal(PMSIGNAL_WAKEN_ARCHIVER);
+ 
+ 	/* Tell the collector about a new file waiting to be archived */
+ 	pgstat_send_archiver(xlog, ARCH_READY);
  }
  
  /*
*** a/src/backend/catalog/system_views.sql
--- b/src/backend/catalog/system_views.sql
***
*** 697,702  CREATE VIEW pg_stat_archiver AS
--- 697,703 
  s.failed_count,
  s.last_failed_wal,
  s.last_failed_time,
+ s.ready_count,
  s.stats_reset
  FROM pg_stat_get_archiver() s;
  
*** a/src/backend/postmaster/pgarch.c
--- b/src/backend/postmaster/pgarch.c
***
*** 491,497  pgarch_ArchiverCopyLoop(void)
   * Tell the collector about the WAL file that we successfully
   * archived
   */
! pgstat_send_archiver(xlog, false);
  
  break;			/* out of inner retry loop */
  			}
--- 491,497 
   * Tell the collector about the WAL file that we successfully
   * archived
   */
! pgstat_send_archiver(xlog, ARCH_SUCCESS);
  
  break;			/* out of inner retry loop */
  			}
***
*** 501,507  pgarch_ArchiverCopyLoop(void)
   * Tell the collector about the WAL file that we failed to
   * archive
   */
! pgstat_send_archiver(xlog, true);
  
  if (++failures >= NUM_ARCHIVE_RETRIES)
  {
--- 501,507 
   * Tell the collector about the WAL file that we failed to
   * archive
   */
! pgstat_send_archiver(xlog, ARCH_FAIL);
  
  if (++failures >= NUM_ARCHIVE_RETRIES)
  {
*** a/src/backend/postmaster/pgstat.c
--- b/src/backend/postmaster/pgstat.c
***
*** 36,41 
--- 36,42 
  #include "access/transam.h"
  #include "access/twophase_rmgr.h"
  #include "access/xact.h"
+ #include "access/xlog_internal.h"
  #include "catalog/pg_database.h"
  #include "catalog/pg_proc.h"
  #include "lib/ilist.h"
***
*** 3084,3094  pgstat_send(void *msg, int len)
   * pgstat_send_archiver() -
   *
   *	Tell the collector about the WAL file that we successfully
!  *	archived or failed to archive.
   * --
   */
  void
! pgstat_send_archiver(const char *xlog, bool failed)
  {
  	PgStat_MsgArchiver msg;
  
--- 3085,3096 
   * pgstat_send_archiver() -
   *
   *	Tell the collector about the WAL file that we successfully
!  *	archived or failed to archive, or the new file waiting
!  *	to be archived.
   * --
   */
  void
! pgstat_send_archiver(const char *xlog, ArchiverReason reason)
  {
  	PgStat_MsgArchiver msg;
  
***
*** 3096,3102  pgstat_send_archiver(const char *xlog, bool failed)
  	 * Prepare and send the message
  	 */
  	pgstat_setheader(&msg.m_hdr, PGSTAT_MTYPE_ARCHIVER);
! 	msg.m_failed = failed;
  	strncpy(msg.m_xlog, xlog, sizeof(msg.m_xlog));
  	msg.m_timestamp = GetCurrentTimestamp();
  	pgstat_send(&msg, sizeof(msg));
--- 3098,3104 
  	 * Prepare and send the message
  	 */
  	pgstat_setheader(&msg.m_hdr, PGSTAT_MTYPE_ARCHIVER);
! 	msg.m_reason = reason;
  	strncpy(msg.m_xlog, xlog, sizeof(msg.m_xlog));
  	msg.m_timestamp = GetCurrentTimestamp();
  	pgstat_send(&msg, sizeof(msg));
***
*** 3921,3927  pgstat_read_statsfiles(Oid onlydb, bool permanent, bool deep)
  	/*
  	 * Try to open the stats file. If it doesn't exist, the backends simply
  	 * return zero for anything and the collector simply starts from scratch
! 	 * with empty counters.
  	 *
  	 * ENOENT is a possibility if the stats collector is not running or has
  	 * not yet written the stats file the first time.  Any other failure
--- 3923,3930 
  	/*
  	 * Try to open the stats file. If it doesn't exist, the backends simply
  	 * return zer

Re: [HACKERS] 9.5: Memory-bounded HashAgg

[Jeff Davis]

On Wed, 2014-08-20 at 14:32 -0400, Robert Haas wrote:
> Well, I think you're certainly right that a hash table lookup is more
> expensive than modulo on a 32-bit integer; so much is obvious.  But if
> the load factor is not too large, I think that it's not a *lot* more
> expensive, so it could be worth it if it gives us other advantages.
> As I see it, the advantage of Jeff's approach is that it doesn't
> really matter whether our estimates are accurate or not.  We don't
> have to decide at the beginning how many batches to do, and then
> possibly end up using too much or too little memory per batch if we're
> wrong; we can let the amount of memory actually used during execution
> determine the number of batches.  That seems good.  Of course, a hash
> join can increase the number of batches on the fly, but only by
> doubling it, so you might go from 4 batches to 8 when 5 would really
> have been enough.  And a hash join also can't *reduce* the number of
> batches on the fly, which might matter a lot.  Getting the number of
> batches right avoids I/O, which is a lot more expensive than CPU.

My approach uses partition counts that are powers-of-two also, so I
don't think that's a big differentiator. In principle my algorithm could
adapt to other partition counts, but I'm not sure how big of an
advantage there is.

I think the big benefit of my approach is that it doesn't needlessly
evict groups from the hashtable. Consider input like 0, 1, 0, 2, ..., 0,
N. For large N, if you evict group 0, you have to write out about N
tuples; but if you leave it in, you only have to write out about N/2
tuples. The hashjoin approach doesn't give you any control over
eviction, so you only have about 1/P chance of saving the skew group
(where P is the ultimate number of partitions). With my approach, we'd
always keep the skew group in memory (unless we're very unlucky, and the
hash table fills up before we even see the skew value).

Regards,
Jeff Davis




-- 
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] [TODO] Process pg_hba.conf keywords as case-insensitive

[Heikki Linnakangas]

On 07/23/2014 09:14 AM, Viswanatham kirankumar wrote:

On 16 July 2014 23:12, Tom Lane wrote

Christoph Berg  writes:
Re: Viswanatham kirankumar 2014-07-16


Attached patch is implementing following TODO item Process
pg_hba.conf keywords as case-insensitive



Hmm. I see a case for accepting "ALL" (as in hosts.allow(5)), so +1 on
that, but I don't think the other keywords like "host" and "peer"
should be valid in upper case.



I think the argument was that SQL users are accustomed to thinking that 
keywords are
case-insensitive.  It makes sense to me that we should adopt that same 
convention in pg_hba.conf.



Re-reading the original thread, there was also concern about whether
we should try to make quoting/casefolding behave more like it does in SQL,
specifically for matching pg_hba.conf items to SQL identifiers (database and 
role names).
This patch doesn't seem to have addressed that part of it, but I think we need 
to think those
things through before we just do a blind s/strcmp/pg_strcasecmp/g.  Otherwise 
we might
find that we've added ambiguity that will give us trouble when we do try to fix 
that.


I had updated as per you review comments

1) database and role names behave similar to SQL identifiers (case-sensitive / 
case-folding).

2) users and user-groups only requires special handling and behavior as follows
 Normal user :
   A. unquoted ( USER ) will be treated as user ( downcase ).
   B. quoted  ( "USeR" )  will be treated as USeR (case-sensitive).
   C. quoted ( "+USER" ) will be treated as normal user +USER (i.e. will 
not be considered as user-group) and case-sensitive as string is quoted.
User Group :
   A. unquoted ( +USERGROUP ) will be treated as +usergruop ( downcase ).
   B. plus quoted ( +"UserGROUP"  ) will be treated as +UserGROUP 
(case-sensitive).

3) Host name is not a SQL object so it will be treated as case-sensitive
except for all, samehost, samenet are considered as keywords.
For these user need to use quotes to differentiate between hostname and 
keywords.

4) All the fixed keywords mention in pg_hba.conf and Client Authentication 
section will be considered as keywords
 Eg: host, local, hostssl etc..



With this patch, database (and role?) names are compared 
case-insensitively. For example:


local  MixedDB all trust
local  mixedDB all reject

psql -d "mixedDB"
psql (9.5devel)
Type "help" for help.

mixedDB=#

That connection should've matched that 2nd line, and be rejected.

PS. Please update the docs.

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

> "Heikki" == Heikki Linnakangas  writes:

 > On 08/13/2014 09:43 PM, Atri Sharma wrote:
 >> Sorry, forgot to attach the patch for fixing cube in contrib,
 >> which breaks since we now reserve "cube" keyword. Please find
 >> attached the same.

 Heikki> Ugh, that will make everyone using the cube extension
 Heikki> unhappy. After this patch, they will have to quote contrib's
 Heikki> cube type and functions every time.

 Heikki> I think we should bite the bullet and rename the extension,

I agree, the contrib/cube patch as posted is purely so we could test
everything without having to argue over the new name first.  (And it
is posted separately from the main patch because of its length and
utter boringness.)

However, even if/when a new name is chosen, there's the question of
how to make the upgrade path easiest. Once CUBE is reserved,
up-to-date pg_dump will quote all uses of the "cube" type and function
when dumping an older database (except inside function bodies of
course), so there may be merit in keeping a "cube" domain over the new
type, and maybe also merit in keeping the extension name.

So what's the new type name going to be? cuboid? hypercube?
geometric_cube?  n_dimensional_box?

-- 
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] Specifying the unit in storage parameter

[Michael Paquier]

On Fri, Aug 8, 2014 at 12:32 PM, Fujii Masao  wrote:
> This is not user-friendly. I'd like to propose the attached patch which
> introduces the infrastructure which allows us to specify the unit when
> setting INTEGER storage parameter like autovacuum_vacuum_cost_delay.
> Comment? Review?
This patch makes autovacuum_vacuum_cost_delay more consistent with
what is at server level. So +1.

Looking at the patch, the parameter "fillfactor" in the category
RELOPT_KIND_HEAP (the first element in intRelOpts of reloptions.c) is
not updated with the new field. It is only a one-line change.
@@ -97,7 +97,7 @@ static relopt_int intRelOpts[] =
"Packs table pages only to this percentage",
RELOPT_KIND_HEAP
},
-   HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100
+   HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100, 0
},

Except that, I tested as well the patch and it works as expected. The
documentation, as well as the regression tests remain untouched, but I
guess that this is fine (not seeing similar tests in regressions, and
documentation does not specify the unit for a given parameter).

Regards,
-- 
Michael


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

Re: [HACKERS] pg_xlogdump --stats

[Heikki Linnakangas]

On 07/07/2014 10:32 PM, Abhijit Menon-Sen wrote:

At 2014-07-07 09:48:44 +0200, and...@2ndquadrant.com wrote:


I'd suggest only defining INT64_MODIFIER here and build INT64_FORMAT,
UINT64_FORMAT ontop, in c.h.


Oh, I see. I'm sorry, I misread your earlier suggestion. Regenerated
patches attached. Is this what you had in mind?


Committed the patch to add INT64_MODIFIER, with minor fixes.

The new rm_identify method needs to be documented. Not sure where; 
surprisingly I can't find any documentation on the existing methods 
either. Perhaps add comments to the RmgrData struct, explaining all of 
the methods. In particular, should give guidelines on what output 
belongs in rm_desc and what in rm_identify.


I think the names that rm_identify returns should match those that the 
rm_desc functions print. As the patch stands, for example when heap_desc 
prints out something like:


hot_update(init): xmax 123; new tid 1/2 xmax 456

The corresponding rm_identify output is:

HOT_UPDATE+INIT

It would be better to spell them the same, so that when you look at the 
summary stats and the raw pg_xlogdump output, you can tell which records 
contributed to which summary line.


- Heikki



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