Re: [HACKERS] BRIN indexes - TRAP: BadArgument

[Michael Paquier]

On Sat, Nov 8, 2014 at 5:40 PM, David Rowley  wrote:
> Please find attached another small fix. This time it's just a small typo in
> the README, and just some updates to some, now outdated docs.
Speaking about the feature... The index operators are still named with
"minmax", wouldn't it be better to switch to "brin"?
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] Add CREATE support to event triggers

[Robert Haas]

On Sat, Nov 8, 2014 at 6:24 PM, Andres Freund  wrote:
> On 2014-11-08 17:49:30 -0500, Robert Haas wrote:
>> Patch 2 adds support for GRANT and REVOKE to the event trigger
>> mechanism.  I wonder if it's a bad idea to make the
>> ddl_command_start/end events fire for DCL.  We discussed a lot of
>> these issues when this patch originally went in, and I think it'd be
>> worth revisiting that discussion.
>
> Well, it doesn't generally support it for all GRANT statement, but just
> for ones that are database local. I think that mostly skirts the
> problems from the last round of discussion. But I only vaguely remember
> them.

The issue I was alluding to was terminological: it's not clear that
GRANT and REVOKE should be called DDL rather than DCL, although we do
have precedent in some of the logging settings.

The other issue I remember is that if you have a separate event
trigger for GRANT/REVOKE, you can expose fields like operation/object
type/permission/target.  That's much harder to do for an event trigger
that's very broad.

But anyway, I think it would be worth going back and looking at the
previous discussion.

> As I said, there was clear interest at at least two of the cluster
> hackers meetings...

We need to get some better data here.

But again, my core concern is that we have no good way to test this
code for bugs, including of omission, without which I think we will be
very sad.

-- 
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] Add CREATE support to event triggers

[Robert Haas]

On Sat, Nov 8, 2014 at 4:35 PM, Andres Freund  wrote:
>> > I don't understand why this is particularly difficult to regresssion
>> > test. It actually is comparatively simple?
>>
>> If it is, great.  I previously wrote this email:
>>
>> http://www.postgresql.org/message-id/CA+TgmoZ=vzrijmxlkqi_v0jg4k4leapmwusc6rwxs5mquxu...@mail.gmail.com
>>
>> Alvaro came up with a way of addressing the second point I raised
>> there, which I'm quite pleased about, but AFAIK there's been no
>> progress on the first one.  Maybe I missed something?
>
> I unfortunately don't think so. And that sounds like a completely
> reasonable criticism.

I'm glad you agree.  If you can find a way to address that point, I
can live with the rest of it.  I don't think it's dumb to be concerned
about features that increase the cost of adding more features.  But
what really concerns me is that that code won't be well-tested, and if
there are cases missing or somebody forgets to do it altogether, it's
very likely that we won't notice.  That seems like a huge problem from
where I sit.

-- 
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] Add CREATE support to event triggers

[Andres Freund]

On 2014-11-08 17:49:30 -0500, Robert Haas wrote:
> Patch 2 adds support for GRANT and REVOKE to the event trigger
> mechanism.  I wonder if it's a bad idea to make the
> ddl_command_start/end events fire for DCL.  We discussed a lot of
> these issues when this patch originally went in, and I think it'd be
> worth revisiting that discussion.

Well, it doesn't generally support it for all GRANT statement, but just
for ones that are database local. I think that mostly skirts the
problems from the last round of discussion. But I only vaguely remember
them.

> Patch 5 depends on patch 4, which does a bunch of things.  I *think*
> the upshot of patch 5 is that we're not currently firing event
> triggers in some situations where we should, in which case +1 for
> fixing that.  It would help if there were a real commit message,
> and/or some more contents, and I think it could be more completely
> disentangled from patch 4.

> > We could just integrate those parts, and be done with it. But would that
> > actually be a good thing for the community? Then slony needs to do it
> > and potentially others as well? Then auditing can't use it? Then
> > potential schema tracking solutions can't use it?
> 
> Do you think Slony is really going to use this?

There was a fair amount noise about it at one of the past cluster
hackers thingies.

> I guess we can let
> the Slony guys speak for themselves, but I've been skeptical since day
> one that this is the best way to do DDL replication, and I still am.

Well, I've yet to hear anything that's realistic otherwise.

> There are lots of ways that a replicated DDL statement can fail on the
> replicas, and what are you going to do then?  It's too late to show
> the user the error message, so you can throw it in a log someplace and
> hope that somebody notices, but that's it.

Sure. And absolutely the same is true for DML. And the lack of DDL
integration makes it happen really rather frequently...

> It makes a lot more sense
> to me to use some kind of a tool that applies the DDL in a coordinated
> fashion on all nodes - or even just do it manually, since it might
> very well be desirable to take the lock on different nodes at widely
> different times, separated by a switchover.  I certainly think there's
> a use-case for what you're trying to do here, but I don't think it'll
> be right for everyone.

I agree that it's not the right.

> Certainly, if the Slony guys - or some other team building an
> out-of-core replication solutions says, hey, we really want this in
> core, that would considerably strengthen the argument for putting it
> there.  But I haven't heard anyone say that yet - unlike logical
> decoding, were we did have other people expressing clear interest in
> using it.

As I said, there was clear interest at at least two of the cluster
hackers meetings...

> > There've been people for a long while asking about triggers on catalogs
> > for that purpose. IIRC Jan was one of them.
> 
> My impression, based on something Christopher Brown said a few years
> ago, is that Slony's DDL trigger needs are largely satisfied by the
> existing event trigger stuff.  It would be helpful to get confirmation
> as to whether that's the case.

Oh, that's contrary to what I remember, but yes, it'd be interesting to
hear about htat.

> >> On the flip side, the *cost* of pushing it into core is that
> >> it now becomes the PostgreSQL's community problem to update the
> >> deparsing code every time the grammar changes.  That cost-benefit
> >> trade-off does not look favorable to me, especially given the absence
> >> of any kind of robust testing framework.
> >
> > I agree that there should be testing in this.
> >
> > But I think you're quite overstating the effort of maintaining
> > this. Looking at gram.y between the stamping of 9.3 devel and 9.4 devel
> > for commits that'd require DDL deparsing changes:
> > * ALTER TABLE .. ALTER CONSTRAINT: About 7 lines for the deparse code,
> >   out of a ~350 line patch
> > * REFRESH MATERIALIZED VIEW ... CONCURRENTLY: About 1 line for deparse,
> >   out of ~700
> > * WITH CHECK OPTION for views: About 3 lines for deparse, out of ~1300
> > * REPLICA IDENTITY: About 24 lines for deparse, out of ~950
> > * ALTER TABLESPACE MOVE: About 20 lines for deparse, out of
> >   ~340. Although that patch was essentially scrapped afterwards. And
> >   rewritten differently.
> > * CREATE TABLESPACE ... WITH ...:  Not supported by event triggers right
> >   now as it's a global object.
> >
> > That's really not a whole lot.
> 
> Those are pretty minor syntax patches, though.

Sure, but these are all the ones from the stamping of 9.3devel to
9.4devel. I wanted to look at a release cycle, and that seemed the
easiest way to do it. I didn't choose that cycle, because I knew it was
"light" on ddl, I chose it because it was the last complete one.

> I think it's more helpful to look at things like the row-level
> security stuff, or materialized views per se, or DDL suppor

Re: [HACKERS] Add CREATE support to event triggers

[Robert Haas]

On Sat, Nov 8, 2014 at 1:05 PM, Andres Freund  wrote:
>> There's nothing to keep you from exposing the parse trees to
>> C functions that can live in an extension, and you can do all of this
>> deparsing there.
>
> Not really. There's some core functions that need to be touched. Like
> most of the stuff in patches 1,2,3,5,16 does.

Patch 1 is fine.  We've done similar things in the past (cf.
c504513f83a9ee8dce4a719746ca73102cae9f13,
82b1b213cad3a69cf5f3dfaa81687c14366960fc).  I'd just commit that.

Patch 2 adds support for GRANT and REVOKE to the event trigger
mechanism.  I wonder if it's a bad idea to make the
ddl_command_start/end events fire for DCL.  We discussed a lot of
these issues when this patch originally went in, and I think it'd be
worth revisiting that discussion.

Patch 3 is the same kind of idea as patch 2, only for COMMENT.

Patch 5 depends on patch 4, which does a bunch of things.  I *think*
the upshot of patch 5 is that we're not currently firing event
triggers in some situations where we should, in which case +1 for
fixing that.  It would help if there were a real commit message,
and/or some more contents, and I think it could be more completely
disentangled from patch 4.

Patch 16 again contains almost no comments and no description of its
specific purpose, but it appears to be similar to patch 1, so probably
mostly uncontroversial.

> We could just integrate those parts, and be done with it. But would that
> actually be a good thing for the community? Then slony needs to do it
> and potentially others as well? Then auditing can't use it? Then
> potential schema tracking solutions can't use it?

Do you think Slony is really going to use this?  I guess we can let
the Slony guys speak for themselves, but I've been skeptical since day
one that this is the best way to do DDL replication, and I still am.
There are lots of ways that a replicated DDL statement can fail on the
replicas, and what are you going to do then?  It's too late to show
the user the error message, so you can throw it in a log someplace and
hope that somebody notices, but that's it.  It makes a lot more sense
to me to use some kind of a tool that applies the DDL in a coordinated
fashion on all nodes - or even just do it manually, since it might
very well be desirable to take the lock on different nodes at widely
different times, separated by a switchover.  I certainly think there's
a use-case for what you're trying to do here, but I don't think it'll
be right for everyone.

Certainly, if the Slony guys - or some other team building an
out-of-core replication solutions says, hey, we really want this in
core, that would considerably strengthen the argument for putting it
there.  But I haven't heard anyone say that yet - unlike logical
decoding, were we did have other people expressing clear interest in
using it.

> There've been people for a long while asking about triggers on catalogs
> for that purpose. IIRC Jan was one of them.

My impression, based on something Christopher Brown said a few years
ago, is that Slony's DDL trigger needs are largely satisfied by the
existing event trigger stuff.  It would be helpful to get confirmation
as to whether that's the case.

>> On the flip side, the *cost* of pushing it into core is that
>> it now becomes the PostgreSQL's community problem to update the
>> deparsing code every time the grammar changes.  That cost-benefit
>> trade-off does not look favorable to me, especially given the absence
>> of any kind of robust testing framework.
>
> I agree that there should be testing in this.
>
> But I think you're quite overstating the effort of maintaining
> this. Looking at gram.y between the stamping of 9.3 devel and 9.4 devel
> for commits that'd require DDL deparsing changes:
> * ALTER TABLE .. ALTER CONSTRAINT: About 7 lines for the deparse code,
>   out of a ~350 line patch
> * REFRESH MATERIALIZED VIEW ... CONCURRENTLY: About 1 line for deparse,
>   out of ~700
> * WITH CHECK OPTION for views: About 3 lines for deparse, out of ~1300
> * REPLICA IDENTITY: About 24 lines for deparse, out of ~950
> * ALTER TABLESPACE MOVE: About 20 lines for deparse, out of
>   ~340. Although that patch was essentially scrapped afterwards. And
>   rewritten differently.
> * CREATE TABLESPACE ... WITH ...:  Not supported by event triggers right
>   now as it's a global object.
>
> That's really not a whole lot.

Those are pretty minor syntax patches, though.  I think it's more
helpful to look at things like the row-level security stuff, or
materialized views per se, or DDL support for collations.  In any
case, the additional coding burden concerns me less than the
additional testing burden - but there's another email further down
that's more to that specific point, so I'll stop here.

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

Re: [HACKERS] Support for detailed description of errors cased by trigger-violations

[Tom Lane]

Andreas Joseph Krogh  writes:
> Hi.   When working with Oracle it is possible to catch constraint-violations 
> caused by triggers using JDBC, but it seems this isn't possible using PG, see 
> this thread: 
> https://github.com/impossibl/pgjdbc-ng/issues/111#issuecomment-62276464

I'm not exactly following the point.  The complaint seems to be that

RAISE EXCEPTION 'ID must be less then 10';

doesn't send anything except the given primary message and a generic
SQLSTATE.  Well, duh: it's not supposed to.  There are a bunch of options
you can supply in RAISE to populate additional fields of the error report.
For example, you could add

USING SCHEMA = TG_TABLE_SCHEMA, TABLE = TG_TABLE_NAME

if you wanted the report to name the table the trigger is attached to.

So it seems to me this is lazy plpgsql programming, not a missing feature.
It would only be a missing feature if you think plpgsql should try to
auto-populate these fields; but I'd be against that because it would
require too many assumptions about exactly what the function might be
complaining about.

regards, tom lane


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

Re: [HACKERS] pg_multixact not getting truncated

[Andres Freund]

On 2014-11-08 12:10:48 -0800, Josh Berkus wrote:
> On 11/07/2014 05:29 PM, Alvaro Herrera wrote:
> > Josh Berkus wrote:
> >> Of course, this will lead to LOTs of additional vacuuming ...
> > 
> > There's a trade-off here: more vacuuming I/O usage for less disk space
> > used.  How stressed your customers really are about 1 GB of disk space?
> 
> These customers not so much.  The users I encountered on chat whose
> pg_multixact was over 20GB, and larger than their database?  Lots.


> On 11/08/2014 03:54 AM, Andres Freund wrote:
> > On 2014-11-07 17:20:44 -0800, Josh Berkus wrote:
> >> So the basic problem is that multixact files are just huge, with an
> >> average of 35 bytes per multixact?
> >
> > Depends on the concurrency. The number of members is determined by the
> > number of xacts concurrenly locking a row..
> 
> Yeah, that leads to some extreme inflation for databases where FK
> conflicts are common though.

On the other hand, those are the ones benefitting most from the gain in
concurrency.

> On 11/08/2014 03:54 AM, Andres Freund wrote:
> > On 2014-11-07 17:20:44 -0800, Josh Berkus wrote:
> >> Of course, this will lead to LOTs of additional vacuuming ...
> >
> > Yes. And that's likely to cause much, much more grief.
> >
> > Also. Didn't you just *vehemently* oppose making these values tunable at
> > all?
> 
> Yes, I opposed adding a *user* tunable with zero information on how it
> should be tuned or why. I always do and always will.

I think that's primarily naive. We don't always *have* that knowledge
ahead of time. There's interactions in the real world that we are not
able to predict. And people are usually more happy to find that their
problem can be fixed by tuning a somewhat obscure GUC than having to
patch their server or, much worse, upgrade to a newer major version that
just came out. *No* user knows all our GUCs, even the really experienced
ones only know half or so. And that's not because there's too
many. Unless there are only three, they'll never.

> I also think our
> defaults for multixact freezing should be tied to the ones for xid
> freezing, and should not by default be completely independent numbers;

I think it'd be a good idea to tune them more automatedly in the
future. But I think the current situation where you can vastly increase
multivacuum_freeze_max_age while having
multivacuum_multixact_freeze_max_age is *much* more useful in practice
than when they always were the same.

> I'm still not convinced that it makes sense to have a separate multixact
> threshold at all **since the same amount of vacuuming needs to be done
> regardless of whether we're truncating xids or mxids**.

That's just plain wrong. The growth rate of one can be nearly
independent of the other. It can e.g. be very sensible to have a huge
xid freeze limit, but a much smaller multixact limit.

> Certainly when I play with tuning this for customers, I'm going to lower
> vacuum_freeze_table_age as well.

I'm these days suggesting that people should add manual vacuuming for
"older" relations during off peak hours on busy databases. There's too
many sites which service degrades noticeably during a full table vacuum.

If you actually mean autovacuum_freeze_max_age - I don't generally
agree. It very often can be a good idea to significantly increase it.

Greetings,

Andres Freund

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


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

Re: [HACKERS] Add CREATE support to event triggers

[Andres Freund]

On 2014-11-08 10:42:15 -0500, Robert Haas wrote:
> On Sat, Nov 8, 2014 at 4:37 AM, Andres Freund  wrote:
> > I don't understand why this is particularly difficult to regresssion
> > test. It actually is comparatively simple?
> 
> If it is, great.  I previously wrote this email:
> 
> http://www.postgresql.org/message-id/CA+TgmoZ=vzrijmxlkqi_v0jg4k4leapmwusc6rwxs5mquxu...@mail.gmail.com
> 
> Alvaro came up with a way of addressing the second point I raised
> there, which I'm quite pleased about, but AFAIK there's been no
> progress on the first one.  Maybe I missed something?

I unfortunately don't think so. And that sounds like a completely
reasonable criticism.

> Just to illustrate the point, consider the CREATE TABLE name OF type
> syntax that Peter added a few years ago.   That patch
> (e7b3349a8ad7afaad565c573fbd65fb46af6abbe) had the following impact on
> gram.y:
> 
>  src/backend/parser/gram.y |   56 
> ++---
>  1 file changed, 43 insertions(+), 13 deletions(-)

> Now let's have a look at what impact it has on the deparsing code.
> Patch 6 has deparse_ColumnDef_Typed from lines 134 to 193.  There's 3
> or so lines in deparseTableElements that decide whether to call it.
> Patch 8 has more handling for this case, lines 439 to 443 and 463 to
> 490.  So, if this feature had been committed before TABLE OF, it would
> have needed about 100 lines of net new code to handle this case -
> exclusive of refactoring.  The actual size of the patch would probably
> have been modestly larger than that, because some code would need to
> be reindented when it got iffed out, and quite possibly some
> rearrangement would have been needed.  But even ignoring all that, the
> deparse footprint of the patch would have been MORE THAN TWICE the
> parser footprint.

Well, you disregarded the related costs of adjusting pg_dump et al, as
Tom mentioned, that's a significant part. And yes, there's some
additions that aren't entirely trivial to add. But the majority of
additions are pretty simple.

> I think that's going to be typical; and I think the deparse code is
> going to be significantly more labor-intensive to write than bison
> productions are.  Do you really think that's not going to be a burden?

I've looked into a fair number of cases and almost all are vastly
simpler than this. Most of the DDL changes that have been done lately
are things like adding IF NOT EXISTS somewhere; expanding existing
syntax for new types of objects (ALTER ... RENAME for foreign servers,
wrappers; LABEL for new types).

I think given the complexity of newly added features the overhead of
adding deparsing code isn't all that high.

> > Being able to replicate DDL is a feature wish that has been around
> > pretty much since the inception of trigger based replication
> > solution. It's not some current fancy. And the json stuff only got there
> > because people wanted some way to manipulate the names in the replicated
> > - which this abstraction provides them with.
> 
> I understand that being able to replicate DDL is an important need,
> and there may be no better way to do it than this.  But that doesn't
> mean that this code is going to be bug-free or easy to maintain.

Agreed. There's definitely no free lunch (here). This has been discussed
more than once, and so far I've read anything superior that also has a
chance of handling ALTER.

> >> may never be, and which I strongly suspect may be too clever by half.
> >> Once you've committed it, you're going to move onto other things and
> >> leave it to everyone who comes after to try to maintain it.  I bet
> >> we'll still be running into bugs and half-implemented features five
> >> years from now, and maybe in ten.  Ramming through special-purpose
> >> infrastructure instead of generalizing it is merely icing on the cake,
> >> but it's still moving in the wrong direction.
> >
> > You're just as much to blame for not writing a general json abstraction
> > layer for EXPLAIN. I'd say that's not much blame, but still, there's
> > really not much difference there.
> 
> Well, this patch is series is at least an order of magnitude larger,
> and it's apparently doing significantly more complex stuff with JSON,
> because the explain.c patch just writes it out into a StringInfo.

And this code builds up the data in memory and then calls into the json
code to build the json value. And to parse json it uses the functions
underlying the SQL accessors.

There's one function it should either not need anymore (dequote_jsonval)
by using json_object_field_text instead of json_object_field or by
exposing dequote_jsonval's functionality from json.c.

I think the json angle here is a red herring. Sure, a nicer API could
save some FunctionCallInfoData boilerplate, but that's pretty much it.

Greetings,

Andres Freund

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


-- 
Sent via pgsql-hackers maili

Re: [HACKERS] row_to_json bug with index only scans: empty keys!

[Andrew Dunstan]

On 11/08/2014 04:19 PM, Ross Reedstrom wrote:

I've no opinion on the not respecting aliases aspect of this, but both
the hstore and json emtpy keys case breaks the format: it's duplicate keys
that collapse to a single value, and expected keynames are missing.

The insidious bit about this bug though is that it works fine during testing
with the developers typically small data sets. It's only triggered in my case
when we the plan switches to index-only. Even an index scan works fine. I can't
imagine that there is code out there that _depends_ on this behavior. Just as
likely to me are that there exist systems that just have "can't reproduce" bugs
that would be fixed by this.





No, I can't imagine it either - it's utterly broken. That's the piece 
that Tom is proposing to fix on the back branches. AIUI.


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] row_to_json bug with index only scans: empty keys!

[Ross Reedstrom]

I've no opinion on the not respecting aliases aspect of this, but both
the hstore and json emtpy keys case breaks the format: it's duplicate keys
that collapse to a single value, and expected keynames are missing.

The insidious bit about this bug though is that it works fine during testing
with the developers typically small data sets. It's only triggered in my case
when we the plan switches to index-only. Even an index scan works fine. I can't
imagine that there is code out there that _depends_ on this behavior. Just as
likely to me are that there exist systems that just have "can't reproduce" bugs
that would be fixed by this.

Ross


On Sat, Nov 08, 2014 at 01:09:23PM -0500, Tom Lane wrote:
> Oh, some more fun: a RowExpr that's labeled with a named composite type
> (rather than RECORD) has the same issue of not respecting aliases.
> Continuing previous example with table "foo":
> 
> regression=# create view vv as select * from foo;
> CREATE VIEW
> regression=# select row_to_json(q) from vv q;
>row_to_json   
> -
>  {"f1":1,"f2":2}
> (1 row)
> 
> regression=# select row_to_json(q) from vv q(a,b);
>row_to_json   
> -
>  {"f1":1,"f2":2}
> (1 row)
> 
> So that's another case we probably want to change in HEAD but not the back
> branches.
> 
>   regards, tom lane
> 

-- 
Ross Reedstrom, Ph.D. reeds...@rice.edu
Systems Engineer & Admin, Research Scientistphone: 713-348-6166
Connexions  http://cnx.orgfax: 713-348-3665
Rice University MS-375, Houston, TX 77005
GPG Key fingerprint = F023 82C8 9B0E 2CC6 0D8E  F888 D3AE 810E 88F0 BEDE


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

Re: [HACKERS] Repeatable read and serializable transactions see data committed after tx start

[Álvaro Hernández Tortosa]

On 07/11/14 22:02, Greg Sabino Mullane wrote:

-BEGIN PGP SIGNED MESSAGE-
Hash: RIPEMD160


Kevin Grittner wrote:

I think most people have always assumed that
BEGIN starts the transaction and that is the point at
which the snapshot is obtained.

But there is so much evidence to the contrary.  Not only does the
*name* of the command (BEGIN or START) imply a start, but
pg_stat_activity shows the connection "idle in transaction" after
the command (and before a snapshot is acquired)

Er...I think we are arguing the same thing here. So no contrary
needed? :)


So do we agree to fix the docs? ^_^




Why?  This "fix" might not deal with the bigger issues that I
discussed, like that the later-to-start and
later-to-acquire-a-snapshot transaction might logically be first in
the apparent order of execution.  You can't "fix" that without a
lot of blocking -- that most of us don't want.

Right, which is why the suggestion of a user-controllable switch,
that defaults to the current behavior, seems an excellent compromise.


I also think so. It's backwards-compatible and opt-in. It also 
makes the documentation very clear, as there is an specific option for this.





Depending on *why* they think this is important, they might need to
be acquiring various locks to prevent behavior they don't want, in which case
having acquired a snapshot at BEGIN would be exactly the *wrong*
thing to do.  The exact nature of the problem we're trying to solve
here does matter.

I cannot speak to the OP, but I also do not think we should try and
figure out every possible scenario people may have. Certainly the
long-standing documentation bug may have caused some unexpected or
unwanted behavior, so let's start by fixing that.


+1


Tom Lane wrote:

Another thing that I think hasn't been mentioned in this thread is
that we used to have severe problems with client libraries that like
to issue BEGIN and then go idle until they have something to do.
Which, for some reason, is a prevalent behavior.

I'm not advocating changing the default behavior, but I would not want
to see bad client libraries used a reason for any change we make. Clients
should not be doing this, period, and there is no reason for us to
support that.


If the "IMMEDIATE FREEZE" mode is not the default, as I suggested, 
it shouldn't introduce any problem with past code.


Regards,

Álvaro



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

Re: [HACKERS] Repeatable read and serializable transactions see data committed after tx start

[Álvaro Hernández Tortosa]

On 06/11/14 15:00, Kevin Grittner wrote:

Álvaro Hernández Tortosa  wrote:


 There has been two comments which seem to state that changing this
may introduce some performance problems and some limitations when you
need to take out some locks. I still believe, however, that current
behavior is confusing for the user. Sure, one option is to patch the
documentation, as I was suggesting.

Yeah, I thought that's what we were talking about, and in that
regard I agree that the docs could be more clear.  I'm not quite
sure what to say where to fix that, but I can see how someone could
be confused and have the expectation that once they have run BEGIN
TRANSACTION ISOLATION LEVEL SERIALIZABLE the transaction will not
see the work of transactions committing after that.  The fact that
this is possible is implied, if one reads carefully and thinks
about it, by the statement right near the start of the "Transaction
Isolation" section which says "any concurrent execution of a set of
Serializable transactions is guaranteed to produce the same effect
as running them one at a time in some order."  As Robert pointed
out, this is not necessarily the commit order or the transaction
start order.

It is entirely possible that if you have serializable transactions
T1 and T2, where T1 executes BEGIN first (and even runs a query
before T2 executes BEGIN) and T1 commits first, that T2 will
"appear" to have run first because it will look at a set of data
which T1 modifies and not see the changes.  If T1 were to *also*
look at a set of data which T2 modifies, then one of the
transactions would be rolled back with a serialization failure, to
prevent a cycle in the apparent order of execution; so the
requirements of the standard (and of most software which is
attempting to handle race conditions) is satisfied.  For many
popular benchmarks (and I suspect most common workloads) this
provides the necessary protections with better performance than is
possible using blocking to provide the required guarantees.[1]


Yes, you're right in that the "any concurrent execution..." phrase 
implicitly means that snapshot may not be taken at BEGIN or SET 
TRANSACTION time, but it's definitely not clear enough for the average 
user. Yet this may apply to the serializable case, but it doesn't to the 
repeatable read where the docs read " The Repeatable Read isolation 
level only sees data committed before the transaction began; it never 
sees either uncommitted data or changes committed during transaction 
execution by concurrent transactions". The first part is confusing, as 
we discussed; the second part is even more confusing as it says "during 
transaction execution", and isn't the transaction -not the snapshot- 
beginning at BEGIN time?


Surprisingly, the language is way more clear in the SET TRANSACTION 
doc page [2].




At any rate, the language in that section is a little fuzzy on the
concept of the "start of the transaction."  Perhaps it would be
enough to change language like:

| sees a snapshot as of the start of the transaction, not as of the
| start of the current query within the transaction.

to:

| sees a snapshot as of the start of the first query within the
| transaction, not as of the start of the current query within the
| transaction.

Would that have prevented the confusion here?


I think that definitely helps. But it may be better to make it even 
more clear, more explicit. And offering a solution for the user who may 
like the snapshot to be taken "at begin time", like suggesting to do a 
"SELECT 1" query.



 But what about creating a flag to BEGIN and SET TRANSACTION
commands, called "IMMEDIATE FREEZE" (or something similar), which
applies only to REPEATABLE READ and SERIALIZABLE? If this flag is set
(and may be off by default, but of course the default may be
configurable via a guc parameter), freeze happens when it is present
(BEGIN or SET TRANSACTION) time. This would be a backwards-compatible
change, while would provide the option of freezing without the nasty
hack of having to do a "SELECT 1" prior to your real queries, and
everything will of course be well documented.

What is the use case where you are having a problem?  This seems
like an odd solution, so it would be helpful to know what problem
it is attempting to solve.


I don't have a particular use case. I just came across the issue 
and thought the documentation and behavior wasn't consistent. So the 
first aim is not to have users surprised (in a bad way). But I see a 
clear use case: users who might want to open a (repeatable read | 
serializable) transaction to have their view of the database frozen, to 
perform any later operation on that frozen view. Sure, that comes at a 
penalty, but I see that potentially interesting too.


Regards,

Álvaro



[1] http://www.postgresql.org/docs/9.4/static/transaction-iso.html
[2] http://www.postgresql.org/docs/9.4/static/sql-set-transaction.html

--
Álvaro Hernández Tortosa


-

[HACKERS] Support for detailed description of errors cased by trigger-violations

[Andreas Joseph Krogh]

Hi.   When working with Oracle it is possible to catch constraint-violations 
caused by triggers using JDBC, but it seems this isn't possible using PG, see 
this thread: 
https://github.com/impossibl/pgjdbc-ng/issues/111#issuecomment-62276464   For 
check of FK-violations the protocol supports this fine, with details about 
which table, column etc. causing the violation. Is there any work going on or 
are there any plans to support similar info for violations caused by triggers?  
Thanks.   -- Andreas Joseph Krogh CTO / Partner - Visena AS Mobile: +47 909 56 
963 andr...@visena.com  www.visena.com 
  

Re: [HACKERS] pg_multixact not getting truncated

[Josh Berkus]

On 11/07/2014 05:29 PM, Alvaro Herrera wrote:
> Josh Berkus wrote:
>> Of course, this will lead to LOTs of additional vacuuming ...
> 
> There's a trade-off here: more vacuuming I/O usage for less disk space
> used.  How stressed your customers really are about 1 GB of disk space?

These customers not so much.  The users I encountered on chat whose
pg_multixact was over 20GB, and larger than their database?  Lots.

On 11/08/2014 03:54 AM, Andres Freund wrote:
> On 2014-11-07 17:20:44 -0800, Josh Berkus wrote:
>> So the basic problem is that multixact files are just huge, with an
>> average of 35 bytes per multixact?
>
> Depends on the concurrency. The number of members is determined by the
> number of xacts concurrenly locking a row..

Yeah, that leads to some extreme inflation for databases where FK
conflicts are common though.

On 11/08/2014 03:54 AM, Andres Freund wrote:
> On 2014-11-07 17:20:44 -0800, Josh Berkus wrote:
>> Of course, this will lead to LOTs of additional vacuuming ...
>
> Yes. And that's likely to cause much, much more grief.
>
> Also. Didn't you just *vehemently* oppose making these values tunable at
> all?

Yes, I opposed adding a *user* tunable with zero information on how it
should be tuned or why.  I always do and always will.  I also think our
defaults for multixact freezing should be tied to the ones for xid
freezing, and should not by default be completely independent numbers;
I'm still not convinced that it makes sense to have a separate multixact
threshold at all **since the same amount of vacuuming needs to be done
regardless of whether we're truncating xids or mxids**.

Certainly when I play with tuning this for customers, I'm going to lower
vacuum_freeze_table_age as well.

-- 
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] two dimensional statistics in Postgres

[Tomas Vondra]

On 8.11.2014 15:40, Katharina Büchse wrote:
> I'm sorry if I repeated myself too often, I somehow started answering
> at the end of your mail and then went up... I promise to do this
> better next time.

Nah, no problem. Better say something twice than not at all ;-)

However, I see you've responded to me directly (not through the
pgsql-hackers list). I assume that's not on purpose, so I'm adding the
list back into the loop ...

> On 07.11.2014 20:37, Tomas Vondra wrote:
>> On 7.11.2014 13:19, Katharina Büchse wrote:
>>> On 06.11.2014 11:56, Tomas Vondra wrote:
 Dne 6 Listopad 2014, 11:15, Katharina Büchse napsal(a):
> because correlations might occur only in parts of the data.
> In this case a histogram based on a sample of the whole table
> might not get the point and wouldn't help for the part of the
> data the user seems to be interested in.
 Yeah, there may be dependencies that are difficult to spot in the whole
 dataset, but emerge once you filter to a specific subset.

 Now, how would that work in practice? Initially the query needs
 to be planned using regular stats (because there's no feedback
 yet), and then - when we decide the estimates are way off - may
 be re-planned using the feedback. The feedback is inherently
 query-specific, so I'm not sure if it's possible to reuse it
 for multiple queries (might be possible for "sufficiently
 similar" ones).

 Would this be done automatically for all queries / all conditions, or
 only when specifically enabled (on a table, columns, ...)?
>>>
>>> The idea is the following: I want to find out correlations with
>>> two different algorithms, one scanning some samples of the data,
>>> the other analyzing query feedback.
>>>
>> So you're starting with the default (either single or
>> multivariate) statistics, computed by ANALYZE from a sample
>> (covering the whole table).
>
> yes
>
>> And then compute matching statistics while running the query (i.e.
>> a sample filtered by the WHERE clauses)?
>
> well, not really... I would like to emphasize that my systems
> consists of two parts:
>
> 1) finding correlations automatically
> 2) creating histograms for correlated columns.
>
> In (1) we use two different approaches, one of them feedback based, but
> even this approach has to collect several feedback data to be able to
> decide if there's correlation or not. So "while running the query" is
> not 100% correct. While running the query I would extract the feedback
> data the algorithm needs, which is the tuple count and the constraint on
> the data that was done in the query. Constraints should look like
> "columnA = 'a' and columnB = 'b'" and the more different queries we have
> with different constraints, the better it is. And yes, when this
> algorithm starts analyzing, there needs to be a check done for choosing
> consistent feedback data. So if there were several queries on two
> columns of a table, but the data changed while these queries took place,
> we cannot use all of the feedback we have.

OK, thanks for the explanation!

>>> If both decide for a column combination that it's correlated,
>>> then there should be made a "regular histogram" for this
>>> combination. If only the "scanning"-algorithm says "correlated",
>>> then it means that there is some correlation, but this is not
>>> interesting for the user right now.
>>>
>> Isn't it sufficient to simply compare the estimated and observed
>> number of rows?
>
> This sounds like the easiest way, but has many disadvantages. Just 
> imagine, that the statistical information is based on data that
> already changed. The estimate could be totally wrong (even if we
> automatically update statistics after a change of, let's say, 10%,
> this might happen, because the user could ask for exactly the part
> which changed, and if there was "only" a change of maybe 8%, the
> histogram would still be the old one), but this has nothing to do
> with correlation. If we then always decided to build a
> multidimensional histogram, it would mean to do unnecessary work,
> because creating and maintain multidimensional histograms is more
> expensive then one dimensional ones.

Good point. IMHO stale stats are a problem in general, and here it may
clearly cause "false positives" if the algorithm is not careful enough.

> But if an algorithm (which checked the data for correlations) says,
> that there really is correlation, the fact, that estimates and query
> results differ a lot from one another could be a good occasion to
> really create a histogram. Of course this decision could be based on
> the same "mistake" that I just described, but we already limited this
> wrong decision to the case that one algorithm "voted" for
> correlation.

Understood. I believe I understand the general goal, although I don't
have a clear idea how to implement that, or how complex it could get.
But I guess that's not a problem ... clearly you have a plan ;-)

>>> So I wou

Re: [HACKERS] Fw: [GENERAL] PLV8 and JS exports / referencing

[Sehrope Sarkuni]

On Sat, Nov 8, 2014 at 12:13 PM, Jon Erdman
 wrote:
>
> So, I was trying to use mustache.js in PG by defining a V8 function that 
> imports it. Older versions worked fine, but in newer versions they use a 
> class factory and I can't figure out how to reference the mustache stuff that 
> it creates. Apparently I need to know how our V8 implementation does exports.

If you define a variable named "exports" before the Mustache factory
code then it'll make it think it's loading in a CommonJS environment
and the "exports" variable will be populated with the Mustache
methods.

I'm not too familiar with PLV8 (so I'm not sure if there's a
better/direct way to support modules) but the following works fine:

CREATE OR REPLACE FUNCTION mustache(template text, view json)
RETURNS TEXT
LANGUAGE plv8
IMMUTABLE
STRICT
AS $BODY$
var exports = {};
// Copy/paste 
https://raw.githubusercontent.com/janl/mustache.js/master/mustache.js
var Mustache = exports;
return Mustache.render(template, view);
$BODY$;

test=> SELECT mustache('test: {{foo}}', '{"foo": "bar"}'::json);
 mustache
---
 test: bar
(1 row)

Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.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] row_to_json bug with index only scans: empty keys!

[Tom Lane]

Oh, some more fun: a RowExpr that's labeled with a named composite type
(rather than RECORD) has the same issue of not respecting aliases.
Continuing previous example with table "foo":

regression=# create view vv as select * from foo;
CREATE VIEW
regression=# select row_to_json(q) from vv q;
   row_to_json   
-
 {"f1":1,"f2":2}
(1 row)

regression=# select row_to_json(q) from vv q(a,b);
   row_to_json   
-
 {"f1":1,"f2":2}
(1 row)

So that's another case we probably want to change in HEAD but not the back
branches.

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 CREATE support to event triggers

[Andres Freund]

On 2014-11-08 12:30:29 -0500, Robert Haas wrote:
> On Sat, Nov 8, 2014 at 12:20 PM, Andres Freund  wrote:
> >> Putting half of it into core wouldn't fix that, it would just put a
> >> lot more maintenance burden on core developers.
> >
> > Imo stuff that can't be done sanely outside core needs to be put into
> > core if it's actually desired by many users. And working DDL replication
> > for logical replication solutions surely is.
> 
> I don't buy it.  This patch series is *all about* transferring the
> maintenance burden of this feature from the BDR developers to the core
> project.

What? Not at all. It's *much* less work to do these kind of things out
of core. As you probably have experienced more than once. If it were
possible to do this entirely in a extension I'm pretty sure nobody would
have bothered.

> There's nothing to keep you from exposing the parse trees to
> C functions that can live in an extension, and you can do all of this
> deparsing there.

Not really. There's some core functions that need to be touched. Like
most of the stuff in patches 1,2,3,5,16 does.

We could just integrate those parts, and be done with it. But would that
actually be a good thing for the community? Then slony needs to do it
and potentially others as well? Then auditing can't use it? Then
potential schema tracking solutions can't use it?

> Nobody will stop you, and when it breaks (not if)
> you can fix it in your code. The overhead of deparsing new types of
> parse nodes can be born by you.  The only benefit of pushing it into
> core is that some other logical replication solution could also take
> advantage of that, but I know of nobody with any plans to do such a
> thing.

There've been people for a long while asking about triggers on catalogs
for that purpose. IIRC Jan was one of them.

> On the flip side, the *cost* of pushing it into core is that
> it now becomes the PostgreSQL's community problem to update the
> deparsing code every time the grammar changes.  That cost-benefit
> trade-off does not look favorable to me, especially given the absence
> of any kind of robust testing framework.

I agree that there should be testing in this.

But I think you're quite overstating the effort of maintaining
this. Looking at gram.y between the stamping of 9.3 devel and 9.4 devel
for commits that'd require DDL deparsing changes:
* ALTER TABLE .. ALTER CONSTRAINT: About 7 lines for the deparse code,
  out of a ~350 line patch
* REFRESH MATERIALIZED VIEW ... CONCURRENTLY: About 1 line for deparse,
  out of ~700
* WITH CHECK OPTION for views: About 3 lines for deparse, out of ~1300
* REPLICA IDENTITY: About 24 lines for deparse, out of ~950
* ALTER TABLESPACE MOVE: About 20 lines for deparse, out of
  ~340. Although that patch was essentially scrapped afterwards. And
  rewritten differently.
* CREATE TABLESPACE ... WITH ...:  Not supported by event triggers right
  now as it's a global object.

That's really not a whole lot.

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] row_to_json bug with index only scans: empty keys!

[Andrew Dunstan]

On 11/08/2014 12:40 PM, Tom Lane wrote:

Andrew Dunstan  writes:

On 11/08/2014 12:14 PM, Tom Lane wrote:

That would be my druthers.  But given the lack of complaints, maybe we
should just stick to the more-backwards-compatible behavior until someone
does complain.  Thoughts?

Wouldn't that would mean we might not pick up the expected aliases in
  select row_to_json(q) from (select a,b from foo) as q(x,y)
? If so, I'm definitely in favor of fixing this now.

Well, it's inconsistent now.  In existing releases you might or might not
get x,y:

regression=# create table foo (f1 int, f2 int);
CREATE TABLE
regression=# insert into foo values(1,2);
INSERT 0 1
regression=# select row_to_json(q) from (select f1 as a, f2 as b from foo) as 
q(x,y);
   row_to_json
---
  {"x":1,"y":2}
(1 row)

regression=# select row_to_json(q) from (select f1 as a, f2 as b from foo 
offset 0) as q(x,y);
row_to_json
-
  {"f1":1,"f2":2}
(1 row)

That seems like something that's worth fixing going forward, but it's a
lot harder to make the case that we should change it in back branches.




Sure. As long as we fix the empty alias issue in the back branches, just 
fixing this prospectively seems fine. But I don't think we should wait 
for more complaints.


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] row_to_json bug with index only scans: empty keys!

[Tom Lane]

Andrew Dunstan  writes:
> On 11/08/2014 12:14 PM, Tom Lane wrote:
>> That would be my druthers.  But given the lack of complaints, maybe we
>> should just stick to the more-backwards-compatible behavior until someone
>> does complain.  Thoughts?

> Wouldn't that would mean we might not pick up the expected aliases in
>  select row_to_json(q) from (select a,b from foo) as q(x,y)
> ? If so, I'm definitely in favor of fixing this now.

Well, it's inconsistent now.  In existing releases you might or might not
get x,y:

regression=# create table foo (f1 int, f2 int);
CREATE TABLE
regression=# insert into foo values(1,2);
INSERT 0 1
regression=# select row_to_json(q) from (select f1 as a, f2 as b from foo) as 
q(x,y);
  row_to_json  
---
 {"x":1,"y":2}
(1 row)

regression=# select row_to_json(q) from (select f1 as a, f2 as b from foo 
offset 0) as q(x,y);
   row_to_json   
-
 {"f1":1,"f2":2}
(1 row)

That seems like something that's worth fixing going forward, but it's a
lot harder to make the case that we should change it in back branches.

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] row_to_json bug with index only scans: empty keys!

[Andrew Dunstan]

On 11/08/2014 12:14 PM, Tom Lane wrote:

I assume that's what you would propose for just the stable branches, and
that going forward we'd always use the aliases from the RTE?

That would be my druthers.  But given the lack of complaints, maybe we
should just stick to the more-backwards-compatible behavior until someone
does complain.  Thoughts?





Wouldn't that would mean we might not pick up the expected aliases in

select row_to_json(q) from (select a,b from foo) as q(x,y)

? If so, I'm definitely in favor of fixing this now.

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] Add CREATE support to event triggers

[Robert Haas]

On Sat, Nov 8, 2014 at 12:20 PM, Andres Freund  wrote:
>> Putting half of it into core wouldn't fix that, it would just put a
>> lot more maintenance burden on core developers.
>
> Imo stuff that can't be done sanely outside core needs to be put into
> core if it's actually desired by many users. And working DDL replication
> for logical replication solutions surely is.

I don't buy it.  This patch series is *all about* transferring the
maintenance burden of this feature from the BDR developers to the core
project.  There's nothing to keep you from exposing the parse trees to
C functions that can live in an extension, and you can do all of this
deparsing there.  Nobody will stop you, and when it breaks (not if)
you can fix it in your code.  The overhead of deparsing new types of
parse nodes can be born by you.  The only benefit of pushing it into
core is that some other logical replication solution could also take
advantage of that, but I know of nobody with any plans to do such a
thing.  On the flip side, the *cost* of pushing it into core is that
it now becomes the PostgreSQL's community problem to update the
deparsing code every time the grammar changes.  That cost-benefit
trade-off does not look favorable to me, especially given the absence
of any kind of robust testing framework.

-- 
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] row_to_json bug with index only scans: empty keys!

[Kevin Grittner]

Tom Lane 
> Andrew Dunstan  writes:

>> I assume that's what you would propose for just the stable branches, and
>> that going forward we'd always use the aliases from the RTE?
>
> That would be my druthers.  But given the lack of complaints, maybe we
> should just stick to the more-backwards-compatible behavior until someone
> does complain.  Thoughts?

I think consistent use of the aliases would be less surprising and
should be what we do on master.  I agree that we should avoid
breaking anything that might be working as intended on stable
branches.

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


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

Re: [HACKERS] Add CREATE support to event triggers

[Andres Freund]

On 2014-11-08 12:07:41 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > On 2014-11-08 11:52:43 -0500, Tom Lane wrote:
> >> Adding a similar
> >> level of burden to support a feature with a narrow use-case seems like
> >> a nonstarter from here.
> 
> > I don't understand this statement. In my experience the lack of a usable
> > replication solution that allows temporary tables and major version
> > differences is one of the most, if not *the* most, frequent criticisms
> > of postgres I hear. How is this a narrow use case?
> 
> [ shrug... ]  I don't personally give a damn about logical replication,
> especially not logical replication implemented in this fashion.

"In this fashion" meaning ddl replication via event triggers? If you
have an actual suggestion how to do it better I'm all ears. So far
nobody has come up with anything.

> Or in short: AFAICS you're not building the next WAL-shipping replication
> solution, you're building the next Slony, and Slony never has and never
> will be more than a niche use-case.

A good number of the sites out there use either londiste or slony. Not
because they like it, but because there's no other alternative.

I'd love to simply say that we can make WAL based replication work
across versions, platforms and subsets of relations in PG
clusters. Since that seems quite unrealistic people have to go different
ways.

> Putting half of it into core wouldn't fix that, it would just put a
> lot more maintenance burden on core developers.

Imo stuff that can't be done sanely outside core needs to be put into
core if it's actually desired by many users. And working DDL replication
for logical replication solutions surely is.

> Core developers are entitled to push back on such proposals.

I'm not saying "core developers" (whover that is) aren't allowed to do
so. But just because they think something is (too) invasive doesn't make
it a niche application.

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] row_to_json bug with index only scans: empty keys!

[Tom Lane]

Andrew Dunstan  writes:
> On 11/08/2014 11:24 AM, Tom Lane wrote:
>> That seems like a pretty silly move: it wouldn't actually fix anything,
>> and it would break cases that perhaps are acceptable to users today.

> What evidence do you have that it might be acceptable to today's users? 
> The only evidence we have that I know of is Ross' complaint that 
> indicates that it's not acceptable.

The fact that we've only gotten two bug reports in however many years the
failure has been possible might mean that few people encounter the case,
or it might mean that it doesn't break things for their usages so badly
that they need to complain.  If the latter, throwing an error rather than
fixing it is not going to be an improvement.

> I assume that's what you would propose for just the stable branches, and 
> that going forward we'd always use the aliases from the RTE?

That would be my druthers.  But given the lack of complaints, maybe we
should just stick to the more-backwards-compatible behavior until someone
does complain.  Thoughts?

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] Fw: [GENERAL] PLV8 and JS exports / referencing

[Jon Erdman]

Got radio silence on this question in -general, and upon reflection I think it 
might be a -hacker level question. I'm not sure who actually implemented PLV8, 
but I think it might take someone at or near that level to answer.

If not, sorry for the noise. Thanks!

If anyone wants to try to reproduce it, replace the snipped out bits using the 
contents of mustache.js from http://github.com/janl/mustache.js and then fiddle 
with my "return" line at the bottom to try to reference any function that's 
defined in that above snipped portion (such as .render()). Here's a sample sql 
call to the full defined pg function, that does work with my hacked up one that 
removes the factory:

SELECT mustache('
CREATE TABLE {{{ table_name }}} (
{{{ table_name }}}_id SERIAL PRIMARY KEY
{{ #cols }}
, {{{ def }}}
{{ /cols }}
);'
, '{
"table_name": "my_table"
, "cols": [ 
{ "def": "t text" }
, { "def": "i int" }
]
   }'
);

--
Jon Erdman (aka StuckMojo)
PostgreSQL Zealot



Begin forwarded message:

Date: Wed, 5 Nov 2014 17:01:29 -0600
From: Jon Erdman 
To: pgsql-gene...@postgresql.org
Subject: [GENERAL] PLV8  and JS exports / referencing



So, I was trying to use mustache.js in PG by defining a V8 function that 
imports it. Older versions worked fine, but in newer versions they use a class 
factory and I can't figure out how to reference the mustache stuff that it 
creates. Apparently I need to know how our V8 implementation does exports. 

Here's the top of mustache.js with the class factory, and my attempted 
reference at the bottom (the return which gives me undefined reference). I 
tried various invocations and couldn't quite get it. I ended up hacking it up 
to remove the factory and change it to explicitly declare a variable to make it 
work, but I'd like to avoid that if possible.

If anyone wants to try to reproduce it, replace the snipped out bits using the 
contents of mustache.js from http://github.com/janl/mustache.js 

CREATE OR REPLACE FUNCTION mustache(template text, view json)
RETURNS TEXT
LANGUAGE plv8
IMMUTABLE
STRICT
AS $$

(function (global, factory) {
  if (typeof exports === "object" && exports) {
factory(exports); // CommonJS
  } else if (typeof define === "function" && define.amd) {
define(['exports'], factory); // AMD
  } else {
factory(global.Mustache = {}); // 

Re: [HACKERS] Add CREATE support to event triggers

[Tom Lane]

Andres Freund  writes:
> On 2014-11-08 11:52:43 -0500, Tom Lane wrote:
>> Adding a similar
>> level of burden to support a feature with a narrow use-case seems like
>> a nonstarter from here.

> I don't understand this statement. In my experience the lack of a usable
> replication solution that allows temporary tables and major version
> differences is one of the most, if not *the* most, frequent criticisms
> of postgres I hear. How is this a narrow use case?

[ shrug... ]  I don't personally give a damn about logical replication,
especially not logical replication implemented in this fashion.  It looks
large and rickety (ie full of opportunities for bugs) and I would never
trust data I cared about to it.

Or in short: AFAICS you're not building the next WAL-shipping replication
solution, you're building the next Slony, and Slony never has and never
will be more than a niche use-case.  Putting half of it into core wouldn't
fix that, it would just put a lot more maintenance burden on core
developers.  Core developers are entitled to push back on such proposals.

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] row_to_json bug with index only scans: empty keys!

[Andrew Dunstan]

On 11/08/2014 11:24 AM, Tom Lane wrote:

Andrew Dunstan  writes:

On 11/08/2014 09:26 AM, Robert Haas wrote:

I'm not sure whether this is safe enough to back-patch, but it seems
like we should probably plan to back-patch *something*, because the
status quo isn't great either.

I confirm that Tom's patch does indeed fix my test case that produces
empty field names.
We should probably not backpatch it, as it is a behaviour change.
However, I do think we should add checks in composite_to_json and
hstore_from_record for empty field names, and error out if they are
found.

That seems like a pretty silly move: it wouldn't actually fix anything,
and it would break cases that perhaps are acceptable to users today.


What evidence do you have that it might be acceptable to today's users? 
The only evidence we have that I know of is Ross' complaint that 
indicates that it's not acceptable.


However,



We could reduce the risks involved by narrowing the cases in which
ExecEvalWholeRowVar will replace field names it got from the input.
I'd be inclined to propose:

1. If Var is of a named composite type, use *exactly* the field names
associated with that type.  (This avoids the need to possibly produce
RECORD outputs from a named-type Var, thus removing the Assert-weakening
issue.)

2. If Var is of type RECORD, replace only empty field names with aliases
from the RTE.  (This might sound inconsistent --- could you end up with
some names coming from point A and some from point B? --- but in practice
I think it would always be all-or-nothing, because the issue is whether
or not the planner bothered to attach column names to a lower-level
targetlist.)




I assume that's what you would propose for just the stable branches, and 
that going forward we'd always use the aliases from the RTE? Or maybe 
I'm not quite understanding enough which cases will be covered. To the 
extent that I do this sounds OK.


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] TODO request: log_long_transaction

[Kevin Grittner]

Robert Haas  wrote:

>> 3. Should long transactions which are rolled back be logged as well?
>
> Yes.

+1

>> 4. We log the statement when exceeding log_min_duration_statement, but
>> for transactions, that does not make a lot of sense, or should the last
>> statement be logged?  I don't think that would be particularly useful.
>
> This is a potentially serious problem with this whole idea, and the
> idea in #2.  You can log that it happened, but without some idea of
> what it did, it's probably not going to be too useful.

The database currently lacks two things which I have seen used for
this purpose in database access middleware: an "application area"
(sort of like application name, but more fine-grained and expected
to change within the lifetime of a connection) and a transaction
class name.

For a connection related to an "In Court" application, there might
be an application area of "Mass Traffic Dispo" which has 10 or 20
transaction classes.  Examples of transaction classes could be to
enter a Default Judgment of Guilty (for all cases scheduled for
that session where the defendant didn't appear), or to Grant Time
to Pay to those found guilty who have not paid the citation in
full.  (It could often make sense for a given transaction class to
be usable from more than one application area, and for the context
to be valuable.)

If we added GUCs for application area and transaction class, those
could be included in the log message for a long-running
transaction.  That would make the messages useful -- at least for
occurrences when either or both were set.  The question is whether 
people would be willing to set these GUCs to make the logging 
useful

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


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

Re: [HACKERS] Add CREATE support to event triggers

[Andres Freund]

On 2014-11-08 11:52:43 -0500, Tom Lane wrote:
> Adding a similar
> level of burden to support a feature with a narrow use-case seems like
> a nonstarter from here.

I don't understand this statement. In my experience the lack of a usable
replication solution that allows temporary tables and major version
differences is one of the most, if not *the* most, frequent criticisms
of postgres I hear. How is this a narrow use case?

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] Add CREATE support to event triggers

[Tom Lane]

Robert Haas  writes:
> If it were a question of writing this code once and being done with
> it, that would be unobjectionable in my view.  But it isn't.
> Practically every change to gram.y is going to require a corresponding
> change to this stuff.  As far as I can see, nobody except me has
> commented on the burden that places on everyone who may wish to add
> syntax support for a new construct in the future, which might mean
> that I'm worrying about something that isn't worth worrying about, but
> what I think is more likely is that nobody's worrying about it right
> now because they haven't had to do it yet.

I haven't been paying much attention to this thread, but I concur with
Robert that adding yet another set of overhead requirements to any
addition of new SQL is not a good thing.

> Just to illustrate the point, consider the CREATE TABLE name OF type
> syntax that Peter added a few years ago.   That patch
> (e7b3349a8ad7afaad565c573fbd65fb46af6abbe) had the following impact on
> gram.y:

This analysis is kind of cheating, because adding new syntax hasn't been
only a matter of touching gram.y for a very long time.  You invariably
have to touch pg_dump, and you have to touch ruleutils.c unless it's
strictly a DDL-command change.  But people are used to those, and the
value of keeping pg_dump working is clear to everybody.  Adding a similar
level of burden to support a feature with a narrow use-case seems like
a nonstarter from here.  ESPECIALLY if we also have to manually add
regression test cases because there's no easy way to test it directly.

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] Convert query plan to sql query

[Tom Lane]

Robert Haas  writes:
> On Sat, Nov 8, 2014 at 1:09 AM, mariem  wrote:
>> I'm aware of ruleutils.c which I think is a good tool but I don't think it's
>> appropriate as it takes the parse tree as input.

> "Parse", "Rewrite", and "Plan" are distinct stages.  ruleutils.c takes
> the tree that results from rewriting, before planning has been done.
> So I'm not sure it's quite accurate to say that it takes the "parse
> tree" - rewrite rules will already have been applied.

More specifically: rewrite is a parsetree-to-parsetree transformation;
it does not change the representational rules at all.  It's true that
the "typical" use of ruleutils is on parser output (ie stored views)
but it will work fine on rewriter output.

If what you're wishing for is that you could also capture the effects
of planner steps that are in the nature of source-to-source
transformations, I think that's going to be harder because no great
effort has been made to keep those at arm's length from steps that
don't have results describable as pure SQL.  However, you could probably
get pretty far if you applied ruleutils.c to the modified parse tree
after the constant-folding and join tree simplification phases.

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] row_to_json bug with index only scans: empty keys!

[Tom Lane]

Andrew Dunstan  writes:
> On 11/08/2014 09:26 AM, Robert Haas wrote:
>> I'm not sure whether this is safe enough to back-patch, but it seems
>> like we should probably plan to back-patch *something*, because the
>> status quo isn't great either.

> I confirm that Tom's patch does indeed fix my test case that produces 
> empty field names.

> We should probably not backpatch it, as it is a behaviour change. 
> However, I do think we should add checks in composite_to_json and 
> hstore_from_record for empty field names, and error out if they are 
> found.

That seems like a pretty silly move: it wouldn't actually fix anything,
and it would break cases that perhaps are acceptable to users today.

We could reduce the risks involved by narrowing the cases in which
ExecEvalWholeRowVar will replace field names it got from the input.
I'd be inclined to propose:

1. If Var is of a named composite type, use *exactly* the field names
associated with that type.  (This avoids the need to possibly produce
RECORD outputs from a named-type Var, thus removing the Assert-weakening
issue.)

2. If Var is of type RECORD, replace only empty field names with aliases
from the RTE.  (This might sound inconsistent --- could you end up with
some names coming from point A and some from point B? --- but in practice
I think it would always be all-or-nothing, because the issue is whether
or not the planner bothered to attach column names to a lower-level
targetlist.)

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] Sequence Access Method WIP

[Robert Haas]

On Sat, Nov 8, 2014 at 6:17 AM, Petr Jelinek  wrote:
> Honestly, I am still not convinced that the centralized sequence server with
> no local caching is something we should be optimizing for as that one will
> suffer from performance problems anyway. And it can ignore the last_value
> input from postgres if it choses to, so it's not like the currently proposed
> patch forbids implementation of such AMs.

I can buy that.

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


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

Re: [HACKERS] Add CREATE support to event triggers

[Robert Haas]

On Sat, Nov 8, 2014 at 4:37 AM, Andres Freund  wrote:
> I don't understand why this is particularly difficult to regresssion
> test. It actually is comparatively simple?

If it is, great.  I previously wrote this email:

http://www.postgresql.org/message-id/CA+TgmoZ=vzrijmxlkqi_v0jg4k4leapmwusc6rwxs5mquxu...@mail.gmail.com

Alvaro came up with a way of addressing the second point I raised
there, which I'm quite pleased about, but AFAIK there's been no
progress on the first one.  Maybe I missed something?

>> and which will likely receive very limited
>> testing from users to support a feature that is not in core,
>
> Just like half of the features you worked on yourself lately? Why is
> this an argument?

Because the stuff I'm adding doesn't break every time someone adds a
new DDL command, something that we do regularly.

If it were a question of writing this code once and being done with
it, that would be unobjectionable in my view.  But it isn't.
Practically every change to gram.y is going to require a corresponding
change to this stuff.  As far as I can see, nobody except me has
commented on the burden that places on everyone who may wish to add
syntax support for a new construct in the future, which might mean
that I'm worrying about something that isn't worth worrying about, but
what I think is more likely is that nobody's worrying about it right
now because they haven't had to do it yet.

Just to illustrate the point, consider the CREATE TABLE name OF type
syntax that Peter added a few years ago.   That patch
(e7b3349a8ad7afaad565c573fbd65fb46af6abbe) had the following impact on
gram.y:

 src/backend/parser/gram.y |   56 ++---
 1 file changed, 43 insertions(+), 13 deletions(-)

Now let's have a look at what impact it has on the deparsing code.
Patch 6 has deparse_ColumnDef_Typed from lines 134 to 193.  There's 3
or so lines in deparseTableElements that decide whether to call it.
Patch 8 has more handling for this case, lines 439 to 443 and 463 to
490.  So, if this feature had been committed before TABLE OF, it would
have needed about 100 lines of net new code to handle this case -
exclusive of refactoring.  The actual size of the patch would probably
have been modestly larger than that, because some code would need to
be reindented when it got iffed out, and quite possibly some
rearrangement would have been needed.  But even ignoring all that, the
deparse footprint of the patch would have been MORE THAN TWICE the
parser footprint.

I think that's going to be typical; and I think the deparse code is
going to be significantly more labor-intensive to write than bison
productions are.  Do you really think that's not going to be a burden?

> Being able to replicate DDL is a feature wish that has been around
> pretty much since the inception of trigger based replication
> solution. It's not some current fancy. And the json stuff only got there
> because people wanted some way to manipulate the names in the replicated
> - which this abstraction provides them with.

I understand that being able to replicate DDL is an important need,
and there may be no better way to do it than this.  But that doesn't
mean that this code is going to be bug-free or easy to maintain.

>> may never be, and which I strongly suspect may be too clever by half.
>> Once you've committed it, you're going to move onto other things and
>> leave it to everyone who comes after to try to maintain it.  I bet
>> we'll still be running into bugs and half-implemented features five
>> years from now, and maybe in ten.  Ramming through special-purpose
>> infrastructure instead of generalizing it is merely icing on the cake,
>> but it's still moving in the wrong direction.
>
> You're just as much to blame for not writing a general json abstraction
> layer for EXPLAIN. I'd say that's not much blame, but still, there's
> really not much difference there.

Well, this patch is series is at least an order of magnitude larger,
and it's apparently doing significantly more complex stuff with JSON,
because the explain.c patch just writes it out into a StringInfo.

-- 
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] row_to_json bug with index only scans: empty keys!

[Andrew Dunstan]

On 11/08/2014 09:26 AM, Robert Haas wrote:

On Fri, Nov 7, 2014 at 11:27 PM, Tom Lane  wrote:

Thoughts?

I'm not sure whether this is safe enough to back-patch, but it seems
like we should probably plan to back-patch *something*, because the
status quo isn't great either.




I confirm that Tom's patch does indeed fix my test case that produces 
empty field names.


We should probably not backpatch it, as it is a behaviour change. 
However, I do think we should add checks in composite_to_json and 
hstore_from_record for empty field names, and error out if they are 
found. That seems like an outright bug which we should defend against, 
including in the back branches. Giving back json or hstore objects with 
made up names like f1 is one thing, giving them back with empty names 
(which, in the hstore case will collapse everything to a single field) 
is far worse.


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] tracking commit timestamps

[Robert Haas]

On Sat, Nov 8, 2014 at 5:35 AM, Petr Jelinek  wrote:
> That's not what I said. I am actually ok with adding the LSN if people see
> it useful.
> I was just wondering if we can make the record smaller somehow - 24bytes per
> txid is around 96GB of data for whole txid range and won't work with pages
> smaller than ~4kBs unless we add 6 char support to SLRU (which is not hard
> and we could also not allow track_commit_timestamps to be turned on with
> smaller pagesize...).
>
> I remember somebody was worried about this already during the original patch
> submission and it can't be completely ignored in the discussion about adding
> more stuff into the record.

Fair point.  Sorry I misunderstood.

I think the key question here is the time for which the data needs to
be retained.  2^32 of anything is a lot, but why keep around that
number of records rather than more (after all, we have epochs to
distinguish one use of a given txid from another) or fewer?  Obvious
alternatives include:

- Keep the data for some period of time; discard the data when it's
older than some threshold.
- Keep a certain amount of total data; every time we create a new
file, discard the oldest one.
- Let consumers of the data say how much they need, and throw away
data when it's no longer needed by the oldest consumer.
- Some combination of the above.

-- 
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] Convert query plan to sql query

[Robert Haas]

On Sat, Nov 8, 2014 at 1:09 AM, mariem  wrote:
> Hi Tom,
>>If you don't need that, why are you insistent on extracting the
>>information from a plan tree?
>
> I need to resolve expressions and apply rewrite rules before I reverse the
> query plan to a query.
>
>>It seems far simpler to me to make use of ruleutils.c to reverse-list
>>the original parsetree.  That functionality already exists and is well
>>tested and well maintained.  If you insist on working from a plan tree,
>>you will be writing a fair amount of code that you will have to maintain
>>yourself.  And I absolutely, positively guarantee that we will break it
>>in every major release, and occasionally in minor releases.  You should
>>read the git history of explain.c and ruleutils.c and ask yourself whether
>>you want to keep up with that level of churn.
>
> I'm aware of ruleutils.c which I think is a good tool but I don't think it's
> appropriate as it takes the parse tree as input.

"Parse", "Rewrite", and "Plan" are distinct stages.  ruleutils.c takes
the tree that results from rewriting, before planning has been done.
So I'm not sure it's quite accurate to say that it takes the "parse
tree" - rewrite rules will already have been applied.

-- 
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] row_to_json bug with index only scans: empty keys!

[Robert Haas]

On Fri, Nov 7, 2014 at 11:27 PM, Tom Lane  wrote:
> Thoughts?

I'm not sure whether this is safe enough to back-patch, but it seems
like we should probably plan to back-patch *something*, because the
status quo isn't great either.

-- 
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] Sequence Access Method WIP

[Simon Riggs]

On 5 November 2014 17:32, Heikki Linnakangas  wrote:

> Why does sequence_alloc need the current value? If it's a "remote" seqam,
> the current value is kept in the remote server, and the last value that was
> given to this PostgreSQL server is irrelevant.
>
> That irks me with this API. The method for acquiring a new value isn't fully
> abstracted behind the AM interface, as sequence.c still needs to track it
> itself. That's useful for the local AM, of course, and maybe some others,
> but for others it's totally useless.

Please bear in mind what seqam is for...

At present it's only use is to provide Global Sequences. There's a few
ways of doing that, but they all look sorta similar.

The only other use we thought of was shot down, so producing the
perfect API isn't likely to help anyone. It's really not worth the
effort to produce a better API.

BDR doesn't require Global Sequences, nor are Global Sequences
restricted in their use to just BDR - lots of cluster configurations
would want something like this.

-- 
 Simon Riggs   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] Add CREATE support to event triggers

[Michael Paquier]

On Sat, Nov 8, 2014 at 12:45 AM, Alvaro Herrera
 wrote:
> Michael Paquier wrote:
>
>> Here are more thoughts among those lines looking at the current state of
>> the patch 4 that introduces the infrastructure of the whole feature. This
>> would make possible in-memory manipulation of jsonb containers without
>> relying on a 3rd-part set of APIs like what this patch is doing with
>> ObjTree to deparse the DDL parse trees.
>
> Thanks for the thoughts.  I have to say that I have no intention of
> reworking the jsonb code.  If somebody else wants to do the legwork and
> add that API as you suggest, I'm happy to remove all the ObjTree stuff
> from this patch.  I don't expect this to happen too soon, though, so I
> would instead consider committing this patch based on ObjTree.  Later,
> when somebody comes around to reworking jsonb, we can rip ObjTree out.
The thing freaking me out in this case is when would that really
happen? Maybe years from now, and perhaps at that point we would
regret to not have put in place the infrastructure that we knew we
could have done.

>> This infrastructure would allow in-memory manipulation of jsonb containers.
>> Containers that can then be easily be manipulated to be changed back to
>> strings and for value lookups using key strings.
>
> Honestly, I had hoped that the jsonb code would have already included
> this kind of functionality.  I wasn't too happy when I discovered that I
> needed to keep the ObjTree crap.  But I don't want to do that work
> myself.
I can't blame you for that.

In any case, if this code goes in as-is (I am against it at this point
but I am just giving my opinion as a reviewer here), I think that at
least the following things could be done with a minimal effort:
- Provide the set of constructor functions for JsonbValue
- Move the jsonb APIs (find_*_in_jsonbcontainer) doing value lookups
using keys from ddl_json.c to jsonb_util.c, rename them to something
more consistent and complete the set for the other object types.
- Add a TODO item in the wiki, and a TODO item in where ObjTree is defined.
Thanks,
-- 
Michael


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

Re: [HACKERS] pg_multixact not getting truncated

[Andres Freund]

On 2014-11-07 17:20:44 -0800, Josh Berkus wrote:
> On 11/07/2014 04:43 PM, Alvaro Herrera wrote:
> > This says that the live multixact range goes from 123 million to 162
> > million; roughly 40 million values.  (The default value for
> > vacuum_multixact_freeze_table_age is 150 million, which is what
> > determines how many values are kept.)
> > 
> > You gist.github paste tells us there are 4598 members files.  Each file
> > has 32 pages, and each page hosts 2045 members; so there are 32 * 2045 *
> > 4598 members, or somewhat about 300 million.  For 40 million
> > multixacts, this means there are about 7 members per multixact, in
> > average, which seems a reasonable number to me.
> 
> So the basic problem is that multixact files are just huge, with an
> average of 35 bytes per multixact?

Depends on the concurrency. The number of members is determined by the
number of xacts concurrenly locking a row.

> > If you want to have vacuum truncate pg_multixact more aggresively, you
> > need to decrease vacuum_multixact_freeze_table_age and
> > vacuum_multixact_freeze_min_age.
> 
> If that's the case, then we need to set the defaults more
> aggressively.

Why? If you have that high transaction volume, a few seldomly read files
won't hurt you.

> Of course, this will lead to LOTs of additional vacuuming ...

Yes. And that's likely to cause much, much more grief.

Also. Didn't you just *vehemently* oppose making these values tunable at
all?

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] pg_multixact not getting truncated

[Andres Freund]

On 2014-11-07 16:09:44 -0800, Josh Berkus wrote:
> On 11/05/2014 11:15 AM, Josh Berkus wrote:
> > On 11/05/2014 10:40 AM, Jim Nasby wrote:
> >> Can you post the contents of pg_multixact/members/?
> > 
> > Well, not as of last week, obviously.
> > 
> > https://gist.github.com/jberkus/d05db3629e8c898664c4
> > 
> > I haven't pasted all the filenames, because, well, look at the count.  I
> > also added the contents of the /offsets directory, for full information.
> > 
> > Note that we've added 400 multixact files since my first email.
> 
> So, just did a quick survey of our managed services customers.  4 out of
> 6 have pg_multixact bloat.  Basically it looks like this:
> 
> User1 (the one above): 1.1GB
> User2: 64K
> User3: 929MB
> User4: 1.3GB
> User5: 301MB
> User6: 48K
> 
> Nobody (out of 6 more) running a version older than 9.3 had a
> pg_multixact larger than 128K.
> 
> None of these users used pg_upgrade, so that's not the source of this
> problem.  Instead, as demonstrated earlier, we are simply not truncating
> pg_multixact sufficiently.
> 
> So this looks like a pretty serious bug.

I've not seen actual evidence of a bug here. What's their multixact
related settings? How large is pg_clog?

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] Sequence Access Method WIP

[Petr Jelinek]

On 08/11/14 03:10, Robert Haas wrote:

On Fri, Nov 7, 2014 at 7:26 PM, Petr Jelinek  wrote:

My main problem is actually not with having tuple per-seqAM, but more
with the fact that Heikki does not want to have last_value as compulsory
column/parameter. How is the new AM then supposed to know where to pick
up and if it even can pick up?


That seems pretty well impossible to know anyway.  If the pluggable AM
was handing out values at random or in some unpredictable fashion,
there may be no well-defined point where it's safe for the default AM
to resume.  Granted, in the case of replication, it probably is
possible, and maybe that's important enough to be worth catering to.


While this is true, I think 99% of this use-case in practice is about 
converting existing schema with "local" sequence to some other sequence 
and perhaps fixing schema after people create sequence using wrong AM 
because their default is not what they thought it is.





And obviously, once the last_value is part of the compulsory columns we
again have to WAL log all the time for the use-case which Heikki is using as
model, so it does not help there (just to clear what my point was about).


But I don't know what to do about that.  :-(



Me neither and I don't think this is actually solvable, we either have 
last_value and logcnt as mandatory columns and the central sequence 
server example Heikki is talking about will be impacted by this, or we 
leave those columns to AM implementations and we lose the ability to do 
AM change for majority of cases, and also IMHO most AMs will then have 
to implement their own last_value and logcnt logic anyway.


Honestly, I am still not convinced that the centralized sequence server 
with no local caching is something we should be optimizing for as that 
one will suffer from performance problems anyway. And it can ignore the 
last_value input from postgres if it choses to, so it's not like the 
currently proposed patch forbids implementation of such AMs.


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


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

Re: [HACKERS] Sequence Access Method WIP

[Craig Ringer]

On 11/08/2014 12:35 AM, Petr Jelinek wrote:
>>
>> Do you think it'd be simple to provide a blocking, transactional
>> sequence allocator via this API? i.e. gapless sequences, much the same
>> as typically implemented with SELECT ... FOR UPDATE on a counter table.
>>
>> It might be more digestible standalone, and would be a handy contrib/
>> example extension demonstrating use of the API.
>>
> 
> Yes I think that's doable (once we iron out the API we can agree on).

Cool; I think that'd be an interesting self-contained example that'd
also have real-world uses ... and provide a nice succinct way to answer
those semi-regular "how do I create a sequence that doesn't have holes"
questions.

-- 
 Craig Ringer   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] tracking commit timestamps

[Petr Jelinek]

On 08/11/14 03:05, Robert Haas wrote:

On Fri, Nov 7, 2014 at 7:07 PM, Petr Jelinek  wrote:

but we can't have everything there
as there are space constraints, and LSN is another 8 bytes and I still want
to have some bytes for storing the "origin" or whatever you want to call it
there, as that's the one I personally have biggest use-case for.
So this would be ~24bytes per txid already, hmm I wonder if we can pull some
tricks to lower that a bit.


It won't do to say "let's do the things that I want, and foreclose
forever the things that other people want".  I find it quite hard to
believe that 16 bytes per transaction is a perfectly tolerable
overhead but 24 bytes per transaction will break the bank.  But if
that is really true then we ought to reject this patch altogether,
because it's unacceptable, in any arena, for a patch that only
benefits extensions to consume all of the available bit-space in,
leaving none for future core needs.



That's not what I said. I am actually ok with adding the LSN if people 
see it useful.
I was just wondering if we can make the record smaller somehow - 24bytes 
per txid is around 96GB of data for whole txid range and won't work with 
pages smaller than ~4kBs unless we add 6 char support to SLRU (which is 
not hard and we could also not allow track_commit_timestamps to be 
turned on with smaller pagesize...).


I remember somebody was worried about this already during the original 
patch submission and it can't be completely ignored in the discussion 
about adding more stuff into the record.


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


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

Re: [HACKERS] Add CREATE support to event triggers

[Andres Freund]

On 2014-11-07 21:41:17 -0500, Robert Haas wrote:
> On Fri, Nov 7, 2014 at 10:45 AM, Alvaro Herrera
>  wrote:
> > Michael Paquier wrote:
> >
> >> Here are more thoughts among those lines looking at the current state of
> >> the patch 4 that introduces the infrastructure of the whole feature. This
> >> would make possible in-memory manipulation of jsonb containers without
> >> relying on a 3rd-part set of APIs like what this patch is doing with
> >> ObjTree to deparse the DDL parse trees.
> >
> > Thanks for the thoughts.  I have to say that I have no intention of
> > reworking the jsonb code.  If somebody else wants to do the legwork and
> > add that API as you suggest, I'm happy to remove all the ObjTree stuff
> > from this patch.  I don't expect this to happen too soon, though, so I
> > would instead consider committing this patch based on ObjTree.  Later,
> > when somebody comes around to reworking jsonb, we can rip ObjTree out.
> >
> >> This infrastructure would allow in-memory manipulation of jsonb containers.
> >> Containers that can then be easily be manipulated to be changed back to
> >> strings and for value lookups using key strings.
> >
> > Honestly, I had hoped that the jsonb code would have already included
> > this kind of functionality.  I wasn't too happy when I discovered that I
> > needed to keep the ObjTree crap.  But I don't want to do that work
> > myself.
> 
> If we're going to have infrastructure for this in core, we really
> ought to make the effort to make it general instead of not.
> 
> I still think this whole idea is a mess.  It adds what looks to be a
> LOT of code that, at least as I understand it, we have no compelling
> way to regression test

I don't understand why this is particularly difficult to regresssion
test. It actually is comparatively simple?

> and which will likely receive very limited
> testing from users to support a feature that is not in core,

Just like half of the features you worked on yourself lately? Why is
this an argument?

Being able to replicate DDL is a feature wish that has been around
pretty much since the inception of trigger based replication
solution. It's not some current fancy. And the json stuff only got there
because people wanted some way to manipulate the names in the replicated
- which this abstraction provides them with.

> may never be, and which I strongly suspect may be too clever by half.
> Once you've committed it, you're going to move onto other things and
> leave it to everyone who comes after to try to maintain it.  I bet
> we'll still be running into bugs and half-implemented features five
> years from now, and maybe in ten.  Ramming through special-purpose
> infrastructure instead of generalizing it is merely icing on the cake,
> but it's still moving in the wrong direction.

You're just as much to blame for not writing a general json abstraction
layer for EXPLAIN. I'd say that's not much blame, but still, there's
really not much difference there.

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] BRIN indexes - TRAP: BadArgument

[David Rowley]

On Sat, Nov 8, 2014 at 8:56 AM, Alvaro Herrera 
wrote:

>
> I just pushed this, after some more minor tweaks.  Thanks, and please do
> continue testing!
>

Please find attached another small fix. This time it's just a small typo in
the README, and just some updates to some, now outdated docs.

Kind Regards

David Rowley
diff --git a/doc/src/sgml/ref/create_index.sgml 
b/doc/src/sgml/ref/create_index.sgml
index ead3375..18bd0d3 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -57,8 +57,8 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] 

PostgreSQL provides the index methods
-   B-tree, hash, GiST, SP-GiST, and GIN.  Users can also define their own index
-   methods, but that is fairly complicated.
+   B-tree, hash, GiST, SP-GiST, GIN, and BRIN.  Users can also define their own
+   index methods, but that is fairly complicated.
   
 
   
@@ -166,7 +166,8 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS 
] 
 The name of the index method to be used.  Choices are
 btree, hash,
-gist, spgist and gin.
+gist, spgist, gin, and
+brin.
 The default method is btree.

   
@@ -492,7 +493,7 @@ Indexes:
   
 
   
-   Currently, only the B-tree, GiST and GIN index methods support
+   Currently, only the B-tree, GiST, GIN, and BRIN index methods support
multicolumn indexes. Up to 32 fields can be specified by default.
(This limit can be altered when building
PostgreSQL.)  Only B-tree currently
diff --git a/src/backend/access/brin/README b/src/backend/access/brin/README
index 2619be8..636d965 100644
--- a/src/backend/access/brin/README
+++ b/src/backend/access/brin/README
@@ -126,7 +126,7 @@ that page range is complete and new tuples belong in a new 
page range that
 hasn't yet been summarized.  Those insertions do not create a new index
 entry; instead, the page range remains unsummarized until later.
 
-Wehn VACUUM is run on the table, all unsummarized page ranges are
+Whenever VACUUM is run on the table, all unsummarized page ranges are
 summarized.  This action can also be invoked by the user via
 brin_summarize_new_values().  Both these procedures scan all the
 unsummarized ranges, and create a summary tuple.  Again, this includes the

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