[HACKERS] pg_upgrade version check improvements and small fixes
Not sure what the normal process is for patches, but I put together a
few small patches for pg_upgrade after trying to use it earlier today
and staring a non-helpful error message before I finally figured out
what was going on.
0001 is just a simple typo fix, but didn't want to mix it in with the rest.
0002 moves a function around to be declared in the only place it is
needed, and prevents a sh: /oasdfpt/pgsql-8.4/bin/pg_config: No such
file or directory error message when you give it a bogus bindir.
0003 is what I really wanted to solve, which was my failure with
pg_upgrade. The call to pg_ctl didn't succeed because the binaries
didn't match the data directory, thus resulting in this:
$ pg_upgrade --check -d /tmp/olddata -D /tmp/newdata -b /usr/bin/ -B /usr/bin/
Performing Consistency Checks
-
Checking old data directory (/tmp/olddata) ok
Checking old bin directory (/usr/bin) ok
Checking new data directory (/tmp/newdata) ok
Checking new bin directory (/usr/bin) ok
pg_resetxlog: pg_control exists but is broken or unknown version; ignoring it
Trying to start old server .ok
Unable to start old postmaster with the command: /usr/bin/pg_ctl -l
/dev/null -D /tmp/olddata -o -p 5432 -c autovacuum=off -c
autovacuum_freeze_max_age=20 start /dev/null 21
Perhaps pg_hba.conf was not set to trust.
The error had nothing to do with trust at all; it was simply that I
tried to use 9.0 binaries with an 8.4 data directory. My patch checks
for this and ensures that the -D bindir is the correct version, just
as the -B datadir has to be the correct version.
I'm not on the mailing list nor do I have a lot of free time to keep
up with normal development, but if there are quick things I can do to
get these patches in let me know.
-Dan
From 840bdd22b62c8d45796abf7eb9e7b3da0329dce8 Mon Sep 17 00:00:00 2001
From: Dan McGee d...@archlinux.org
Date: Tue, 21 Jun 2011 18:48:01 -0500
Subject: [PATCH 1/3] pg_upgrade: fix typo in consistency check message
---
contrib/pg_upgrade/check.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
index 376d25a..2b481da 100644
--- a/contrib/pg_upgrade/check.c
+++ b/contrib/pg_upgrade/check.c
@@ -30,7 +30,7 @@ output_check_banner(bool *live_check)
if (old_cluster.port == new_cluster.port)
pg_log(PG_FATAL, When checking a live server,
the old and new port numbers must be different.\n);
- pg_log(PG_REPORT, PerForming Consistency Checks on Old Live Server\n);
+ pg_log(PG_REPORT, Performing Consistency Checks on Old Live Server\n);
pg_log(PG_REPORT, \n);
}
else
--
1.7.5.4
From f3e393318ba36ef543f77fb8983902d466ebe8c8 Mon Sep 17 00:00:00 2001
From: Dan McGee d...@archlinux.org
Date: Tue, 21 Jun 2011 18:49:47 -0500
Subject: [PATCH 2/3] pg_upgrade: remove libpath from cluster info struct
We only use this item in one check and then no longer need it.
Additionally, get_pkglibdir() is currently run before we do our checks
on the bin/ directory, so an incorrectly specified bin/ directory will
evoke failures at the wrong point.
Move the entire function into the file that uses it so it can remain
static.
---
contrib/pg_upgrade/check.c | 35 +-
contrib/pg_upgrade/option.c | 45 ---
contrib/pg_upgrade/pg_upgrade.h |1 -
3 files changed, 34 insertions(+), 47 deletions(-)
diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
index 2b481da..5ff2d81 100644
--- a/contrib/pg_upgrade/check.c
+++ b/contrib/pg_upgrade/check.c
@@ -19,6 +19,7 @@ static void check_is_super_user(ClusterInfo *cluster);
static void check_for_prepared_transactions(ClusterInfo *cluster);
static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
static void check_for_reg_data_type_usage(ClusterInfo *cluster);
+static char *get_pkglibdir(const char *bindir);
void
@@ -246,14 +247,17 @@ void
check_cluster_compatibility(bool live_check)
{
char libfile[MAXPGPATH];
+ char *libpath;
FILE *lib_test;
/*
* Test pg_upgrade_support.so is in the proper place. We cannot copy it
* ourselves because install directories are typically root-owned.
*/
- snprintf(libfile, sizeof(libfile), %s/pg_upgrade_support%s, new_cluster.libpath,
+ libpath = get_pkglibdir(new_cluster.bindir);
+ snprintf(libfile, sizeof(libfile), %s/pg_upgrade_support%s, libpath,
DLSUFFIX);
+ pg_free(libpath);
if ((lib_test = fopen(libfile, r)) == NULL)
pg_log(PG_FATAL,
@@ -730,3 +734,32 @@ check_for_reg_data_type_usage(ClusterInfo *cluster)
else
check_ok();
}
+
+
+static char *
+get_pkglibdir(const char *bindir)
+{
+ char cmd[MAXPGPATH];
+ char bufin[MAX_STRING];
+ FILE *output;
+ int i;
+
+ snprintf(cmd,
Re: [HACKERS] Indication of db-shared tables
Excerpts from Greg Sabino Mullane's message of mié jun 22 03:24:34 UTC 2011: Hash: RIPEMD160 Do we do enough to show which tables are db shared, e.g. pg_database? I don't see any indication from psql \dS. Are our docs clear enough? I don't think \dS should be indicating such a thing. I think it's documented well enough: if you are doing something that it matters enough which tables are shared, you really oughtta know about them anyway. Yeah. The user can't create new ones either, so why would it matter? -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP pgindent replacement
# Avoid bug that converts 'x =- 1' to 'x = -1' $source =~ s!=- !-= !g; I haven't looked at the shell script this replaces, but is that the correct substitution pattern? (BTW, I'm not seeing the token =- anywhere except in the Makefile, which wouldn't be run against, no? Am I missing something?) Regards, David -- David Christensen End Point Corporation da...@endpoint.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] Coding style point: const in function parameter declarations
On 22.06.2011 01:51, Tom Lane wrote: I notice that the SSI code is rather heavily invested in function declarations like this: extern bool PageIsPredicateLocked(const Relation relation, const BlockNumber blkno); I find this to be poor style, and would like to see if there's any support for getting rid of the const keywords. My objections are twofold: 1. What such a const marking actually does is to forbid the function from changing the value of its local variable that received the passed parameter value. While you may or may not think that it's good style to avoid doing so, whether the function chooses to do that or not is surely no business of its callers'. Putting such a marking into the extern declaration doesn't create any useful API contract, it just means you'll have to change the declaration if you change the function's implementation. Yeah, that makes no sense. 2. In cases such as const Relation foo where the parameter type is a typedeffed pointer, it is easy for readers to arrive at the false conclusion that this guarantees the function doesn't change the pointed-to structure. But it guarantees no such thing, because that construction is *not* equivalent to const struct RelationData *foo; rather it means struct RelationData * const foo, ie only the pointer is being const-ified, not that to which it points. The function can hack the struct contents, or pass the pointer to functions that do arbitrary things, and the compiler won't make a whimper. So I think that declarations like this are positively pernicious --- they'll mislead all but the most language-lawyerly readers. Agreed. I did not realize the difference in the SSI patch. The intention of the const declarations was clearly to document that the function doesn't modify the data the pointer points to, but if the const qualifier doesn't accomplish that, it's just wrong. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.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] Repeated PredicateLockRelation calls during seqscan
On 22.06.2011 07:58, Dan Ports wrote: I was looking at ExecSeqScan today and noticed that it invokes PredicateLockRelation each time it's called, i.e. for each tuple returned. Any reason we shouldn't skip that call if rs_relpredicatelocked is already set, as in the attached patch? That would save us a bit of overhead, since checking that flag is cheaper than doing a hash lookup in the local predicate lock table before bailing out. Hmm, I wonder if we should move this logic to heapam.c. The optimization to acquire a relation lock straight away should apply to all heap scans, not only those coming from ExecSeqScan. The distinction is academic at the moment, because that's the only caller that uses a regular MVCC snapshot, but it seems like a modularity violation for nodeSeqscan.c to reach into the HeapScanDesc to set the flag and grab the whole-relation lock, while heapam.c contains the PredicateLockTuple and CheckForSerializableConflictOut() calls. BTW, isn't bitgetpage() in nodeBitmapHeapscan.c missing PredicateLockTuple() and CheckForSerializableConflictOut() calls in the codepath for a lossy bitmap? In the non-lossy case, heap_hot_search_buffer() takes care of it, but not in the lossy case. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.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] Coding style point: const in function parameter declarations
On 22.06.2011 02:58, Dan Ports wrote: On Tue, Jun 21, 2011 at 06:51:20PM -0400, Tom Lane wrote: I find this to be poor style, and would like to see if there's any support for getting rid of the const keywords. I'm in favor of removing them too. Ok, I've removed all the useless const qualifiers. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [v9.2] DROP Reworks Part.0 - 'missing_ok' support of get_object_address
On Tue, Jun 21, 2011 at 11:11:41PM -0400, Robert Haas wrote: On Tue, Jun 21, 2011 at 11:04 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: Some of the refactoring you've done here seems likely to break things, because you're basically making the relation locking happen later than it does not, and that's going to cause problems. get_object_address_relobject() is a particularly egregious rearrangement. ?It seems to me that the right formula is to call relation_openrv() if missing_ok is false, and try_relation_openrv() if missing_ok is true. ?But that's sort of a pain, so I propose to first apply the attached patch, which gets rid of try_relation_openrv() and try_heap_openrv() and instead adds a missing_ok argument to relation_openrv() and heap_openrv(). ?If we do this, then the missing_ok argument can just be passed through all the way down. Thoughts? ?Comments? ?Objections? At least the last hunk (in pltcl.c) seems to have the flag backwards. Ah, nuts. Sorry. I think that and parse_relation.c are the only places where the try variants are used; nobody else is willing to fail, and everyone else is passing false. Revised patch attached. All existing call sites updated by this patch hardcode the flag, and only 3? proposed call sites would take advantage of the ability to not do so. The try_relation_openrv() case is fairly rare and likely to remain that way. Given that, I mildly prefer the code as it is, even if that means doing missing_ok ? try_relation_openrv() : relation_openrv() in a few places. Could always wrap that in a static function of objectaddress.c. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Make the visibility map crash-safe.
On 22.06.2011 06:05, Robert Haas wrote: Second, when inserting, updating, or deleting a tuple, we can no longer get away with clearing the visibility map bit after releasing the lock on the corresponding heap page, because an intervening crash might leave the visibility map bit set and the page-level bit clear. heap_update seems to still do that. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.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] Hugetables question
On Sun, Jun 19, 2011 at 12:56, Radosław Smogura rsmog...@softperience.eu wrote: I want to implement hugepages for shared memory Hi, Have you read this post by Tom Lane about the performance estimation and a proof-of-concept patch with hugepages? http://archives.postgresql.org/pgsql-hackers/2010-11/msg01842.php It's possible that there was a flaw in his analysis, but his conclusion is that it's not worth it: And the bottom line is: if there's any performance benefit at all, it's on the order of 1%. The best result I got was about 3200 TPS with hugepages, and about 3160 without. The noise in these numbers is more than 1% though. Regards, Marti -- 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] Latch implementation that wakes on postmaster death on both win32 and Unix
Attached patch addresses Fujii's more recent concerns.
On 22 June 2011 04:54, Fujii Masao masao.fu...@gmail.com wrote:
+WaitLatch(volatile Latch *latch, int wakeEvents, long timeout)
+WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket
sock, long timeout)
If 'wakeEvent' is zero, we cannot get out of WaitLatch(). Something like
Assert(waitEvents != 0) is required? Or, WaitLatch() should always wait
on latch even when 'waitEvents' is zero?
Well, not waking when the client has not specified an event to wake on
is the correct thing to do in that case. It would also be inherently
undesirable, so I'd be happy to guard against it using an assertion.
Both implementations now use one.
In unix_latch.c, select() in WaitLatchOrSocket() checks the timeout only when
WL_TIMEOUT is set in 'wakeEvents'. OTOH, in win32_latch.c,
WaitForMultipleObjects()
in WaitLatchOrSocket() always checks the timeout even if WL_TIMEOUT is not
given. Is this intentional?
No, it's a mistake. Fixed.
+ else if (rc == WAIT_OBJECT_0 + 2
+ ((wakeEvents WL_SOCKET_READABLE) ||
(wakeEvents WL_SOCKET_WRITEABLE)))
Another corner case: when WL_POSTMASTER_DEATH and WL_SOCKET_READABLE
are given and 'sock' is set to PGINVALID_SOCKET, we can wrongly pass through
the
above check. If this OK?
I see your point - Assert(sock != PGINVALID_SOCKET) could be violated.
We fix the issue now by setting and checking a bool that simply
indicates that we're interested in sockets.
rc = WaitForMultipleObjects(numevents, events, FALSE,
(timeout = 0) ?
(timeout / 1000) : INFINITE);
- if (rc == WAIT_FAILED)
+ if ( (wakeEvents WL_POSTMASTER_DEATH)
+ !PostmasterIsAlive(true))
After WaitForMultipleObjects() detects the death of postmaster,
WaitForSingleObject()
is called in PostmasterIsAlive(). In this case, what code does
WaitForSingleObject() return?
I wonder if WaitForSingleObject() returns the code other than
WAIT_TIMEOUT and really
can detect the death of postmaster.
As noted up-thread, the fact that the archiver does wake and finish on
Postmaster death can be clearly observed on Windows. I'm not sure why
you wonder that, as this is fairly standard use of
PostmasterIsAlive(). I've verified that the waitLatch() call
correctly reports Postmaster death in its return code on Windows, and
indeed that it actually wakes up.
Are you suggesting that there should be a defensive else if{ } for the
case where PostmasterIsAlive() incorrectly reports that the PM is
alive due to some implementation related race-condition, and we've
already considered every other possibility? Well, I suppose that's not
necessary, because we will loop until we find a reason - it's okay to
miss it the first time around, because whatever caused
WaitForMultipleObjects() to wake up will cause it to immediately
return for the next iteration.
In any case, we don't rely on the PostmasterIsAlive() call at all
anymore, so it doesn't matter. We just look at rc's value now, as we
do for every other case, though it's a bit trickier when checking
Postmaster death. Similarly, we don't have a PostmasterIsAlive() call
within the unix latch implementation anymore.
+ if (fcntl(postmaster_alive_fds[POSTMASTER_FD_WATCH], F_GETFL) 0)
+ {
+ ereport(FATAL,
+ (errcode_for_socket_access(),
+ errmsg(failed to set the postmaster death watching
fd's flags:
%s, strerror(errno;
+ }
Is the above check really required? It's harmless, but looks unnecessary.
Yes, it's not possible for it to detect an error condition now. Removed.
'%m' should be used instead of '%s' and 'strerror(errno)'.
It is of course better to use the simpler, built-in facility. Fixed.
--
Peter Geoghegan http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 4952d22..bfe6bcd 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -10158,7 +10158,7 @@ retry:
/*
* Wait for more WAL to arrive, or timeout to be reached
*/
- WaitLatch(XLogCtl-recoveryWakeupLatch, 500L);
+ WaitLatch(XLogCtl-recoveryWakeupLatch, WL_LATCH_SET | WL_TIMEOUT, 500L);
ResetLatch(XLogCtl-recoveryWakeupLatch);
}
else
diff --git a/src/backend/port/unix_latch.c b/src/backend/port/unix_latch.c
index 6dae7c9..6d2e3a1 100644
--- a/src/backend/port/unix_latch.c
+++ b/src/backend/port/unix_latch.c
@@ -93,6 +93,7 @@
#endif
#include miscadmin.h
+#include postmaster/postmaster.h
#include storage/latch.h
#include storage/shmem.h
@@ -188,22 +189,25 @@ DisownLatch(volatile Latch *latch)
* backend-local latch initialized with InitLatch, or a shared latch
*
Re: [HACKERS] Hugetables question
On Wed, 22 Jun 2011 14:24:17 +0300, Marti Raudsepp wrote: On Sun, Jun 19, 2011 at 12:56, Radosław Smogura rsmog...@softperience.eu wrote: I want to implement hugepages for shared memory Hi, Have you read this post by Tom Lane about the performance estimation and a proof-of-concept patch with hugepages? http://archives.postgresql.org/pgsql-hackers/2010-11/msg01842.php It's possible that there was a flaw in his analysis, but his conclusion is that it's not worth it: And the bottom line is: if there's any performance benefit at all, it's on the order of 1%. The best result I got was about 3200 TPS with hugepages, and about 3160 without. The noise in these numbers is more than 1% though. Regards, Marti Actually when I tried to implement hugepages for palloc (I ware unable to write fast and effective mallocator), my result was that when I was using normal pages I got small performance degree, but when I was using huge pages this was faster then normal build (even with infective mallocator). I know there are some problems with accessing higher memory (when server is more then 8GB), and hugepages may resolve this. I strictly disagree with opinion if there is 1% it's worthless. 1% here, 1% there, and finally You get 10%, but of course hugepages will work quite well if will be used in code that require many random jumps. I think this can be reproduced and some not-common case may be found to get performance of about 10% (maybe upload whole table in shared buffer and randomly peek records one by one). Regards, Radek -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP pgindent replacement
On 06/22/2011 02:03 AM, David Christensen wrote: # Avoid bug that converts 'x =- 1' to 'x = -1' $source =~ s!=- !-= !g; I haven't looked at the shell script this replaces, but is that the correct substitution pattern? (BTW, I'm not seeing the token =- anywhere except in the Makefile, which wouldn't be run against, no? Am I missing something?) It's exactly what the current script does. The reason you don't see this anywhere is that previous pgindent runs have removed it. We don't undo the transformation. But maybe we should just get rid of it. 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] [v9.2] DROP Reworks Part.0 - 'missing_ok' support of get_object_address
On Wed, Jun 22, 2011 at 6:18 AM, Noah Misch n...@leadboat.com wrote: On Tue, Jun 21, 2011 at 11:11:41PM -0400, Robert Haas wrote: On Tue, Jun 21, 2011 at 11:04 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: Some of the refactoring you've done here seems likely to break things, because you're basically making the relation locking happen later than it does not, and that's going to cause problems. get_object_address_relobject() is a particularly egregious rearrangement. ?It seems to me that the right formula is to call relation_openrv() if missing_ok is false, and try_relation_openrv() if missing_ok is true. ?But that's sort of a pain, so I propose to first apply the attached patch, which gets rid of try_relation_openrv() and try_heap_openrv() and instead adds a missing_ok argument to relation_openrv() and heap_openrv(). ?If we do this, then the missing_ok argument can just be passed through all the way down. Thoughts? ?Comments? ?Objections? At least the last hunk (in pltcl.c) seems to have the flag backwards. Ah, nuts. Sorry. I think that and parse_relation.c are the only places where the try variants are used; nobody else is willing to fail, and everyone else is passing false. Revised patch attached. All existing call sites updated by this patch hardcode the flag, and only 3? proposed call sites would take advantage of the ability to not do so. The try_relation_openrv() case is fairly rare and likely to remain that way. Given that, I mildly prefer the code as it is, even if that means doing missing_ok ? try_relation_openrv() : relation_openrv() in a few places. Could always wrap that in a static function of objectaddress.c. I don't really like the idea of having a static function in objectaddress.c, because I think there will eventually be more callers who sometimes want to pass missing_ok = true and other times pass missing_ok = false. Plus, it seems a little nutty to have a function that, depending on whether its argument is true or false, calls one of two other functions which are virtually cut-and-paste copies of each other, except that one internally has true and the other false. I just work here, though. Another option might be to leave heap_openrv() and relation_openrv() alone and add a missing_ok argument to try_heap_openrv() and try_relation_openrv(). Passing true would give the same behavior as presently; passing false would make them behave like the non-try version. -- 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] WIP pgindent replacement
On 06/22/2011 08:35 AM, Andrew Dunstan wrote: On 06/22/2011 02:03 AM, David Christensen wrote: # Avoid bug that converts 'x =- 1' to 'x = -1' $source =~ s!=- !-= !g; I haven't looked at the shell script this replaces, but is that the correct substitution pattern? (BTW, I'm not seeing the token =- anywhere except in the Makefile, which wouldn't be run against, no? Am I missing something?) It's exactly what the current script does. The reason you don't see this anywhere is that previous pgindent runs have removed it. We don't undo the transformation. But maybe we should just get rid of it. Further research shows that C89 explicitly dropped support for the old KR =- operator, so we probably *should* remove this in case it introduces an unintended bug. 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] WIP pgindent replacement
Andrew Dunstan wrote: On 06/22/2011 08:35 AM, Andrew Dunstan wrote: On 06/22/2011 02:03 AM, David Christensen wrote: # Avoid bug that converts 'x =- 1' to 'x = -1' $source =~ s!=- !-= !g; I haven't looked at the shell script this replaces, but is that the correct substitution pattern? (BTW, I'm not seeing the token =- anywhere except in the Makefile, which wouldn't be run against, no? Am I missing something?) It's exactly what the current script does. The reason you don't see this anywhere is that previous pgindent runs have removed it. We don't undo the transformation. But maybe we should just get rid of it. Further research shows that C89 explicitly dropped support for the old KR =- operator, so we probably *should* remove this in case it introduces an unintended bug. Well, the point is if someone does use that, it isn't going to generate a pgindent error, but rather produce incorrect C code because =- is going to be changed. FYI, my gcc 2.95.3 allows =- and does work as intended. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Indication of db-shared tables
Alvaro Herrera wrote: Excerpts from Greg Sabino Mullane's message of mi?? jun 22 03:24:34 UTC 2011: Hash: RIPEMD160 Do we do enough to show which tables are db shared, e.g. pg_database? I don't see any indication from psql \dS. Are our docs clear enough? I don't think \dS should be indicating such a thing. I think it's documented well enough: if you are doing something that it matters enough which tables are shared, you really oughtta know about them anyway. Yeah. The user can't create new ones either, so why would it matter? I assumed it was important to indicate if someone was looking at per-database or per-cluster data, like pg_tablespace. The issue comes up when I do admin training about the system tables. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] smallserial / serial2
On Tue, Jun 21, 2011 at 10:58 PM, Robert Haas robertmh...@gmail.com wrote: Committed the main patch, and your regression tests. Hmph, looks like buildfarm members koi and jaguar are failing make check now: http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=koidt=2011-06-22%2008%3A06%3A00 due to a difference in sequence.out. I didn't muck with the part about SELECT * FROM foo_seq_new; which is causing the diff, but it looks like the only difference is in the log_cnt column, which seems pretty fragile to rely on in a regression test. Maybe those SELECTS just shouldn't include log_cnt. Josh -- 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] smallserial / serial2
On Wed, Jun 22, 2011 at 9:14 AM, Josh Kupershmidt schmi...@gmail.com wrote: On Tue, Jun 21, 2011 at 10:58 PM, Robert Haas robertmh...@gmail.com wrote: Committed the main patch, and your regression tests. Hmph, looks like buildfarm members koi and jaguar are failing make check now: http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=koidt=2011-06-22%2008%3A06%3A00 due to a difference in sequence.out. I didn't muck with the part about SELECT * FROM foo_seq_new; which is causing the diff, but it looks like the only difference is in the log_cnt column, which seems pretty fragile to rely on in a regression test. Maybe those SELECTS just shouldn't include log_cnt. Seems like a reasonable thing to try. Done. -- 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] Indication of db-shared tables
Greetings, On Wednesday, June 22, 2011 09:10:02 AM Bruce Momjian wrote: I assumed it was important to indicate if someone was looking at per-database or per-cluster data, like pg_tablespace. The issue comes up when I do admin training about the system tables. +1 I favor features that make training easier for the teacher. Regards, J -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP pgindent replacement
On 06/22/2011 09:08 AM, Bruce Momjian wrote: Andrew Dunstan wrote: On 06/22/2011 08:35 AM, Andrew Dunstan wrote: On 06/22/2011 02:03 AM, David Christensen wrote: # Avoid bug that converts 'x =- 1' to 'x = -1' $source =~ s!=- !-= !g; I haven't looked at the shell script this replaces, but is that the correct substitution pattern? (BTW, I'm not seeing the token =- anywhere except in the Makefile, which wouldn't be run against, no? Am I missing something?) It's exactly what the current script does. The reason you don't see this anywhere is that previous pgindent runs have removed it. We don't undo the transformation. But maybe we should just get rid of it. Further research shows that C89 explicitly dropped support for the old KR =- operator, so we probably *should* remove this in case it introduces an unintended bug. Well, the point is if someone does use that, it isn't going to generate a pgindent error, but rather produce incorrect C code because =- is going to be changed. FYI, my gcc 2.95.3 allows =- and does work as intended. As intended by whom? If the effect of x=4; x =- 1; is to subtract 1 from x then that's simply wrong by C89. It should assign -1 to x. The =- must be parsed as two operators in C89, assignment and unary minus. pgindent should not under any circumstances change the semantics of the program being indented, and that's what this transformation does for compilers conforming to the standard we explicitly follow. What happens when your ancient gcc is told to apply the ansi standard? 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] WIP pgindent replacement
Andrew Dunstan wrote: Further research shows that C89 explicitly dropped support for the old KR =- operator, so we probably *should* remove this in case it introduces an unintended bug. Well, the point is if someone does use that, it isn't going to generate a pgindent error, but rather produce incorrect C code because =- is going to be changed. FYI, my gcc 2.95.3 allows =- and does work as intended. As intended by whom? If the effect of x=4; x =- 1; is to subtract 1 from x then that's simply wrong by C89. It should assign -1 to x. The =- must be parsed as two operators in C89, assignment and unary minus. pgindent should not under any circumstances change the semantics of the program being indented, and that's what this transformation does for compilers conforming to the standard we explicitly follow. What happens when your ancient gcc is told to apply the ansi standard? I see now that my test wasn't complete. You are right it assigns -1 so we can remove this from pgupgrade. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP Reworks Part.0 - 'missing_ok' support of get_object_address
Robert Haas robertmh...@gmail.com writes: Another option might be to leave heap_openrv() and relation_openrv() alone and add a missing_ok argument to try_heap_openrv() and try_relation_openrv(). +1 for that, although the try_ prefix might be inappropriate naming now; how about cond_relation_openrv? 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] Repeated PredicateLockRelation calls during seqscan
Dan Ports d...@csail.mit.edu writes: I was looking at ExecSeqScan today and noticed that it invokes PredicateLockRelation each time it's called, i.e. for each tuple returned. Any reason we shouldn't skip that call if rs_relpredicatelocked is already set, as in the attached patch? Why is the call in ExecSeqScan at all, and not in the node startup function? 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_dump vs malloc
On Fri, Jun 10, 2011 at 21:07, Tom Lane t...@sss.pgh.pa.us wrote:
Magnus Hagander mag...@hagander.net writes:
I came across a situation today with a pretty bad crash of pg_dump,
due to not checking the return code from malloc(). When looking
through the code, it seems there are a *lot* of places in pg_dump that
doesn't check the malloc return code.
But we do have a pg_malloc() function in there - but from what I can
tell it's only used very sparsely?
Shouldn't we be using that one more or less everywhere
Yup. Have at it.
Something along the line of this?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
diff --git a/src/bin/pg_dump/Makefile b/src/bin/pg_dump/Makefile
index 8410af1..0737505 100644
--- a/src/bin/pg_dump/Makefile
+++ b/src/bin/pg_dump/Makefile
@@ -20,7 +20,7 @@ override CPPFLAGS := -I$(libpq_srcdir) $(CPPFLAGS)
OBJS= pg_backup_archiver.o pg_backup_db.o pg_backup_custom.o \
pg_backup_files.o pg_backup_null.o pg_backup_tar.o \
- pg_backup_directory.o dumputils.o compress_io.o $(WIN32RES)
+ pg_backup_directory.o pg_dump_alloc.o dumputils.o compress_io.o $(WIN32RES)
KEYWRDOBJS = keywords.o kwlookup.o
@@ -35,8 +35,8 @@ pg_dump: pg_dump.o common.o pg_dump_sort.o $(OBJS) $(KEYWRDOBJS) | submake-libpq
pg_restore: pg_restore.o $(OBJS) $(KEYWRDOBJS) | submake-libpq submake-libpgport
$(CC) $(CFLAGS) pg_restore.o $(KEYWRDOBJS) $(OBJS) $(libpq_pgport) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
-pg_dumpall: pg_dumpall.o dumputils.o $(KEYWRDOBJS) | submake-libpq submake-libpgport
- $(CC) $(CFLAGS) pg_dumpall.o dumputils.o $(KEYWRDOBJS) $(WIN32RES) $(libpq_pgport) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
+pg_dumpall: pg_dumpall.o dumputils.o pg_dump_alloc.o $(KEYWRDOBJS) | submake-libpq submake-libpgport
+ $(CC) $(CFLAGS) pg_dumpall.o dumputils.o pg_dump_alloc.o $(KEYWRDOBJS) $(WIN32RES) $(libpq_pgport) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
install: all installdirs
$(INSTALL_PROGRAM) pg_dump$(X) '$(DESTDIR)$(bindir)'/pg_dump$(X)
diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index a631f64..b194f24 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -980,57 +980,3 @@ simple_string_list_member(SimpleStringList *list, const char *val)
}
-/*
- * Safer versions of some standard C library functions. If an
- * out-of-memory condition occurs, these functions will bail out
- * safely; therefore, their return value is guaranteed to be non-NULL.
- *
- * XXX need to refactor things so that these can be in a file that can be
- * shared by pg_dumpall and pg_restore as well as pg_dump.
- */
-
-char *
-pg_strdup(const char *string)
-{
- char *tmp;
-
- if (!string)
- exit_horribly(NULL, NULL, cannot duplicate null pointer\n);
- tmp = strdup(string);
- if (!tmp)
- exit_horribly(NULL, NULL, out of memory\n);
- return tmp;
-}
-
-void *
-pg_malloc(size_t size)
-{
- void *tmp;
-
- tmp = malloc(size);
- if (!tmp)
- exit_horribly(NULL, NULL, out of memory\n);
- return tmp;
-}
-
-void *
-pg_calloc(size_t nmemb, size_t size)
-{
- void *tmp;
-
- tmp = calloc(nmemb, size);
- if (!tmp)
- exit_horribly(NULL, NULL, out of memory\n);
- return tmp;
-}
-
-void *
-pg_realloc(void *ptr, size_t size)
-{
- void *tmp;
-
- tmp = realloc(ptr, size);
- if (!tmp)
- exit_horribly(NULL, NULL, out of memory\n);
- return tmp;
-}
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 0e1037c..78b5e93 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -735,8 +735,6 @@ ArchiveEntry(Archive *AHX,
TocEntry *newToc;
newToc = (TocEntry *) calloc(1, sizeof(TocEntry));
- if (!newToc)
- die_horribly(AH, modulename, out of memory\n);
AH-tocCount++;
if (dumpId AH-maxDumpId)
@@ -1131,8 +1129,6 @@ archprintf(Archive *AH, const char *fmt,...)
free(p);
bSize *= 2;
p = (char *) malloc(bSize);
- if (p == NULL)
- exit_horribly(AH, modulename, out of memory\n);
va_start(ap, fmt);
cnt = vsnprintf(p, bSize, fmt, ap);
va_end(ap);
@@ -1266,8 +1262,6 @@ ahprintf(ArchiveHandle *AH, const char *fmt,...)
free(p);
bSize *= 2;
p = (char *) malloc(bSize);
- if (p == NULL)
- die_horribly(AH, modulename, out of memory\n);
va_start(ap, fmt);
cnt = vsnprintf(p, bSize, fmt, ap);
va_end(ap);
@@ -1736,8 +1730,6 @@ ReadStr(ArchiveHandle *AH)
else
{
buf = (char *) malloc(l + 1);
- if (!buf)
- die_horribly(AH, modulename, out of memory\n);
if ((*AH-ReadBufPtr) (AH, (void *) buf, l) != l)
die_horribly(AH, modulename, unexpected end of file\n);
@@ -1930,8 +1922,6 @@ _allocAH(const char *FileSpec, const ArchiveFormat fmt,
#endif
AH = (ArchiveHandle *) calloc(1, sizeof(ArchiveHandle));
- if (!AH)
- die_horribly(AH, modulename, out of memory\n);
/* AH-debugLevel = 100; */
@@ -1976,8 +1966,6 @@ _allocAH(const char *FileSpec, const ArchiveFormat fmt,
AH-currWithOids = -1; /* force SET
Re: [HACKERS] smallserial / serial2
Josh Kupershmidt schmi...@gmail.com writes: Hmph, looks like buildfarm members koi and jaguar are failing make check now: http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=koidt=2011-06-22%2008%3A06%3A00 due to a difference in sequence.out. I didn't muck with the part about SELECT * FROM foo_seq_new; which is causing the diff, but it looks like the only difference is in the log_cnt column, which seems pretty fragile to rely on in a regression test. Maybe those SELECTS just shouldn't include log_cnt. We've been around on this before: http://archives.postgresql.org/pgsql-hackers/2008-08/msg01359.php The eventual solution was bb3f839bfcb396c3066a31559d3a72ef0b4b5fae, which is not consistent with what Robert just did, in fact he forgot about that variant output file altogether (which probably means we'll still get occasional failure reports). That previous approach of adding extra expected files isn't going to scale nicely if there are multiple places at risk ... but do we need multiple places selecting the sequence contents? I remain of the opinion that just omitting the value isn't good testing policy. 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] ALTER TABLE lock strength reduction patch is unsafe
On Tue, Jun 21, 2011 at 4:12 PM, Tom Lane t...@sss.pgh.pa.us wrote: Simon Riggs si...@2ndquadrant.com writes: On Mon, Jun 20, 2011 at 10:56 PM, Tom Lane t...@sss.pgh.pa.us wrote: The ALTER TABLE patch has greatly expanded the scope of the issue, and that *is* a regression compared to prior releases. I agree the scope for RELOID errors increased with my 9.1 patch. I'm now happy with the locking patch (attached), which significantly reduces the scope - back to the original error scope, in my testing. I tried to solve both, but I think that's a step too far given the timing. It seems likely that there will be objections to this patch. Yup, you're right. Having read this patch, I have absolutely zero confidence in it. It introduces some locks in random places, with no rhyme or reason that I can see. There is something to be salvaged here and I have additional analysis and suggestions. Please read all the way through. We already allowed INHERIT/dis INHERIT with a lower lock level than AccessExclusiveLock and have received no complaints to speak of. We aren't discussing moving that operation to AccessExclusiveLock in the light of this discussion, nor are we discussing TYPE OF operations. I don't think its sensible to reset those operations to AccessExclusiveLock. The patch has extensive comments that explain the carefully analysed and tested locations for the locking. We take an ExclusiveLock on the RelationDefinition during an ALTER TABLE. We take a ShareLock on CatCache and RelCache rebuilds. The only time these can block is during an ALTER TABLE with a lower lock than AccessExclusiveLock that occurs immediately after other DDL. The locking has been placed to minimise the lock window for simple ALTER TABLE statements. There is no reason to think that this is a complete solution, and considerable reason to think that it isn't (notably, the RELOID syscache is hardly the only one at risk). The whole of the relcache rebuild is protected, not just access to pg_class, so that is safe, no matter which catalog tables are accessed. Catcache does look more complex, so your fears here are worth looking into. I was already doing that since my last post and have now finished. Based on the above, I think we should limit the patch to these areas: 1. Simple operations on pg_class and/or pg_attribute. This includes case AT_SetStatistics: ATTNAME catcache case AT_SetOptions: ATTNAME catcache case AT_ResetOptions: ATTNAME catcache case AT_SetStorage:ATTNAME catcache case AT_SetRelOptions: RELOID catcache case AT_ResetRelOptions: RELOID catcache 2. FK VALIDATION which touches CONSTROID Limiting the scope of the patch to those areas provides good benefit, whilst at the same time limiting our risk footprint. Doing that means we can evaluate the patch against those aspects. For people that still think there is still risk, I've added a parameter called relaxed_ddl_locking which defaults to off. That way people can use this feature in an informed way without exposing us to support risks from its use. [Patch needs minor docs and test documentation changes prior to commit] Worse, it's adding more locking in performance-critical places, which seems to me to severely degrade the argument for the original feature, namely that it was supposed to give us *less* locking. I think we should be clear that this adds *one* lock/unlock for each object for each session, ONCE only after each transaction that executed a DDL statement against an object. So with 200 sessions, a typical ALTER TABLE statement will cause 400 locks. The current lock manager maxes out above 50,000 locks per second, so any comparative analysis will show this is a minor blip on even a heavy workload. It isn't in a performance critical place and the lock held in case of a rebuild is a ShareLock so won't block, accept against an ALTER TABLE statement. This is a marked *reduction* in locking overhead, in comparison with ALTER TABLE being an AccessExclusiveLock which would make the SQL statement wait for potentially a very long time. With relaxed_ddl_locking = off (default) there is zero effect. So that is not a valid argument to refuse this patch. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services relaxed_ddl_locking.v4.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] smallserial / serial2
I wrote: That previous approach of adding extra expected files isn't going to scale nicely if there are multiple places at risk ... but do we need multiple places selecting the sequence contents? I remain of the opinion that just omitting the value isn't good testing policy. Actually, on looking closer, you didn't add additional selections from sequences. The real problem here is simply that you forgot to update expected/sequence_1.out altogether. So Robert's fix should be reverted in favor of doing that. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump vs malloc
Magnus Hagander mag...@hagander.net writes: Something along the line of this? I think this is a seriously, seriously bad idea: +#define strdup(x) pg_strdup(x) +#define malloc(x) pg_malloc(x) +#define calloc(x,y) pg_calloc(x, y) +#define realloc(x,y) pg_realloc(x, y) as it will render the code unreadable to people expecting the normal behavior of these fundamental functions; not to mention break any call sites that have some other means of dealing with an alloc failure besides going belly-up. Please take the trouble to do s/malloc/pg_malloc/g and so on, instead. 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_dump vs malloc
On 22 June 2011 16:25, Magnus Hagander mag...@hagander.net wrote: Something along the line of this? IMHO the redefinition of malloc() looks a bit hairy...can't you just make the callers use the functions directly? -- 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] pg_dump vs malloc
On Wed, Jun 22, 2011 at 17:48, Tom Lane t...@sss.pgh.pa.us wrote: Magnus Hagander mag...@hagander.net writes: Something along the line of this? I think this is a seriously, seriously bad idea: +#define strdup(x) pg_strdup(x) +#define malloc(x) pg_malloc(x) +#define calloc(x,y) pg_calloc(x, y) +#define realloc(x,y) pg_realloc(x, y) as it will render the code unreadable to people expecting the normal behavior of these fundamental functions; not to mention break any call sites that have some other means of dealing with an alloc failure besides going belly-up. Please take the trouble to do s/malloc/pg_malloc/g and so on, instead. Ok, I'll try that approach. This seemed like a nicer approach, but I think once written out, i agree with your arguments :-) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: Coding style point: const in function parameter declarations
Tom Lane t...@sss.pgh.pa.us wrote: 2. In cases such as const Relation foo where the parameter type is a typedeffed pointer, it is easy for readers to arrive at the false conclusion that this guarantees the function doesn't change the pointed-to structure. So easy that in fact that was my belief when I wrote it. Since that was wrong, I totally support the (now accomplished) removal of the useless and misleading qualifier. Thanks for spotting this mistaken assumption. -Kevin -- 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] Coding style point: const in function parameter declarations
On Tue, Jun 21, 2011 at 5:51 PM, Tom Lane t...@sss.pgh.pa.us wrote: Declarations like const structtype *param are fine, because those create a real, enforced contract on what the function can do to data that is visible to its caller. But I don't see any value at all in const-ifying the parameter itself. Comments? What about making a separate typedef for that (like ConstRelation)? Maybe the const contract is useful, and aren't there occasional performance enhancements the compiler can make when it function arguments are const? merlin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Repeated PredicateLockRelation calls during seqscan
Tom Lane t...@sss.pgh.pa.us wrote: Why is the call in ExecSeqScan at all, and not in the node startup function? Because when I asked about the placement of such calls in January of 2010 I didn't get any advice which suggested that, and this was a place I was able to find which worked correctly. If there's a better place, based on performance and/or modularity needs, let's use it. -Kevin -- 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] Coding style point: const in function parameter declarations
Merlin Moncure mmonc...@gmail.com writes: On Tue, Jun 21, 2011 at 5:51 PM, Tom Lane t...@sss.pgh.pa.us wrote: Declarations like const structtype *param are fine, because those create a real, enforced contract on what the function can do to data that is visible to its caller. But I don't see any value at all in const-ifying the parameter itself. What about making a separate typedef for that (like ConstRelation)? Well, any change along this line would require pretty widespread changes, as you'd have to const-ify from the bottom up, or else hold your nose and cast-away-const in assorted calls (which I think is so evil you might as well not have the const in the first place). If we were thinking of moving in that direction, I would argue that we should get rid of typedef'd pointers altogether, ie, change Relation to be a typedef for the struct and write Relation *rel not Relation rel. Then, attaching a const to the front is easy, natural, and does what the naive reader would expect. But this would be a sufficiently sweeping change that I'm not sure the benefits would be worth the cost. 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] [v9.2] DROP Reworks Part.0 - 'missing_ok' support of get_object_address
Excerpts from Robert Haas's message of mié jun 22 08:56:02 -0400 2011: Another option might be to leave heap_openrv() and relation_openrv() alone and add a missing_ok argument to try_heap_openrv() and try_relation_openrv(). Passing true would give the same behavior as presently; passing false would make them behave like the non-try version. That would be pretty weird, having two functions, one of them sometimes doing the same thing as the other one. I understand Noah's concern but I think your original proposal was saner than both options presented so far. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- 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] Indication of db-shared tables
Excerpts from Jeff MacDonald's message of mié jun 22 09:27:36 -0400 2011: Greetings, On Wednesday, June 22, 2011 09:10:02 AM Bruce Momjian wrote: I assumed it was important to indicate if someone was looking at per-database or per-cluster data, like pg_tablespace. The issue comes up when I do admin training about the system tables. +1 I favor features that make training easier for the teacher. So what UI would you guys propose? I don't think widening the output of frequently used commands like \d or \d+ is okay for a feature with this small a use case. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP Reworks Part.0 - 'missing_ok' support of get_object_address
On Wed, Jun 22, 2011 at 12:51 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Robert Haas's message of mié jun 22 08:56:02 -0400 2011: Another option might be to leave heap_openrv() and relation_openrv() alone and add a missing_ok argument to try_heap_openrv() and try_relation_openrv(). Passing true would give the same behavior as presently; passing false would make them behave like the non-try version. That would be pretty weird, having two functions, one of them sometimes doing the same thing as the other one. I understand Noah's concern but I think your original proposal was saner than both options presented so far. I agree with you. If we had a whole pile of options it might be worth having heap_openrv() and heap_openrv_extended() so as not to complicate the simple case, but since there's no forseeable need to add anything other than missing_ok, my gut is to just add it and call it good. -- 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] Repeated PredicateLockRelation calls during seqscan
On 22.06.2011 17:28, Tom Lane wrote: Dan Portsd...@csail.mit.edu writes: I was looking at ExecSeqScan today and noticed that it invokes PredicateLockRelation each time it's called, i.e. for each tuple returned. Any reason we shouldn't skip that call if rs_relpredicatelocked is already set, as in the attached patch? Why is the call in ExecSeqScan at all, and not in the node startup function? It makes sense to delay it until the scan is actually started, so that you don't get unnecessary serialization failures if the scan is never actually executed. I don't know if that was Kevin's original reason for putting it there, but that's why I left it there. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.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] Indication of db-shared tables
Alvaro Herrera wrote: Excerpts from Jeff MacDonald's message of mi?? jun 22 09:27:36 -0400 2011: Greetings, On Wednesday, June 22, 2011 09:10:02 AM Bruce Momjian wrote: I assumed it was important to indicate if someone was looking at per-database or per-cluster data, like pg_tablespace. The issue comes up when I do admin training about the system tables. +1 I favor features that make training easier for the teacher. So what UI would you guys propose? I don't think widening the output of frequently used commands like \d or \d+ is okay for a feature with this small a use case. I am going to try to add descriptions for system tables for \d+ so maybe I can put it there. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Coding style point: const in function parameter declarations
On Wed, Jun 22, 2011 at 12:41 PM, Tom Lane t...@sss.pgh.pa.us wrote: Merlin Moncure mmonc...@gmail.com writes: On Tue, Jun 21, 2011 at 5:51 PM, Tom Lane t...@sss.pgh.pa.us wrote: Declarations like const structtype *param are fine, because those create a real, enforced contract on what the function can do to data that is visible to its caller. But I don't see any value at all in const-ifying the parameter itself. What about making a separate typedef for that (like ConstRelation)? Well, any change along this line would require pretty widespread changes, as you'd have to const-ify from the bottom up, or else hold your nose and cast-away-const in assorted calls (which I think is so evil you might as well not have the const in the first place). If we were thinking of moving in that direction, I would argue that we should get rid of typedef'd pointers altogether, ie, change Relation to be a typedef for the struct and write Relation *rel not Relation rel. Then, attaching a const to the front is easy, natural, and does what the naive reader would expect. But this would be a sufficiently sweeping change that I'm not sure the benefits would be worth the cost. I don't really see the point, either. I mean, the Relation is basically a cache. And so it could happen that we want to start caching something we don't cache today. Then someone for whom it was const suddenly needs it to be non-const. Of course I don't think there's much call for monkeying with rel-rd_id but how likely is that to be a real source of coding errors? -- 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] Repeated PredicateLockRelation calls during seqscan
Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: On 22.06.2011 17:28, Tom Lane wrote: Why is the call in ExecSeqScan at all, and not in the node startup function? It makes sense to delay it until the scan is actually started, so that you don't get unnecessary serialization failures if the scan is never actually executed. I don't know if that was Kevin's original reason for putting it there, but that's why I left it there. I honestly can't remember whether that was a factor. I went through the README files and source code comments and set breakpoints at the low level heap reads in gdb and captured stack traces from as many execution plans as I knew how to generate, and went looking through those for likely places to put the predicate locking calls. I reasoned through the alternatives as best I could coming in cold and having been discouraged from asking questions. It would not shock me if those with greater familiarity with the code and a deeper understanding of how the pieces fit together can improve on my work there. I certainly won't take offense at any improvements made there; but I do have some concern over making changes this late in the release cycle unless they are clearly safe. Anssi Kääriäinen put in days of testing with real production data and software, and YAMAMOTO Takashi put in what appears to have been weeks of solid run time with I don't know what testing setup, but one which was really good at exposing race conditions. I get nervous about invalidating those efforts if they won't be repeated before release. By the way, I didn't miss the concern about the lossy bitmaps in bitgetpage() -- I'm trying to work my way through that now. What's a good way to generate a plan which uses lossy bitmaps? I'd like to try to generate a failing test. That's often very useful to me during coding, and tends to make a good addition to the test suite. -Kevin -- 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] Indication of db-shared tables
Excerpts from Bruce Momjian's message of mié jun 22 14:31:51 -0400 2011: Alvaro Herrera wrote: Excerpts from Jeff MacDonald's message of mi jun 22 09:27:36 -0400 2011: Greetings, On Wednesday, June 22, 2011 09:10:02 AM Bruce Momjian wrote: I assumed it was important to indicate if someone was looking at per-database or per-cluster data, like pg_tablespace. The issue comes up when I do admin training about the system tables. +1 I favor features that make training easier for the teacher. So what UI would you guys propose? I don't think widening the output of frequently used commands like \d or \d+ is okay for a feature with this small a use case. I am going to try to add descriptions for system tables for \d+ so maybe I can put it there. That would be OK with me, I guess -- something like shared system catalog for databases, you mean? -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- 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] Indication of db-shared tables
Alvaro Herrera wrote: Excerpts from Bruce Momjian's message of mi?? jun 22 14:31:51 -0400 2011: Alvaro Herrera wrote: Excerpts from Jeff MacDonald's message of mi jun 22 09:27:36 -0400 2011: Greetings, On Wednesday, June 22, 2011 09:10:02 AM Bruce Momjian wrote: I assumed it was important to indicate if someone was looking at per-database or per-cluster data, like pg_tablespace. The issue comes up when I do admin training about the system tables. +1 I favor features that make training easier for the teacher. So what UI would you guys propose? I don't think widening the output of frequently used commands like \d or \d+ is okay for a feature with this small a use case. I am going to try to add descriptions for system tables for \d+ so maybe I can put it there. That would be OK with me, I guess -- something like shared system catalog for databases, you mean? I was thinking (shared) as part of the description. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump vs malloc
Excerpts from Magnus Hagander's message of mié jun 22 11:25:43 -0400 2011: On Fri, Jun 10, 2011 at 21:07, Tom Lane t...@sss.pgh.pa.us wrote: Magnus Hagander mag...@hagander.net writes: I came across a situation today with a pretty bad crash of pg_dump, due to not checking the return code from malloc(). When looking through the code, it seems there are a *lot* of places in pg_dump that doesn't check the malloc return code. But we do have a pg_malloc() function in there - but from what I can tell it's only used very sparsely? Shouldn't we be using that one more or less everywhere Yup. Have at it. Something along the line of this? Huh, do you really need a new file for the four new functions? What's wrong with common.c? -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- 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] Indication of db-shared tables
Excerpts from Bruce Momjian's message of mié jun 22 15:24:40 -0400 2011: Alvaro Herrera wrote: Excerpts from Bruce Momjian's message of mi jun 22 14:31:51 -0400 2011: I am going to try to add descriptions for system tables for \d+ so maybe I can put it there. That would be OK with me, I guess -- something like shared system catalog for databases, you mean? I was thinking (shared) as part of the description. Well, that's what I was proposing above, sans the (unnecessary?) parens. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- 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] Indication of db-shared tables
On 06/22/2011 03:24 PM, Bruce Momjian wrote: I am going to try to add descriptions for system tables for \d+ so maybe I can put it there. That would be OK with me, I guess -- something like shared system catalog for databases, you mean? I was thinking (shared) as part of the description. I'm a bit unclear who this is going to help much. If you know what a shared table is you're pretty likely to know which tables are shared - there aren't that many of them after all. If you don't, this description is at least as likely to confuse you as help you in any way. I don't have strong feelings, just a bit of a suspicion that this isn't going to do much good. 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] Indication of db-shared tables
Alvaro Herrera wrote: Excerpts from Bruce Momjian's message of mi?? jun 22 15:24:40 -0400 2011: Alvaro Herrera wrote: Excerpts from Bruce Momjian's message of mi jun 22 14:31:51 -0400 2011: I am going to try to add descriptions for system tables for \d+ so maybe I can put it there. That would be OK with me, I guess -- something like shared system catalog for databases, you mean? I was thinking (shared) as part of the description. Well, that's what I was proposing above, sans the (unnecessary?) parens. OK. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] SYNONYMS (again)
Per: http://archives.postgresql.org/pgsql-hackers/2010-11/msg02043.php It seems we did come up with a use case in the procpid discussion. The ability to change the names of columns/databases etc, to handle the fixing of bad decision decisions during development over time. Thoughts? JD -- Command Prompt, Inc. - http://www.commandprompt.com/ PostgreSQL Support, Training, Professional Services and Development The PostgreSQL Conference - http://www.postgresqlconference.org/ @cmdpromptinc - @postgresconf - 509-416-6579 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] fixing PQsetvalue()
On Jun 6 (http://archives.postgresql.org/pgsql-hackers/2011-06/msg00272.php), Pavel discovered an issue with PQsetvalue that could cause libpq to wander off into unallocated memory that was present in 9.0.x. A fairly uninteresting fix was quickly produced, but Tom indicated during subsequent review that he was not happy with the behavior of the function. Everyone was busy with the beta wrap at the time so I didn't press the issue. A little history here: PQsetvalue's (http://www.postgresql.org/docs/9.0/static/libpq-misc.html) main reason to exist is to allow creating a PGresult out of scratch data on a result created via PQmakeEmptyResult(). This behavior works as intended and is not controversial...it was initially done to support libpqtypes but has apparently found use in other places like ecpg. PQsetvalue *also* allows you to mess with results returned by the server using standard query methods for any tuple, not just the one you are creating as you iterate through. This behavior was unnecessary in terms of what libpqtypes and friends needed and may (as Tom suggested) come back to bite us at some point. As it turns out, PQsetvalue's operation on results that weren't created via PQmakeEmptyResult was totally busted because of the bug, so we have a unique opportunity to tinker with libpq here: you could argue that you have a window of opportunity to change the behavior here since we know it isn't being usefully used. I think it's too late for that but it's if it had to be done the time is now. Pavel actually has been requesting to go further with being able to mess around with PGresults (see: http://archives.postgresql.org/pgsql-interfaces/2011-06/msg0.php), such that the result would start to have some 'recordset' features you find in various other libraries. Maybe it's not the job of libpq to do that, but I happen to think it's pretty cool. Anyways, something has to be done -- I hate to see an unpatched known 9.0 issue remain in the wild. merlin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Repeated PredicateLockRelation calls during seqscan
On Wed, Jun 22, 2011 at 12:07:04PM +0300, Heikki Linnakangas wrote: Hmm, I wonder if we should move this logic to heapam.c. The optimization to acquire a relation lock straight away should apply to all heap scans, not only those coming from ExecSeqScan. The distinction is academic at the moment, because that's the only caller that uses a regular MVCC snapshot, but it seems like a modularity violation for nodeSeqscan.c to reach into the HeapScanDesc to set the flag and grab the whole-relation lock, while heapam.c contains the PredicateLockTuple and CheckForSerializableConflictOut() calls. On modularity grounds, I think that's a good idea. The other PredicateLock* calls live in the access methods: heapam, nbtree, and indexam for the generic index support. heap_beginscan_internal seems like a reasonable place, as long as we're OK with taking the lock even if the scan is initialized but never called. Note that this hadn't been a reasonable option until last week when we added the check for non-MVCC snapshots, since there are lots of things that use heap scans but SeqScan is the only (currently-existing) one we want to lock. I am rather uneasy about making changes here unless we can be absolutely certain they're right... Dan -- Dan R. K. Ports MIT CSAILhttp://drkp.net/ -- 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] lazy vxid locks, v1
On Jun12, 2011, at 23:39 , Robert Haas wrote: So, the majority (60%) of the excess spinning appears to be due to SInvalReadLock. A good chunk are due to ProcArrayLock (25%). Hm, sizeof(LWLock) is 24 on X86-64, making sizeof(LWLockPadded) 32. However, cache lines are 64 bytes large on recent Intel CPUs AFAIK, so I guess that two adjacent LWLocks currently share one cache line. Currently, the ProcArrayLock has index 4 while SInvalReadLock has index 5, so if I'm not mistaken exactly the two locks where you saw the largest contention on are on the same cache line... Might make sense to try and see if these numbers change if you either make LWLockPadded 64bytes or arrange the locks differently... best regards, Florian Pflug -- 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] Repeated PredicateLockRelation calls during seqscan
Dan Ports d...@csail.mit.edu wrote: Note that this hadn't been a reasonable option until last week when we added the check for non-MVCC snapshots, since there are lots of things that use heap scans but SeqScan is the only (currently-existing) one we want to lock. That is the sort of thing that I tended to notice going through the backtraces from heap access I mentioned in another post, and is most likely the reason the call landed where it did. The MVCC snapshot tests are then a game-changer. It would be nice to find a way not to acquire the relation lock if the node is never used, though. I am rather uneasy about making changes here unless we can be absolutely certain they're right... Yeah -Kevin -- 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] SYNONYMS (again)
Excerpts from Joshua D. Drake's message of mié jun 22 15:37:17 -0400 2011: Per: http://archives.postgresql.org/pgsql-hackers/2010-11/msg02043.php It seems we did come up with a use case in the procpid discussion. The ability to change the names of columns/databases etc, to handle the fixing of bad decision decisions during development over time. Thoughts? Let's start with what was discussed and supported in that thread, that is, databases. It seems less clear that columns are widely believed to be a good idea to have synonyms for. Besides, synonyms for databases should be reasonably simple to implement, which is not something I would say for columns. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- 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] Coding style point: const in function parameter declarations
On Wed, Jun 22, 2011 at 5:41 PM, Tom Lane t...@sss.pgh.pa.us wrote: If we were thinking of moving in that direction, I would argue that we should get rid of typedef'd pointers altogether, ie, change Relation to be a typedef for the struct and write Relation *rel not Relation rel. Hm. I have to say the single most confusing thing about the Postgres source that took me a *long* time to get over was remembering that some of the typedefs were already pointers and some weren't. It seems silly now but when I was trying to understand what the intent of a function was and it wasn't obvious that some of the arguments appeared to be pass by value but were actually pass by reference it made things really surprising. -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Coding style point: const in function parameter declarations
Greg Stark st...@mit.edu writes: On Wed, Jun 22, 2011 at 5:41 PM, Tom Lane t...@sss.pgh.pa.us wrote: If we were thinking of moving in that direction, I would argue that we should get rid of typedef'd pointers altogether, ie, change Relation to be a typedef for the struct and write Relation *rel not Relation rel. Hm. I have to say the single most confusing thing about the Postgres source that took me a *long* time to get over was remembering that some of the typedefs were already pointers and some weren't. Yeah, the lack of consistency about that is annoying. 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] Coding style point: const in function parameter declarations
On 23 June 2011 00:37, Greg Stark st...@mit.edu wrote: Hm. I have to say the single most confusing thing about the Postgres source that took me a *long* time to get over was remembering that some of the typedefs were already pointers and some weren't. It seems silly now but when I was trying to understand what the intent of a function was and it wasn't obvious that some of the arguments appeared to be pass by value but were actually pass by reference it made things really surprising. I agree that the inconsistency is annoying. If we were starting from scratch, I'd encourage abandoning the convention. It's a leaky abstraction. At the risk of saying something that will raise eyebrows, I'll point out that the win32 API follows a similar practice. The difference is that they have consistent conventions in how they name their typdefs, so that you get to learn the patterns. -- 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] crash-safe visibility map, take five
On Thu, 2011-06-16 at 23:17 -0400, Noah Misch wrote: 2. In the words of a comment added by the patch: * The critical integrity requirement here is that we must never end up with * a situation where the visibility map bit is set, and the page-level * PD_ALL_VISIBLE bit is clear. If that were to occur, then a subsequent * page modification would fail to clear the visibility map bit. It does this by WAL-logging the operation of setting a vm bit. This also has the benefit of getting vm bits set correctly on standbys. In the same function, there is also the comment: We don't bump the LSN of the heap page when setting the visibility map bit, because that would generate an unworkable volume of full-page writes. This exposes us to torn page hazards, but since we're not inspecting the existing page contents in any way, we don't care. It would be nice to have a comment explaining why that is safe with respect to the WAL-before-data rule. Obviously torn pages aren't much of a problem, because it's a single bit and completely idempotent. Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] crash-safe visibility map, take five
On Wed, Jun 22, 2011 at 8:53 PM, Jeff Davis pg...@j-davis.com wrote: On Thu, 2011-06-16 at 23:17 -0400, Noah Misch wrote: 2. In the words of a comment added by the patch: * The critical integrity requirement here is that we must never end up with * a situation where the visibility map bit is set, and the page-level * PD_ALL_VISIBLE bit is clear. If that were to occur, then a subsequent * page modification would fail to clear the visibility map bit. It does this by WAL-logging the operation of setting a vm bit. This also has the benefit of getting vm bits set correctly on standbys. In the same function, there is also the comment: We don't bump the LSN of the heap page when setting the visibility map bit, because that would generate an unworkable volume of full-page writes. This exposes us to torn page hazards, but since we're not inspecting the existing page contents in any way, we don't care. It would be nice to have a comment explaining why that is safe with respect to the WAL-before-data rule. Obviously torn pages aren't much of a problem, because it's a single bit and completely idempotent. That's exactly what I was trying to explain in the comment you cite. Feel free to propose a specific change... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_upgrade version check improvements and small fixes
Dan McGee wrote:
Not sure what the normal process is for patches, but I put together a
few small patches for pg_upgrade after trying to use it earlier today
and staring a non-helpful error message before I finally figured out
what was going on.
Thanks for the detailed report and patches. Let me address each one
individually.
0001 is just a simple typo fix, but didn't want to mix it in with the rest.
I applied this message capitalization fix to 9.0, 9.1, and master (9.2).
0002 moves a function around to be declared in the only place it is
needed, and prevents a sh: /oasdfpt/pgsql-8.4/bin/pg_config: No such
file or directory error message when you give it a bogus bindir.
You are right that I was calling pg_ctl before I had validated the bin
directory, and you were right that the function wasn't in an ideal
C file.
What I did was to move the function, and bundle all the
pg_upgrade_support test into that single function, so the function now
either errors out or returns void.
Patch attached and applied to 9.1 and master. Good catch.
0003 is what I really wanted to solve, which was my failure with
pg_upgrade. The call to pg_ctl didn't succeed because the binaries
didn't match the data directory, thus resulting in this:
$ pg_upgrade --check -d /tmp/olddata -D /tmp/newdata -b /usr/bin/ -B /usr/bin/
Performing Consistency Checks
-
Checking old data directory (/tmp/olddata) ok
Checking old bin directory (/usr/bin) ok
Checking new data directory (/tmp/newdata) ok
Checking new bin directory (/usr/bin) ok
pg_resetxlog: pg_control exists but is broken or unknown version; ignoring it
Trying to start old server
.ok
Unable to start old postmaster with the command: /usr/bin/pg_ctl -l
/dev/null -D /tmp/olddata -o -p 5432 -c autovacuum=off -c
autovacuum_freeze_max_age=20 start /dev/null 21
Perhaps pg_hba.conf was not set to trust.
The error had nothing to do with trust at all; it was simply that I
tried to use 9.0 binaries with an 8.4 data directory. My patch checks
for this and ensures that the -D bindir is the correct version, just
as the -B datadir has to be the correct version.
I had not thought about testing if the bin and data directory were the
same major version, but you are right that it generates an odd error if
they are not.
I changed your sscanf format string because the version can be just
9.2dev and there is no need for the minor version. I saw no reason
to test if the binary version matches the pg_upgrade version because we
already test the cluster version, and we test the cluster version is the
same as the binary version.
Patch attached and applied to 9.1 and master.
I'm not on the mailing list nor do I have a lot of free time to keep
up with normal development, but if there are quick things I can do to
get these patches in let me know.
All done!
--
Bruce Momjian br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
new file mode 100644
index 2b481da..377dea2
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
*** static void check_is_super_user(ClusterI
*** 19,24
--- 19,25
static void check_for_prepared_transactions(ClusterInfo *cluster);
static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
static void check_for_reg_data_type_usage(ClusterInfo *cluster);
+ static void check_for_support_lib(ClusterInfo *cluster);
void
*** check_cluster_versions(void)
*** 245,265
void
check_cluster_compatibility(bool live_check)
{
! char libfile[MAXPGPATH];
! FILE *lib_test;
!
! /*
! * Test pg_upgrade_support.so is in the proper place. We cannot copy it
! * ourselves because install directories are typically root-owned.
! */
! snprintf(libfile, sizeof(libfile), %s/pg_upgrade_support%s, new_cluster.libpath,
! DLSUFFIX);
!
! if ((lib_test = fopen(libfile, r)) == NULL)
! pg_log(PG_FATAL,
! pg_upgrade_support%s must be created and installed in %s\n, DLSUFFIX, libfile);
! else
! fclose(lib_test);
/* get/check pg_control data of servers */
get_control_data(old_cluster, live_check);
--- 246,252
void
check_cluster_compatibility(bool live_check)
{
! check_for_support_lib(new_cluster);
/* get/check pg_control data of servers */
get_control_data(old_cluster, live_check);
*** check_for_reg_data_type_usage(ClusterInf
*** 730,732
--- 717,758
else
check_ok();
}
+
+
+ /*
+ * Test pg_upgrade_support.so is in the proper place. We cannot copy it
+ * ourselves because install directories are typically root-owned.
+ */
+ static void
+
Re: [HACKERS] [COMMITTERS] pgsql: Make the visibility map crash-safe.
On Wed, Jun 22, 2011 at 9:12 AM, Robert Haas robertmh...@gmail.com wrote: On Wed, Jun 22, 2011 at 6:55 AM, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: On 22.06.2011 06:05, Robert Haas wrote: Second, when inserting, updating, or deleting a tuple, we can no longer get away with clearing the visibility map bit after releasing the lock on the corresponding heap page, because an intervening crash might leave the visibility map bit set and the page-level bit clear. heap_update seems to still do that. Aw, crap. I'll take another look. Well, it seems I didn't put nearly enough thought into heap_update(). The fix for the immediate problem looks simple enough - all the code has been refactored to use the new API, so the calls can be easily be moved into the critical section (see attached). But looking at this a little more, I see that heap_update() is many bricks short of a load, because there are several places where the buffer can be unlocked and relocked, and we don't recheck whether the page is all-visible after reacquiring the lock. So I've got some more work to do here. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company heap-update-vm-fix.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] crash-safe visibility map, take five
On Wed, 2011-06-22 at 21:53 -0400, Robert Haas wrote: On Wed, Jun 22, 2011 at 8:53 PM, Jeff Davis pg...@j-davis.com wrote: On Thu, 2011-06-16 at 23:17 -0400, Noah Misch wrote: 2. In the words of a comment added by the patch: * The critical integrity requirement here is that we must never end up with * a situation where the visibility map bit is set, and the page-level * PD_ALL_VISIBLE bit is clear. If that were to occur, then a subsequent * page modification would fail to clear the visibility map bit. It does this by WAL-logging the operation of setting a vm bit. This also has the benefit of getting vm bits set correctly on standbys. In the same function, there is also the comment: We don't bump the LSN of the heap page when setting the visibility map bit, because that would generate an unworkable volume of full-page writes. This exposes us to torn page hazards, but since we're not inspecting the existing page contents in any way, we don't care. It would be nice to have a comment explaining why that is safe with respect to the WAL-before-data rule. Obviously torn pages aren't much of a problem, because it's a single bit and completely idempotent. That's exactly what I was trying to explain in the comment you cite. Feel free to propose a specific change... Well, I was a little unsure, but here's my attempt: The potential problems are: 1. Torn pages -- not a problem because it's a single bit and idempotent. 2. PD_ALL_VISIBLE bit makes it to disk before a WAL record representing an action that makes the page all-visible. Depending on what action makes a page all-visible: a. An old snapshot is released -- not a problem, because if there is a crash all snapshots are released. b. Cleanup action on the page -- not a problem, because that will create a WAL record and update the page's LSN before setting the PD_ALL_VISIBLE. First of all, is that correct? Second, are there other cases to consider? Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
