Re: [HACKERS] Use of rsync for data directory copying

2012-07-15 Thread Cédric Villemain
Le dimanche 15 juillet 2012 07:02:01, Stephen Frost a écrit :
> Bruce,
> 
> * Bruce Momjian (br...@momjian.us) wrote:
> > On Sat, Jul 14, 2012 at 09:17:22PM -0400, Stephen Frost wrote:
> > > So, can you explain which case you're specifically worried about?
> > 
> > OK.  The basic problem is that I previously was not clear about how
> > reliant our use of rsync (without --checksum) was on the presence of WAL
> > replay.
> 
> We should only be relying on WAL replay for hot backups which used
> pg_start/pg_stop_backup.
> 
> > Here is an example from our documentation that doesn't have WAL replay:
> > http://www.postgresql.org/docs/9.2/static/backup-file.html
> > 
> > Another option is to use rsync to perform a file system backup. This is
> > done by first running rsync while the database server is running, then
> > shutting down the database server just long enough to do a second rsync.
> > The second rsync will be much quicker than the first, because it has
> > relatively little data to transfer, and the end result will be
> > consistent because the server was down. This method allows a file system
> > backup to be performed with minimal downtime.
> 
> To be honest, this looks like a recommendation that might have been made
> all the way back to before we had hot backups.  Technically speaking, it
> should work fine to use the above method where the start/stop backup is
> only done for the second rsync, if there's a reason to implement such a
> system (perhaps the WALs grow too fast or too numerous for a full backup
> with rsync between the start_backup and stop_backup?).
> 
> > Now, if a write happens in both the first and second half of a second,
> > and only the first write is seen by the first rsync, I don't think the
> > second rsync will see the write, and hence the backup will be
> > inconsistent.
> 
> To be more specific, rsync relies on the combination of mtime and size
> to tell if the file has been changed or not.  In contrast, cp --update
> looks like it might only depend on mtime (from reading the cp man page
> on a Debian system).
> 
> It seems like there could be an issue where PG is writing to a file, an
> rsync comes along and copies the file, and then PG writes to that same
> file again, after the rsync is done, but within the same second.  If the
> file size isn't changed by that write, a later rsync might feel that it
> isn't necessary to check if the file contents changed.  I have to say
> that I don't believe I'v ever seen that happen though.

+1
And rsync use utimes() whenever possible (if µs modtime)...but this ignore a 
ntp update in the meantime...

-- 
Cédric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - Développement, Expertise et Formation


signature.asc
Description: This is a digitally signed message part.


Re: [HACKERS] Getting rid of pre-assignment of index names in CREATE TABLE LIKE

2012-07-15 Thread Gurjeet Singh
On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane  wrote:

>
> While I was at it, it seemed like DefineIndex's parameter list had grown
> well past any sane bound, so I refactored it to pass the IndexStmt
> struct as-is rather than passing all the fields individually.
>
> With or without that choice, though, this approach means a change in
> DefineIndex's API, as well as the contents of struct IndexStmt.  That
> means it's probably unsafe to back-patch, since it seems plausible that
> there might be third-party code out there that creates indexes and would
> use these interfaces.
>
> I would like to sneak this fix into 9.2, though.  Does anyone think
> it's already too late to be touching these APIs for 9.2?
>

I'd like us to stick to the standard practice of not changing features/API
in beta releases.

Best regards,
-- 
Gurjeet Singh
EnterpriseDB Corporation
The Enterprise PostgreSQL Company


Re: [HACKERS] Getting rid of pre-assignment of index names in CREATE TABLE LIKE

2012-07-15 Thread Tom Lane
Gurjeet Singh  writes:
> On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane  wrote:
>> I would like to sneak this fix into 9.2, though.  Does anyone think
>> it's already too late to be touching these APIs for 9.2?

> I'd like us to stick to the standard practice of not changing features/API
> in beta releases.

This is a bug fix, not a feature addition, and sometimes you can't fix
bugs without touching APIs that might be used by third party code.
So the question here is whether this bug fix is sufficiently important,
and on the other side how likely it is that anyone has already built
extensions for 9.2 that depend on IndexStmt or DefineIndex.  I don't
think trying to treat it as a "policy" matter is helpful -- it's a
tradeoff.

If you happen to know of EDB-private code that would be broken by
this change, telling us so (and why a mid-beta change would be
problematic) would be helpful.

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] [PERFORM] DELETE vs TRUNCATE explanation

2012-07-15 Thread Tom Lane
Jeff Janes  writes:
> On Thu, Jul 12, 2012 at 9:55 PM, Jeff Janes  wrote:
>> The topic was poor performance when truncating lots of small tables
>> repeatedly on test environments with fsync=off.
>> 
>> On Thu, Jul 12, 2012 at 6:00 PM, Jeff Janes  wrote:
>>> I think the problem is in the Fsync Absorption queue.  Every truncate
>>> adds a FORGET_RELATION_FSYNC to the queue, and processing each one of
>>> those leads to sequential scanning the checkpointer's pending ops hash
>>> table, which is quite large.  It is almost entirely full of other
>>> requests which have already been canceled, but it still has to dig
>>> through them all.   So this is essentially an N^2 operation.

> The attached patch addresses this problem by deleting the entry when
> it is safe to do so, and flagging it as canceled otherwise.

I don't like this patch at all.  It seems ugly and not terribly safe,
and it won't help at all when the checkpointer is in the midst of an
mdsync scan, which is a nontrivial part of its cycle.

I think what we ought to do is bite the bullet and refactor the
representation of the pendingOps table.  What I'm thinking about
is reducing the hash key to just RelFileNodeBackend + ForkNumber,
so that there's one hashtable entry per fork, and then storing a
bitmap to indicate which segment numbers need to be sync'd.  At
one gigabyte to the bit, I think we could expect the bitmap would
not get terribly large.  We'd still have a "cancel" flag in each
hash entry, but it'd apply to the whole relation fork not each
segment.

If we did this then the FORGET_RELATION_FSYNC code path could use
a hashtable lookup instead of having to traverse the table
linearly; and that would get rid of the O(N^2) performance issue.
The performance of FORGET_DATABASE_FSYNC might still suck, but
DROP DATABASE is a pretty heavyweight operation anyhow.

I'm willing to have a go at coding this design if it sounds sane.
Comments?

> Also, I still wonder if it is worth memorizing fsyncs (under
> fsync=off) that may or may not ever take place.  Is there any
> guarantee that we can make by doing so, that couldn't be made
> otherwise?

Yeah, you have a point there.  It's not real clear that switching fsync
from off to on is an operation that we can make any guarantees about,
short of executing something like the code recently added to initdb
to force-sync the entire PGDATA tree.  Perhaps we should change fsync
to be PGC_POSTMASTER (ie frozen at postmaster start), and then we could
skip forwarding fsync requests when it's off?

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] Getting rid of pre-assignment of index names in CREATE TABLE LIKE

2012-07-15 Thread Gurjeet Singh
On Sun, Jul 15, 2012 at 11:49 AM, Tom Lane  wrote:

> Gurjeet Singh  writes:
> > On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane  wrote:
> >> I would like to sneak this fix into 9.2, though.  Does anyone think
> >> it's already too late to be touching these APIs for 9.2?
>
> > I'd like us to stick to the standard practice of not changing
> features/API
> > in beta releases.
>
> This is a bug fix, not a feature addition, and sometimes you can't fix
> bugs without touching APIs that might be used by third party code.
> So the question here is whether this bug fix is sufficiently important,
> and on the other side how likely it is that anyone has already built
> extensions for 9.2 that depend on IndexStmt or DefineIndex.  I don't
> think trying to treat it as a "policy" matter is helpful -- it's a
> tradeoff.
>

I was hoping that we could fix the bug in released code without having to
change the structure or the API, but if that's not feasible, I will
withdraw my objection.


> If you happen to know of EDB-private code that would be broken by
> this change, telling us so (and why a mid-beta change would be
> problematic) would be helpful.
>

I checked, and I don't see any EDB code that would be affected by this
change.

Best regards,
-- 
Gurjeet Singh
EnterpriseDB Corporation
The Enterprise PostgreSQL Company


Re: [HACKERS] elog/ereport noreturn decoration

2012-07-15 Thread Robert Haas
On Sat, Jul 14, 2012 at 6:16 PM, Tom Lane  wrote:
> Peter Eisentraut  writes:
>> A small sidetrack here.  I've managed to set up the Solaris Studio
>> compiler on Linux and tried this out.  It turns out these "statement not
>> reached" warnings have nothing to do with knowledge about library
>> functions such as abort() or exit() not returning.  The warnings come
>> mostly from loops that never end (except by returning from the function)
>> and some other more silly cases where the supposed fallback return
>> statement is clearly unnecessary.  I think these should be fixed,
>> because the code is wrong and could mask real errors if someone ever
>> wanted to rearrange those loops or something.
>
>> Patch attached.  I tried this out with old and new versions of gcc,
>> clang, and the Solaris compiler, and everyone was happy about.  I didn't
>> touch the regex code.  And there's some archeological knowledge about
>> Perl in there.
>
> Hm.  I seem to recall that at least some of these lines were themselves
> put in to suppress compiler warnings.  So we may just be moving the
> warnings from one set of non-mainstream compilers to another set.
> Still, we might as well try it and see if there's a net reduction.
> (In some places we might need to tweak the code a bit harder than this,
> to make it clearer that the unreachable spot is unreachable.)

You mean things like this?

-
-   /* keep compiler happy */
-   return NULL;

I admit that compiler warnings are horribly annoying and we probably
ought to favor new compilers over old ones, at least in master, but
I'm kinda skeptical about the contention that no compiler anywhere
will complain if we remove hunks like that one.  The comment seems
pretty clear.  I feel like we're missing something 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.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] pgsql_fdw in contrib

2012-07-15 Thread Kohei KaiGai
2012/7/12 Etsuro Fujita :
>> 2012/6/26 Kohei KaiGai :
>> > Harada-san,
>> >
>> > I checked your patch, and had an impression that includes many
>> > improvements from the previous revision that I looked at the last
>> > commit fest.
>> >
>> > However, I noticed several points to be revised, or investigated.
>> >
>> > * It seems to me expected results of the regression test is not
>> >   attached, even though test cases were included. Please add it.
>
> KaiGai-san, Did you find the file?  That is in the contrib/pgsql_fdw/expected
> directory, named pgsql_fdw.out.  If necessary, I will send the file to you.
> BTW, I found some bugs on the expected results of the file.  Attached is a 
> patch
> fixing the bugs.
>
Fujita-san,

Yes, I overlooked the expected regression test result. Probably, I forgot
to add my git repository as Hanada-san pointed out.

BTW, your patch does not make sense in my environment that is just
after initdb without any parameter customizing. Could you give us
the step to reproduce the Nested-Loop plan from Hash-Join?

Thanks,
-- 
KaiGai Kohei 

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


Re: [HACKERS] elog/ereport noreturn decoration

2012-07-15 Thread Tom Lane
Robert Haas  writes:
> On Sat, Jul 14, 2012 at 6:16 PM, Tom Lane  wrote:
>> Hm.  I seem to recall that at least some of these lines were themselves
>> put in to suppress compiler warnings.

> You mean things like this?

> -
> - /* keep compiler happy */
> - return NULL;

That particular case appears to have been in the aboriginal commit of
the GIN code.  It's possible that Teodor's compiler complained without
it, but it seems at least as likely that (a) he was just guessing that
some compilers would complain, or (b) it was leftover from an earlier
version of the function in which the loop wasn't so obviously
non-exitable.

> I admit that compiler warnings are horribly annoying and we probably
> ought to favor new compilers over old ones, at least in master, but
> I'm kinda skeptical about the contention that no compiler anywhere
> will complain if we remove hunks like that one.  The comment seems
> pretty clear.  I feel like we're missing something here.

I don't have a whole lot of sympathy for compilers that think a
"for (;;) { ... }" loop with no break statement might terminate.
If you're going to emit warnings on the basis of reachability
analysis, I think you should be doing reachability analysis that's
not completely brain-dead.  Or else not be surprised when people
ignore your warnings.

Now, I'm prepared to believe that some of these functions might
contain cases that are not quite as black-and-white as that one.
That's why I suggested we might end up doing further code tweaking.
But at least for this example, I don't have a problem with applying
the patch and seeing what happens.

The bigger picture here is that we're threading a line between
getting warnings from compilers that are too smart, versus warnings
from compilers that are not smart enough (which could actually be
the same compiler with different settings :-().  We're not going
to be able to satisfy all cases --- but I think it's probably wise
to tilt towards satisfying the smarter compilers, when we have to
compromise.

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] isolation check takes a long time

2012-07-15 Thread Andrew Dunstan
On Fri, Jul 13, 2012 at 6:25 PM, Alvaro Herrera
wrote:

>
> Excerpts from Andrew Dunstan's message of vie jul 13 16:05:37 -0400 2012:
> > Why does the isolation check take such a long time? On some of my slower
> > buildfarm members I am thinking of disabling it because it takes so
> > long. This single test typically takes longer than a full serial
> > standard regression test. Is there any way we could make it faster?
>
> I think the "prepared transactions" test is the one that takes the
> longest.  Which is a shame when prepared xacts are not enabled, because
> all it does is throw millions of "prepared transactions are not enabled"
> errors.  There is one other test that takes very long because it commits
> a large amount of transactions.  I found it to be much faster if run
> with fsync disabled.
>
> Maybe it'd be a good idea to disable fsync on buildfarm runs, if we
> don't already do so?
>


I'm looking into that. But given that the default is to set
max_prepared_transactions to 0, shouldn't we just remove that test from the
normal installcheck schedule?

We could provide an alternative schedule that does include it.

cheers

andrew


[HACKERS] CompactCheckpointerRequestQueue versus pad bytes

2012-07-15 Thread Tom Lane
CompactCheckpointerRequestQueue supposes that it can use an entry of the
checkpointer request queue directly as a hash table key.  This will work
reliably only if there are no pad bytes in the CheckpointerRequest
struct, which means in turn that neither RelFileNodeBackend nor
RelFileNode can contain any pad bytes.

It might have accidentally failed to fail if tested on a compiler that
gives a full 32 bits to enum ForkNumber, but there absolutely would be
padding there if ForkNumber is allocated as short or char.

As best I can tell, a failure from uninitialized padding would not cause
visible misbehavior but only make it not notice that two requests are
identical, so that the queue compaction would not accomplish as much as
it should.  Nonetheless, this seems pretty broken.

We could fairly cheaply dodge the problem with padding after ForkNumber
if we were to zero out the whole request array at shmem initialization,
so that any such pad bytes are guaranteed zero.  However, padding in
RelFileNodeBackend would be more annoying to deal with, and at least
in the current instantiation of those structs it's probably impossible
anyway.  Should we document those structs as required to not contain
any padding, or do what's needful in checkpointer.c to not depend on
there not being padding?

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] [PERFORM] DELETE vs TRUNCATE explanation

2012-07-15 Thread Tom Lane
... btw, in the penny wise and pound foolish department, I observe that
smgrdounlink calls mdunlink separately for each possibly existing fork
of a relation to be dropped.  That means we are queuing a separate fsync
queue entry for each fork, and could immediately save a factor of four
in FORGET_RELATION_FSYNC traffic if we were to redefine those queue
entries as applying to all forks.  The only reason to have a per-fork
variant, AFAICS, is for smgrdounlinkfork(), which is used nowhere and
exists only because I was too chicken to remove the functionality
outright in commit ece01aae479227d9836294b287d872c5a6146a11.  But given
that we know the fsync queue can be a bottleneck, my vote is to refactor
mdunlink to apply to all forks and send only one message.

I am also wondering whether it's really necessary to send fsync request
messages for backend-local relations.  If rnode.backend says it's local,
can't we skip sending the fsync request?  All local relations are
flush-on-crash anyway, no?

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] Closing out the June commitfest

2012-07-15 Thread Tom Lane
We are now at the end of the originally scheduled one-month window for
the June commitfest.  While the numbers look fairly bad:

Needs Review: 17, Waiting on Author: 10, Ready for Committer: 3, Committed: 29, 
Returned with Feedback: 12, Rejected: 5. Total: 76.

it's not quite a complete disaster, because almost all of the "needs
review" patches did actually get some review and/or had new versions
posted during the fest.  We did not get them to the point of being
committable, but we did make progress.  I only see about three patches
that seem to have received no attention whatsoever.

At this point we could move the open items to the September fest and
call this one good, or we could keep trying to close things out.
Personally I'd like to do the former, because we really need to spend
some effort on closing out the various open issues for 9.2, and the
commitfest seems to have sucked up all the available time of those who
might've been fixing those issues over the past month.

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


Re: [HACKERS] Closing out the June commitfest

2012-07-15 Thread Josh Berkus

> At this point we could move the open items to the September fest and
> call this one good, or we could keep trying to close things out.
> Personally I'd like to do the former, because we really need to spend
> some effort on closing out the various open issues for 9.2, and the
> commitfest seems to have sucked up all the available time of those who
> might've been fixing those issues over the past month.
> 
> Thoughts?

Which three patches didn't get any review?

-- 
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] [PERFORM] DELETE vs TRUNCATE explanation

2012-07-15 Thread Craig Ringer

On 07/16/2012 02:29 AM, Tom Lane wrote:

Yeah, you have a point there.  It's not real clear that switching fsync
from off to on is an operation that we can make any guarantees about,
short of executing something like the code recently added to initdb
to force-sync the entire PGDATA tree.


There's one way that doesn't have any housekeeping cost to Pg. It's 
pretty bad manners if there's anybody other than Pg on the system though:


  sync()

Let the OS do the housekeeping.

It's possible to do something similar on Windows, in that there are 
utilities for the purpose:


  http://technet.microsoft.com/en-us/sysinternals/bb897438.aspx

This probably uses:

  http://msdn.microsoft.com/en-us/library/s9xk9ehd%28VS.71%29.aspx

from COMMODE.OBJ (unfortunate name), which has existed since win98.



Perhaps we should change fsync
to be PGC_POSTMASTER (ie frozen at postmaster start), and then we could
skip forwarding fsync requests when it's off?


Personally, I didn't even know it was runtime switchable.

fsync=off is much less necessary with async commits, group commit via 
commit delay, WAL improvements, etc. To me it's mostly of utility when 
testing, particularly on SSDs. I don't see a DB restart requirement as a 
big issue. It'd be interesting to see what -general has to say, if there 
are people depending on this.


If it's necessary to retain the ability to runtime switch it, making it 
a somewhat rude sync() in exchange for boosted performance the rest of 
the time may well be worthwhile anyway. It'd be interesting to see.


All this talk of synchronisation is making me really frustrated that 
there seems to be very poor support in OSes for syncing a set of files 
in a single pass, potentially saving a lot of time and thrashing. A way 
to relax the ordering guarantee from "Files are synced in the order 
fsync() is called on each" to "files are all synced when this call 
completes" would be great. I've been running into this issue in some 
non-Pg-related work and it's been bugging me.


--
Craig Ringer


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


Re: [HACKERS] Closing out the June commitfest

2012-07-15 Thread Josh Berkus

> Which three patches didn't get any review?

Or to be more specific: I'm in favor of closing out everything which has
had some review.  I think the three patches without any review should be
dealt with case-by-case.

-- 
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] Closing out the June commitfest

2012-07-15 Thread Peter Geoghegan
On 16 July 2012 01:16, Tom Lane  wrote:
> At this point we could move the open items to the September fest and
> call this one good, or we could keep trying to close things out.
> Personally I'd like to do the former, because we really need to spend
> some effort on closing out the various open issues for 9.2, and the
> commitfest seems to have sucked up all the available time of those who
> might've been fixing those issues over the past month.

I didn't really have the opportunity to give more feedback to any of
the three patches that I'm reviewing last week, due to other
commitments. I expect to be able to spend more time on review this
week. I think that I stand a good chance of seeing at least one of
those three committed. Hopefully the passing of the nominal deadline
will help to focus things.

-- 
Peter Geoghegan   http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and 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] [patch] libpq one-row-at-a-time API

2012-07-15 Thread Tom Lane
Marko Kreen  writes:
> Now, looking at the problem with some perspective, the solution
> is obvious: when in single-row mode, the PQgetResult() must return
> proper PGresult for that single row.  And everything else follows that.

> Such API is implemented in attached patch:

I'm starting to look at this patch now.  I think we could drop the
PQgetRowData() API: it complicates matters for little gain that I can
see.  The argument for it was to avoid the cost of creating a PGresult
per row, but we're already going to pay the cost of creating a
PGresult in order to return the PGRES_SINGLE_TUPLE status.  And as was
pointed out upthread, any per-tuple malloc costs are going to be in
the noise compared to the server-side effort expended to create the
tuple, anyway.  The point of this feature is to avoid accumulating the
entire resultset in memory, not to micro-optimize linear-time costs.

Moreover, if the argument for changing 9.2 at this late date is to get
rid of a fragile, breakable API, surely an API that's designed around
returning pointers into the library's network buffer ought to be a
prime target.

And lastly, since the proposed patch for dblink doesn't use
PQgetRowData, there's not even much reason to think that it's
bug-free.

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] Closing out the June commitfest

2012-07-15 Thread Tom Lane
Josh Berkus  writes:
>> Which three patches didn't get any review?

> Or to be more specific: I'm in favor of closing out everything which has
> had some review.  I think the three patches without any review should be
> dealt with case-by-case.

Well, I might be wrong, but the ones that don't show any activity in the
CF app are

tuplesort memory usage: grow_memtuples
https://commitfest.postgresql.org/action/patch_view?id=818

Trim trailing NULL columns
https://commitfest.postgresql.org/action/patch_view?id=840

Restrict ALTER FUNCTION CALLED ON NULL INPUT
https://commitfest.postgresql.org/action/patch_view?id=854

(Note: some of the individual patches in the "logical replication" herd
haven't been given individual reviews, but certainly that patchset as a
whole has gotten its fair share of time and more.)

None of the three above seem to me to be blocking further work,
so I don't have a hard time with punting them to September.

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] pgbench--new transaction type

2012-07-15 Thread Peter Geoghegan
On 1 June 2012 01:02, Jeff Janes  wrote:
> Sorry it has taken me a year to get back to this patch.  I have wanted
> to use it, and to ask other people to run it and report their results,
> several time recently, so I would like to get it into the core.

Who marked this patch as rejected in the commitfest app? Why?

It would be nice if this information was automatically posted as a
"comment". Surely if a patch is rejected, there should be at least a
one-line explanation.

-- 
Peter Geoghegan   http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and 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] [PERFORM] DELETE vs TRUNCATE explanation

2012-07-15 Thread Tom Lane
Craig Ringer  writes:
> On 07/16/2012 02:29 AM, Tom Lane wrote:
>> Yeah, you have a point there.  It's not real clear that switching fsync
>> from off to on is an operation that we can make any guarantees about,
>> short of executing something like the code recently added to initdb
>> to force-sync the entire PGDATA tree.

> There's one way that doesn't have any housekeeping cost to Pg. It's 
> pretty bad manners if there's anybody other than Pg on the system though:
>sync()

Yeah, I thought about that: if we could document that issuing a manual
sync after turning fsync on leaves you in a guaranteed-good state once
the sync is complete, it'd probably be fine.  However, I'm not convinced
that we could promise that with a straight face.  In the first place,
PG has only very weak guarantees about how quickly all processes in the
system will absorb a GUC update.  In the second place, I'm not entirely
sure that there aren't race conditions around checkpoints and the fsync
request queue (particularly if we do what Jeff is suggesting and
suppress queuing requests at the upstream end).  It might be all right,
or it might be all right after expending some work, but the whole thing
is not an area where I think anyone wants to spend time.  I think it'd
be much safer to document that the correct procedure is "stop the
database, do a manual sync, enable fsync in postgresql.conf, restart the
database".  And if that's what we're documenting, we lose little or
nothing by marking fsync as PGC_POSTMASTER.

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] [PERFORM] DELETE vs TRUNCATE explanation

2012-07-15 Thread Craig Ringer

On 07/16/2012 09:37 AM, Tom Lane wrote:

There's one way that doesn't have any housekeeping cost to Pg. It's
pretty bad manners if there's anybody other than Pg on the system though:
sync()

Yeah, I thought about that: if we could document that issuing a manual
sync after turning fsync on leaves you in a guaranteed-good state once
the sync is complete, it'd probably be fine.  However, I'm not convinced
that we could promise that with a straight face.  In the first place,
PG has only very weak guarantees about how quickly all processes in the
system will absorb a GUC update.  In the second place, I'm not entirely
sure that there aren't race conditions around checkpoints and the fsync
request queue (particularly if we do what Jeff is suggesting and
suppress queuing requests at the upstream end).  It might be all right,
or it might be all right after expending some work, but the whole thing
is not an area where I think anyone wants to spend time.  I think it'd
be much safer to document that the correct procedure is "stop the
database, do a manual sync, enable fsync in postgresql.conf, restart the
database".  And if that's what we're documenting, we lose little or
nothing by marking fsync as PGC_POSTMASTER.
Sounds reasonable to me; I tend to view fsync=off as a testing feature 
anyway. Will clone onto -general and see if anyone yells.


--
Craig Ringer



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


Re: [HACKERS] pgbench--new transaction type

2012-07-15 Thread Tom Lane
Peter Geoghegan  writes:
> On 1 June 2012 01:02, Jeff Janes  wrote:
>> Sorry it has taken me a year to get back to this patch.  I have wanted
>> to use it, and to ask other people to run it and report their results,
>> several time recently, so I would like to get it into the core.

> Who marked this patch as rejected in the commitfest app? Why?

You don't think this bunch of database weenies would use an app without
a transaction log, I hope ;-).  If you go to the "activity log" (link
at the upper right) it shows that Heikki did the deed, at
2012-06-20 07:34:08.  The decision seems to have been taken in the
thread ending here:
http://archives.postgresql.org/pgsql-hackers/2012-06/msg01229.php
which I observe you participated in ...

> It would be nice if this information was automatically posted as a
> "comment". Surely if a patch is rejected, there should be at least a
> one-line explanation.

I suppose Heikki should have added a comment pointing to one or another
of those messages.

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] CompactCheckpointerRequestQueue versus pad bytes

2012-07-15 Thread Amit Kapila
From: pgsql-hackers-ow...@postgresql.org
[mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tom Lane
Sent: Monday, July 16, 2012 3:06 AM

> It might have accidentally failed to fail if tested on a compiler that
> gives a full 32 bits to enum ForkNumber, but there absolutely would be
> padding there if ForkNumber is allocated as short or char.

> As best I can tell, a failure from uninitialized padding would not cause
> visible misbehavior but only make it not notice that two requests are
> identical, so that the queue compaction would not accomplish as much as
> it should.  Nonetheless, this seems pretty broken.

> We could fairly cheaply dodge the problem with padding after ForkNumber
> if we were to zero out the whole request array at shmem initialization,
> so that any such pad bytes are guaranteed zero.  However, padding in
> RelFileNodeBackend would be more annoying to deal with, and at least
> in the current instantiation of those structs it's probably impossible
> anyway.  Should we document those structs as required to not contain
> any padding, or do what's needful in checkpointer.c to not depend on
> there not being padding?

If we just document those structs, then how to handle the case where
ForkNumber
is allocated as short or char?



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