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] 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] 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 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
[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" 2>&1
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
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
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] pika buildfarm member failure on isolationCheck tests
On Wed, Jun 22, 2011 at 01:31:11AM -0400, Dan Ports wrote: > Yes, I suspect it can be done better. The reason it's tricky is a lock > ordering issue; part of releasing a SerializableXact has to be done > while holding SerializableXactHashLock and part has to be done without > it (because it involves taking partition locks). Reworking the order of > these things might work, but would require some careful thought since > most of the code is shared with the non-abort cleanup paths. And yes, > it's definitely the time for that. ...by which I mean it's definitely *not* the time for that, of course. 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] pika buildfarm member failure on isolationCheck tests
On Tue, Jun 21, 2011 at 03:01:48PM +0300, Heikki Linnakangas wrote: > Thanks, committed. Thanks. > In the long term, I'm not sure this is the best way to handle this. It > feels a bit silly to set the flag, release the SerializableXactHashLock, > and reacquire it later to remove the transaction from the hash table. > Surely that could be done in some more straightforward way. But I don't > feel like fiddling with it this late in the release cycle. Yes, I suspect it can be done better. The reason it's tricky is a lock ordering issue; part of releasing a SerializableXact has to be done while holding SerializableXactHashLock and part has to be done without it (because it involves taking partition locks). Reworking the order of these things might work, but would require some careful thought since most of the code is shared with the non-abort cleanup paths. And yes, it's definitely the time for that. I've been meaning to take another look at this part of the code anyway, with an eye towards performance. SerializableXactHashLock can be a bottleneck on certain in-memory workloads. > > One of the prepared_xacts regression tests actually hits this bug. > > I removed the anomaly from the duplicate-gids test so that it fails in > > the intended way, and added a new test to check serialization failures > > with a prepared transaction. > > Hmm, I have ran "make installcheck" with > default_transaction_isolation='serializable' earlier. I wonder why I > didn't see that. The affected test was being run at SERIALIZABLE already, so that wouldn't have made a difference. One reason I didn't notice an issue when I looked at that test before before, is that it was intended to fail anyway, just with a different error. I didn't think too hard about which error would take precedence. 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
[HACKERS] Repeated PredicateLockRelation calls during seqscan
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.
Dan
--
Dan R. K. Ports MIT CSAILhttp://drkp.net/
diff --git a/src/backend/executor/nodeSeqscan.c b/src/backend/executor/nodeSeqscan.c
index f356874..32a8f56 100644
--- a/src/backend/executor/nodeSeqscan.c
+++ b/src/backend/executor/nodeSeqscan.c
@@ -113,9 +113,13 @@ SeqRecheck(SeqScanState *node, TupleTableSlot *slot)
TupleTableSlot *
ExecSeqScan(SeqScanState *node)
{
- PredicateLockRelation(node->ss_currentRelation,
- node->ss_currentScanDesc->rs_snapshot);
- node->ss_currentScanDesc->rs_relpredicatelocked = true;
+ if (!node->ss_currentScanDesc->rs_relpredicatelocked)
+ {
+ PredicateLockRelation(node->ss_currentRelation,
+ node->ss_currentScanDesc->rs_snapshot);
+ node->ss_currentScanDesc->rs_relpredicatelocked = true;
+ }
+
return ExecScan((ScanState *) node,
(ExecScanAccessMtd) SeqNext,
(ExecScanRecheckMtd) SeqRecheck);
--
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] Fwd: Keywords in pg_hba.conf should be field-specific
On 22 June 2011 14:01, Pavel Stehule wrote: > ook, now is clean, so this is majority opinion. > > Please, can you send a final patch. I don't have any further changes to add to Alvaro's version 3, which is already up on the CF app. Cheers, BJ -- 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] Fwd: Keywords in pg_hba.conf should be field-specific
2011/6/22 Brendan Jurd : > On 22 June 2011 00:47, Tom Lane wrote: >> Alvaro Herrera writes: >>> Excerpts from Pavel Stehule's message of mar jun 21 00:59:44 -0400 2011: because >>pamservice<< - is known keyword, but 'pamservice' is some literal without any mean. You should to use a makro token_is_keyword more often. >> >>> Yeah, I wondered about this too (same with auth types, i.e. do we accept >>> quoted "hostssl" and so on or should that by rejected?). I opted for >>> leaving it alone, but maybe this needs to be fixed. (Now that I think >>> about it, what we should do first is verify whether it works with quotes >>> in the unpatched code). >> >> AFAICS, this is only important in places where the syntax allows either >> a keyword or an identifier. If only a keyword is possible, there is no >> value in rejecting it because it's quoted. And, when you do the test, >> I think you'll find that it would be breaking hba files that used to >> work (though admittedly, it's doubtful that there are any such in the >> field). > > Yes, I agree, and this was my thinking when I came up against this > while writing the original patch. It doesn't help to treat "hostssl" > differently than hostssl, because quoted identifiers are meaningless > in that context. ook, now is clean, so this is majority opinion. Please, can you send a final patch. Regards Pavel > > Cheers, > BJ > -- 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/21/2011 11:15 PM, Tom Lane wrote: Andrew Dunstan writes: Attached is a WIP possible replacement for pgindent. Instead of a shell script invoking a mishmash of awk and sed, some of which is pretty impenetrable, it uses a single engine (perl) to do all the pre and post indent processing. Hm ... this should offer the chance of running on Windows, but otherwise I'm not sure it does very much for us, if you still have to have a patched version of NetBSD indent underneath. Well, to start with it provides a nicer way to invoke pgindent for the whole repo. And it will be relatively easy to incorporate new stuff, for example what Greg Smith was doing in his wrapper script. It's true that the script contains an unfortunately large number of workarounds for the limitations and failure modes of the patched bsd indent we've been using. Some of those are currently working less than perfectly, and I found it fairly difficult to understand exactly what they were doing. I hope what I have produced will be a bit clearer and more maintainable than what it replaces, as well as working a bit better and more robustly. If anybody is so inclined, this could be used as a basis for removing the use of bsd indent altogether, as has been suggested before, as well as external entab/detab. Now *that* I would get excited about. But surely it would be a huge amount more work? Yes, which is why I decided to do this - I certainly don't currently have time to re-implement indent. 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] Latch implementation that wakes on postmaster death on both win32 and Unix
On Tue, Jun 21, 2011 at 6:22 PM, Peter Geoghegan wrote:
> Thanks for giving this your attention Fujii. Attached patch addresses
> your concerns.
Thanks for updating the patch! I have a few comments;
+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?
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?
+ 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?
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.
+ 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.
+errmsg( "pipe() call failed to create pipe to monitor
postmaster
death: %s", strerror(errno;
+errmsg("failed to set the postmaster death watching
fd's flags:
%s", strerror(errno;
+errmsg("failed to set the postmaster death watching
fd's flags:
%s", strerror(errno;
'%m' should be used instead of '%s' and 'strerror(errno)'.
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
--
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 using appname to lock out other users
Tom Lane wrote: > Bruce Momjian writes: > > Discussion seems to have ended on this thread without a clear direction. > > I still think the right thing is to just use a non-default port number. > That gets 90% of the benefit for 10% of the work of any other approach > (except for the ones for which the ratio is even worse). I agree. -- Bruce Momjian http://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
-BEGIN PGP SIGNED MESSAGE- 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. - -- Greg Sabino Mullane g...@turnstep.com End Point Corporation http://www.endpoint.com/ PGP Key: 0x14964AC8 201106212323 http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8 -BEGIN PGP SIGNATURE- iEYEAREDAAYFAk4BYF8ACgkQvJuQZxSWSsjOYACgnDq27MbRCg4Dr7QL/p6tq1kj 3EwAoPnJCqazL+akS1Au5WoxB5RvceDu =RpBk -END PGP SIGNATURE- -- 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 writes: > Attached is a WIP possible replacement for pgindent. Instead of a shell > script invoking a mishmash of awk and sed, some of which is pretty > impenetrable, it uses a single engine (perl) to do all the pre and post > indent processing. Hm ... this should offer the chance of running on Windows, but otherwise I'm not sure it does very much for us, if you still have to have a patched version of NetBSD indent underneath. > If anybody is so inclined, this could be used as a basis for removing > the use of bsd indent altogether, as has been suggested before, as well > as external entab/detab. Now *that* I would get excited about. But surely it would be a huge amount more work? 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
On Tue, Jun 21, 2011 at 11:04 PM, Tom Lane wrote: > Robert Haas 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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company there-is-no-try-v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_upgrade using appname to lock out other users
Bruce Momjian writes: > Discussion seems to have ended on this thread without a clear direction. I still think the right thing is to just use a non-default port number. That gets 90% of the benefit for 10% of the work of any other approach (except for the ones for which the ratio is even worse). 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] Indication of db-shared tables
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? -- Bruce Momjian http://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 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. 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] smallserial / serial2
On Thu, Jun 9, 2011 at 10:27 PM, Josh Kupershmidt wrote: > On Wed, Jun 8, 2011 at 6:36 PM, Brar Piening wrote: >> I tried to look at everything and cover everthing but please consider that >> this is my first review so please have a second look at it! > > I took a look at this as well, and I didn't encounter any problems > either. The patch is surprisingly small, but it looks like all the > documentation spots got covered, and I didn't see any missing pieces; > pg_dump copes fine, I didn't try pg_upgrade but I wouldn't expect > problems there either. > > Actually, the one piece I think could be added is a regression test. I > didn't see any existing tests that covered bigserial/serial8, either, > so I went ahead and added a few tests for smallerial and bigserial. I > didn't see the testcases Brar posted at first, or I might have adopted > a few from there, but this should cover basic use of smallserial. Committed the main patch, and your regression tests. -- 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] Fwd: Keywords in pg_hba.conf should be field-specific
On 22 June 2011 00:47, Tom Lane wrote: > Alvaro Herrera writes: >> Excerpts from Pavel Stehule's message of mar jun 21 00:59:44 -0400 2011: >>> because >>pamservice<< - is known keyword, but 'pamservice' is some >>> literal without any mean. You should to use a makro token_is_keyword >>> more often. > >> Yeah, I wondered about this too (same with auth types, i.e. do we accept >> quoted "hostssl" and so on or should that by rejected?). I opted for >> leaving it alone, but maybe this needs to be fixed. (Now that I think >> about it, what we should do first is verify whether it works with quotes >> in the unpatched code). > > AFAICS, this is only important in places where the syntax allows either > a keyword or an identifier. If only a keyword is possible, there is no > value in rejecting it because it's quoted. And, when you do the test, > I think you'll find that it would be breaking hba files that used to > work (though admittedly, it's doubtful that there are any such in the > field). Yes, I agree, and this was my thinking when I came up against this while writing the original patch. It doesn't help to treat "hostssl" differently than hostssl, because quoted identifiers are meaningless in that context. Cheers, BJ -- 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] deadlock_timeout at < PGC_SIGHUP?
2011/6/17 Shigeru Hanada : > (2011/06/12 6:43), Noah Misch wrote: >> On Wed, Mar 30, 2011 at 04:48:26PM -0400, Robert Haas wrote: >>> Me neither. If making the deadlock timeout PGC_SUSET is independently >>> useful, I don't object to doing that first, and then we can wait and >>> see if anyone feels motivated to do more. >> >> Here's the patch for that. Not much to it. > > I've reviewed the patch following the article in the PostgreSQL wiki. > It seems fine except that it needs to be rebased, so I'll mark this > "Ready for committers'. OK, committed. -- 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 using appname to lock out other users
Bruce Momjian wrote: > Bruce Momjian wrote: > > I meant the PGPASSWORD environment variable: > > > > > >PGPASSWORD > > > > PGPASSWORD behaves the same as the > linkend="libpq-connect-password"> connection parameter. > > Use of this environment variable > > is not recommended for security reasons, as some operating systems > > allow non-root users to see process environment variables via > > ps; instead consider using the > > ~/.pgpass file (see ). > > > > The only other way to do this is to pass it on the command line, but > > some options don't allow that (pg_ctl), and PGPASSFILE is going to > > require me to create a dummy .pgpass password file in a valid format and > > use that. > > One interesting idea would be to write the server password in the > PGPASSFILE format, and allow the server and libpq to read the same file > by pointing PGPASSFILE at that file. Discussion seems to have ended on this thread without a clear direction. Let me throw out some new ideas. The advantage of writing a password file is that we can avoid the need to modify pg_hba.conf to allow access without supplying a password. The downside is that it will only apply to PG 9.2+ servers, making configuration even more complex because the new and old servers would behave differently. One simple improvement would be to set listen_addresses to '' on Unix and 'localhost' on Windows to limit who can connect. This works on old and new servers. PG 9.1 already allows only super-user connections in -b binary-upgrade mode. No one seemed to like the appname filter but it seemed like a cheap solution. Remember we are not trying to secure the server while in pg_upgrade mode --- only avoid accidental connections. And, again, using a default port number of 25432 will work for old/new servers. -- Bruce Momjian http://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] Identifying no-op length coercions
On Tue, Jun 21, 2011 at 5:50 PM, Alexey Klyukin wrote: > On Jun 21, 2011, at 9:58 PM, Noah Misch wrote: >> A pg_regress test needs stable output, so we would do it roughly like this: >> >> CREATE TEMP TABLE relstorage AS SELECT 0::regclass AS oldnode; >> ... >> UPDATE relstorage SET oldnode = >> (SELECT relfilenode FROM pg_class WHERE oid = >> 'test'::regclass); >> ALTER TABLE test ALTER name TYPE varchar(65535); >> SELECT oldnode <> relfilenode AS rewritten >> FROM pg_class, relstorage WHERE oid = 'test'::regclass; >> >> I originally rejected that as too ugly to read. Perhaps not. > > Yes, your example is more appropriate. I think you can make it more > straightforward by getting rid of the temp table: > > CREATE TABLE test(oldnode oid, name varchar(5)); > > INSERT INTO test(oldnode) SELECT relfilenode FROM pg_class WHERE > oid='test'::regclass; > > ALTER TABLE test ALTER name TYPE varchar(10); > > SELECT oldnode <> relfilenode AS rewritten FROM pg_class, test WHERE > oid='test'::regclass; Without wishing to foreclose the possibility of adding a suitable regression test, I've committed the main patch. -- 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
Andrew Dunstan wrote: > > Attached is a WIP possible replacement for pgindent. Instead of a shell > script invoking a mishmash of awk and sed, some of which is pretty > impenetrable, it uses a single engine (perl) to do all the pre and post > indent processing. Of course, if your regex-fu and perl-fu is not up the > scratch this too might be impenetrable, but all but a couple of the > recipes are reduced to single lines, and I'd argue that they are all at > least as comprehensible as what they replace. I am excited Andrew has done this. It has been on my TODO list for a while --- I was hoping someday we could switch to GNU indent but gave up after the GNU indent report from Greg Stark that exactly matched my experience years ago: http://archives.postgresql.org/pgsql-hackers/2011-04/msg01436.php Basically, GNU indent has new bugs, but bugs that are harder to work around than the BSD indent bugs. Once I heard that I realized we were going to be using BSD indent for many more years to come and rewriting the shell script in Perl was the next logical step, which Andrew has done. This will also allow us to use pgindent on Windows that has Perl installed --- we should create a DOS binary of bsd indent so Windows users scan run it. I would also like to add a command-line way to detect our patched version of BSD indent so we know we are using the right binary. I will do that once the Perl version is committed to git. Thanks Andrew. -- Bruce Momjian http://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] Auto Start second postgres 8.3.15-1 instance MAC OS X
On Mon, Jun 20, 2011 at 6:14 AM, Diogo Santos wrote: > Hi, I'm used to work with PostgreSQL on Windows but now I've moved to OS X > and I'm having problems to create a service to auto start a new server > (instance) of PostgreSQL. > Firstly I used the PostgreSQL installer to create the first server and the > postgres user, then I used initdb to create another server and until this > step, everything turned fine. > After that, I created a folder on StartupItems with StartupParameters and > the other script. > Here are the contents of the two files: I have no idea how to troubleshoot this, but you might want to take a loop at the contents of contrib/start-scripts/osx. -- 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] date_part for infinity intervals
On Mon, Jun 20, 2011 at 5:54 AM, Vlad Arkhipov wrote:
> The behaviour of date_part function is opaque for infinity intervals. For
> example
> date_part('epoch', 'infinity'::date) and date_part('year', 'infinity'::date)
> return zero but is supposed to return 'infinity',
> date_part('day', 'infinity'::date) returns zero, should it return 'NaN'
> instead?
Dunno. It's been this way since 2001; before that, it returned NULL.
I don't see any particular justification for making the return value
different in the infinity case depending on whether "epoch" or "day"
is requested. Returning "Infinity" rather than 0 might have some
merit, but I'm not sure it's worth breaking backward compatibility for
it. What do our competitors do in this case?
--
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] SSI tuning points
On Sun, Jun 19, 2011 at 11:10 AM, Kevin Grittner wrote: > That does seem better. Thanks. OK, committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP Reworks Part.0 - 'missing_ok' support of get_object_address
On Sun, Jun 19, 2011 at 7:40 AM, Kohei KaiGai wrote: > Sorry, the previous revision did not update regression test part > towards the latest one. 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? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company there-is-no-try.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
[HACKERS] WIP pgindent replacement
Attached is a WIP possible replacement for pgindent. Instead of a shell
script invoking a mishmash of awk and sed, some of which is pretty
impenetrable, it uses a single engine (perl) to do all the pre and post
indent processing. Of course, if your regex-fu and perl-fu is not up the
scratch this too might be impenetrable, but all but a couple of the
recipes are reduced to single lines, and I'd argue that they are all at
least as comprehensible as what they replace.
Attached also is a diff file showing what it does differently from the
existing script. I think that these are all things where the new script
is more correct than the existing script. Most of the changes come into
two categories:
* places where the existing script fails to combine the function
return type and the function name on a single line in function
prototypes.
* places where unwanted blank lines are removed by the new script
but not by the existing script.
Features include:
* command line compatibility with the existing script, so you can do:
find ../../.. -name '*.[ch]' -type f -print | egrep -v -f
exclude_file_patterns | xargs -n100 ./pgindent.pl typedefs.list
* a new way of doing the same thing much more nicely:
./pgindent.pl --search-base=../../.. --typedefs=typedefs.list
--excludes=exclude_file_patterns
* only passes relevant typedefs to indent, not the whole huge list
* should in principle be runnable on Windows, unlike existing script
(I haven't tested yet)
* no semantic tab literals; tabs are only generated using \t and
tested for using \t, \h or \s as appropriate. This makes debugging
the script much less frustrating. If something looks like a space
it should be a space.
In one case I used perl's extended regex mode to comment a fairly hairy
regex. This should probably be done a bit more, maybe for all of them.
If anybody is so inclined, this could be used as a basis for removing
the use of bsd indent altogether, as has been suggested before, as well
as external entab/detab.
Comments welcome.
cheers
andrew
pgindent.pl
Description: Perl program
diff --git a/contrib/btree_gist/btree_bit.c b/contrib/btree_gist/btree_bit.c
index 8675d24..f69faaa 100644
--- a/contrib/btree_gist/btree_bit.c
+++ b/contrib/btree_gist/btree_bit.c
@@ -95,7 +95,6 @@ gbt_bit_xfrm(bytea *leaf)
static GBT_VARKEY *
gbt_bit_l2n(GBT_VARKEY *leaf)
{
-
GBT_VARKEY *out = leaf;
GBT_VARKEY_R r = gbt_var_key_readable(leaf);
bytea *o;
diff --git a/contrib/btree_gist/btree_text.c b/contrib/btree_gist/btree_text.c
index 3d4f8c1..b55dc7c 100644
--- a/contrib/btree_gist/btree_text.c
+++ b/contrib/btree_gist/btree_text.c
@@ -119,7 +119,6 @@ gbt_text_compress(PG_FUNCTION_ARGS)
Datum
gbt_bpchar_compress(PG_FUNCTION_ARGS)
{
-
GISTENTRY *entry = (GISTENTRY *) PG_GETARG_POINTER(0);
GISTENTRY *retval;
diff --git a/contrib/btree_gist/btree_ts.c b/contrib/btree_gist/btree_ts.c
index 9d3a591..185bed0 100644
--- a/contrib/btree_gist/btree_ts.c
+++ b/contrib/btree_gist/btree_ts.c
@@ -380,7 +380,6 @@ gbt_ts_union(PG_FUNCTION_ARGS)
Datum
gbt_ts_penalty(PG_FUNCTION_ARGS)
{
-
tsKEY *origentry = (tsKEY *) DatumGetPointer(((GISTENTRY *) PG_GETARG_POINTER(0))->key);
tsKEY *newentry = (tsKEY *) DatumGetPointer(((GISTENTRY *) PG_GETARG_POINTER(1))->key);
float *result = (float *) PG_GETARG_POINTER(2);
diff --git a/contrib/btree_gist/btree_utils_num.c b/contrib/btree_gist/btree_utils_num.c
index a3da580..8b8cd31 100644
--- a/contrib/btree_gist/btree_utils_num.c
+++ b/contrib/btree_gist/btree_utils_num.c
@@ -134,7 +134,6 @@ gbt_num_union(GBT_NUMKEY *out, const GistEntryVector *entryvec, const gbtree_nin
bool
gbt_num_same(const GBT_NUMKEY *a, const GBT_NUMKEY *b, const gbtree_ninfo *tinfo)
{
-
GBT_NUMKEY_R b1,
b2;
@@ -156,7 +155,6 @@ gbt_num_same(const GBT_NUMKEY *a, const GBT_NUMKEY *b, const gbtree_ninfo *tinfo
void
gbt_num_bin_union(Datum *u, GBT_NUMKEY *e, const gbtree_ninfo *tinfo)
{
-
GBT_NUMKEY_R rd;
rd.lower = &e[0];
diff --git a/contrib/btree_gist/btree_utils_var.c b/contrib/btree_gist/btree_utils_var.c
index e73799b..dc3ad38 100644
--- a/contrib/btree_gist/btree_utils_var.c
+++ b/contrib/btree_gist/btree_utils_var.c
@@ -54,7 +54,6 @@ gbt_var_decompress(PG_FUNCTION_ARGS)
GBT_VARKEY_R
gbt_var_key_readable(const GBT_VARKEY *k)
{
-
GBT_VARKEY_R r;
r.lower = (bytea *) &(((char *) k)[VARHDRSZ]);
@@ -106,7 +105,6 @@ gbt_var_leaf2node(GBT_VARKEY *leaf, const gbtree_vinfo *tinfo)
static int32
gbt_var_node_cp_len(const GBT_VARKEY *node, const gbtree_vinfo *tinfo)
{
-
GBT_VARKEY_R r = gbt_var_key_readable(node);
int32 i = 0;
int32 l = 0;
@@ -270,7 +268,6 @@ gbt_var_bin_union(Datum *u, GBT_VARKEY *e, Oid collation,
GISTENTRY *
gbt_var_compress(GISTENTRY *entry, const gbtree_vinfo *tinfo)
{
-
GISTENTRY *retval;
if (entry->leafkey)
@@ -299,7 +296,6 @@ GBT_VARKEY *
gbt_var_union(const GistEntryVector *ent
Re: [HACKERS] Coding style point: "const" in function parameter declarations
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. 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] Coding style point: "const" in function parameter declarations
On 21 June 2011 23: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. People may be confused by that inconsistency though. I agree that it generally doesn't make sense to const-qualify a passed-by-value parameter. I've seen it done, but I struggle to recall a case where I thought that it made much 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. IMHO, you don't have to be that much of a language lawyer to pick up on that, because it's not as if the typedef has very effectively encapsulated the fact that the underlying type is actually a pointer anyway. I for one find it intuitively obvious that the pointed-to data isn't declared const in your example. > 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. +1 const is actually an example of C++ influencing C. It tends to work very well with C++, where it cascades usefully throughout an entire system, and where a distinction can be made between logical and bit-wise const-ness. In C, it is a bit wishy-washy, particularly the fact that what happens when const-ness is violated is undefined, rather than just having the code fail to compile. const is also supposed to be able to facilitate certain compiler optimisations, which is why you have the undefined behaviour thing (what if the variable is stored in read-only memory?), but that isn't very relevant in practice. -- 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] Re: patch review : Add ability to constrain backend temporary file space
On 21/06/11 02:39, Cédric Villemain wrote: 2011/6/20 Robert Haas: On Mon, Jun 20, 2011 at 9:15 AM, Cédric Villemain wrote: The feature does not work exactly as expected because the write limit is rounded per 8kB because we write before checking. I believe if one write a file of 1GB in one pass (instead of repetitive 8kB increment), and the temp_file_limit is 0, then the server will write the 1GB before aborting. Can we rearrange thing so we check first, and then write? probably but it needs more work to catch corner cases. We may be safe to just document that (and also in the code). The only way I see so far to have a larger value than 8kB here is to have a plugin doing the sort instead of the postgresql core sort algo. Thanks guys - will look at moving the check, and adding some documentation about the possible impacts of plugins (or new executor methods) that might write in chunks bigger than blocksz. Maybe a few days - I'm home sick ATM, plus looking after these http://www.maftet.co.nz/kittens.html Cheers Mark -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Coding style point: "const" in function parameter declarations
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. 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. 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? 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] Fwd: Keywords in pg_hba.conf should be field-specific
On Jun 21, 2011, at 12:41 PM, Alvaro Herrera wrote: > On the contrary -- we should support it but not document it. +1. ...Robert -- 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] Fwd: Keywords in pg_hba.conf should be field-specific
2011/6/21 Alvaro Herrera : > Excerpts from Pavel Stehule's message of mar jun 21 11:04:11 -0400 2011: >> 2011/6/21 Tom Lane : > >> > AFAICS, this is only important in places where the syntax allows either >> > a keyword or an identifier. If only a keyword is possible, there is no >> > value in rejecting it because it's quoted. And, when you do the test, >> > I think you'll find that it would be breaking hba files that used to >> > work (though admittedly, it's doubtful that there are any such in the >> > field). >> >> It should be better documented. I don't think so this is good >> solution, but this is not too important. > > On the contrary -- we should support it but not document it. I mean, > what good would that do? If someone is so silly to uselessly quote > keywords, let them do it, but let's not encourage it. it is argument too :) It has not good solution - one break compatibility, second is strange and undocumented :( Actually I don't remember a issues about pg_hba.conf - probably 99% users work with default configuration, so we can leave this file in current state. I am thinking so a notice in pg_hba.conf can be redesigned - almost all people don't read it, but if someone read it, then he needs a correct information - in sense, so on quotes works only where literal or known literal can be entered. Regards Pavel Stehule > > -- > Álvaro Herrera > 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] Identifying no-op length coercions
On Jun 21, 2011, at 9:58 PM, Noah Misch wrote: > > A pg_regress test needs stable output, so we would do it roughly like this: > > CREATE TEMP TABLE relstorage AS SELECT 0::regclass AS oldnode; > ... > UPDATE relstorage SET oldnode = > (SELECT relfilenode FROM pg_class WHERE oid = 'test'::regclass); > ALTER TABLE test ALTER name TYPE varchar(65535); > SELECT oldnode <> relfilenode AS rewritten > FROM pg_class, relstorage WHERE oid = 'test'::regclass; > > I originally rejected that as too ugly to read. Perhaps not. Yes, your example is more appropriate. I think you can make it more straightforward by getting rid of the temp table: CREATE TABLE test(oldnode oid, name varchar(5)); INSERT INTO test(oldnode) SELECT relfilenode FROM pg_class WHERE oid='test'::regclass; ALTER TABLE test ALTER name TYPE varchar(10); SELECT oldnode <> relfilenode AS rewritten FROM pg_class, test WHERE oid='test'::regclass; > >> The only nitpick code-wise is these lines in varchar_transform: >> >> +int32 old_max = exprTypmod(source) - VARHDRSZ; >> +int32 new_max = new_typmod - VARHDRSZ; >> >> I have a hard time understanding why VARHDRSZ is subtracted here, so I'd >> assume that's a bug. > > We track the varchar typmod internally as (max length) + VARHDRSZ. Oh, right, haven't thought that this is a varchar specific thing. Thank you, Alexey. -- Command Prompt, Inc. http://www.CommandPrompt.com 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] patch for 9.2: enhanced errors
2011/6/21 Steve Singer : > On 11-06-20 03:44 PM, Pavel Stehule wrote: > > Hello > > You need to update config.sgml at the same time you update this format. > You need to append a "," after application name but before constraintName. > As it stands the CSV log has something like: > .nbtinsert.c:433","psql""a_pkey","public","a","a" > > fixed > > > The CSV log seems fine now. > > > > nbtinsert.c > > pg_get_indrelation is named differently than everything else in this file > (ie _bt...). My guess is that this function belongs somewhere else but I > don't know the code well enough to say where you should move it too. > > I renamed this function to IndexRelationGetParentRelation and muved to > relcache.c > > > Thanks, it looks less out of place there than it did in nbtinsert.c > > I don't call a quote_identifier on only data error properties like > table_name or schema_name (but I am open to arguments for it or > against it). The quote_identifier is used for column names, because > there should be a more names and comma should be used inside name - > and this is consistent with pg_get_indexdef_columns. > > Regards > > > Okay. > > > > Pavel Stehule > > > I'm going to mark this as ready for a committer. > Thank you very much Regards Pavel Stehule > > -- 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] Online base backup from the hot-standby
On 11-06-14 02:52 AM, Jun Ishiduka wrote:
>> I still think that's headed in the wrong direction.
>> (http://archives.postgresql.org/pgsql-hackers/2011-05/msg01405.php)
> Please check these mails, and teach the reason for content of the wrong
> direction.
> (http://archives.postgresql.org/pgsql-hackers/2011-06/msg00209.php)
> (http://archives.postgresql.org/pgsql-hackers/2011-05/msg01566.php)
>
>
Jun, I've been reviewing these threads as a start to reviewing your
patch (I haven't yet looked at the patch).
I *think* the concern is that
1) Today you can do a backup by just calling pg_start_backup('x'); copy
the data directory and
pg_stop_backup(); You do not need to use pg_basebackup to create a
backup. The solution you are proposing would require pg_basebackup to be
used to build backups from standby servers.
2) If I run pg_basebackup but do not specify '-x' then no pg_xlog
segments are included in the output. The relevant pg_xlog segments are
in the archive from the master. I can see situations where you are
already copying the archive to the remote site that the new standby will
be created in so you don't want to have to copy the pg_xlog segments
twice over your network.
What Heikki is proposing will work both when you aren't using
pg_basebackup (as long the output of pg_stop_backup() is somehow
captured in a way that it can be read) and will also work with
pg_basebackup when '-x' isn't specified.
Steve
>
> Jun Ishizuka
> NTT Software Corporation
> TEL：045-317-7018
> E-Mail: ishizuka@po.ntts.co.jp
>
>
>
>
--
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: hstore - Implementation and performance issues around its operators
>> 1. Obviously the '@>' has to be used in order to let use the GiST index.
>> Why is the '->' operator not supported by GiST ('->' is actually
>> mentioned in all examples of the doc.)?
There's no way to make the planner recognize something like
(col->'foo' = 'bar')
as being an indexable condition on "col". (If 'foo' is constant then you
can of course create a functional (btree) index on (col->'foo').)
Whereas (col @> hstore('foo','bar')) can use a GiST or GIN index directly
as long as 'foo' and 'bar' are pseudoconstants (i.e. constants, columns of
other tables, or stable functions of those).
>> 2. Currently the hstore elements are stored in order as they are
>> coming from the insert statement / constructor.
No, the elements of an hstore are stored in order of (keylength,key)
with the key comparison done bytewise (not locale-dependent).
>> Why are the elements not ordered i.e. why is the hstore not cached in
>> all hstore functions (like hstore_fetchval etc.)?
I don't understand what you think would be cached.
>> 3. In the source code 'hstore_io.c' one finds the following enigmatic
>> note: "... very large hstore values can't be output. this could be
>> fixed, but many other data types probably have the same issue."
>> What is the max. length of a hstore (i.e. the max. length of the sum
>> of all elements in text representation)?
An hstore is internally limited to the 1GB limit that applies to all
varlenas and individual memory allocations. However, the output text
representation can be longer than the internal one, and the output is
_also_ limited to 1GB.
The most obvious example of another type with the same issue is bytea,
where a value >512MB will fail to output in hex mode, and a value >256MB
might fail to output in escape mode depending on the content.
>> 4. Last, I don't fully understand the following note in the hstore
>> doc. (http://www.postgresql.org/docs/current/interactive/hstore.html
>> ):
>> Notice that the old names are reversed from the convention
>> formerly followed by the core geometric data types!
>> Why names? Why not rather 'operators' or 'functions'?
It's referring to the name of the operator (e.g. "@" or "~").
>> What does this "reversed from the convention" mean concretely?
In old postgres versions the operators @ and ~ were used by various types
to mean "contains" and "contained by", but many contrib types had them
reversed in meaning from the core geometry types.
For example, if a and b are boxes, (a @ b) means "a is contained in b",
but if they are cubes from contrib/cube, or hstores, then (a @ b) means
"a contains b".
The inconsistency here was obviously intolerable and was resolved by
introducing <@ and @> as the recommended names for "contained in" and
"contains" operators respectively, with the < > giving a visual hint as
to which was the "larger" side.
--
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] Libpq enhancement
On Sun, Jun 19, 2011 at 8:08 PM, Robert Haas wrote: > On Sun, Jun 19, 2011 at 11:04 AM, Jeff Shanab wrote: >> I am wondering If I am missing something obvious. If not, I have a >> suggestion for plpgsql. >> >> Stored procedures can accept rows. >> >> Libpq can receive rows (PQResult). >> >> Wouldn’t it be a great interface if PQResult was “bi-directional”? Create a >> result set on the client then call the database with a command. > > For insert, we have something like this already - this is what "copy" is for. 'copy' is a *bulk* insert statement -- it's great for the very specific case when you are dumbly stuffing data into the database, especially if performance is critical and sane error handling is not. It is not suitable for anything else: feeding data into functions, update/upsert/delete, insert with join, pre-post process, etc. Also copy runs through libpq textually at the line level, not at the field level like the rest of libpq. > For update, it's a bit more complex - we don't have a "replace into" > operator... Actually, we do. 9.1 supports data modifying CTE around which it's possible to rig a perfectly reasonable upsert...barring that, you could trivially do something similar in a hand rolled backend upsert function that takes a row or a set of rows (fed in as a composite array). Point being, the server has the necessary features -- it's the client that's the (solved) problem. At the risk of sounding 'broken record repetitive', let me echo andrew's comment upthread that libpqtypes solves the OP's problem completely in a very elegant way. The basic M.O. is to: 1. register the type you are using for transport (can either be the table or a composite type) 2. for each record you want to send, PQputf that record, and if you are sending more than one, PQputf the record into it's array 3. PQparamExec() a query that might look like one of: /* straight up insert */ PQparamExec(conn, param, "INSERT INTO foo SELECT (unnest(%foo[])).* FROM f", resfmt); /* send to function */ PQparamExec(conn, param, "SELECT do_stuff(%foo[]) ", resfmt); /* upsert -- pre 9.1 this could be done in plpgsql loop, etc */ WITH foos AS (SELECT (UNNEST(%foo[])).*) updated as (UPDATE foo SET foo.a = foos.a ... RETURNING foo.id) INSERT INTO foo SELECT foos.* FROM foos LEFT JOIN updated USING(id) WHERE updated.id IS NULL; Basically, the trick is to exploit the server's composite array type features on the client side to do exactly what the OP is gunning for. You can send anything from simple arrays to entire complex nested structures that way -- although the complex stuff would typically go a to a function. Performance wise, it's faster than traditional query methods (everything is sent in binary) but slower than 'copy'. 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] patch for 9.2: enhanced errors
On 11-06-20 03:44 PM, Pavel Stehule wrote: Hello You need to update config.sgml at the same time you update this format. You need to append a "," after application name but before constraintName. As it stands the CSV log has something like: .nbtinsert.c:433","psql""a_pkey","public","a","a" fixed The CSV log seems fine now. nbtinsert.c pg_get_indrelation is named differently than everything else in this file (ie _bt...). My guess is that this function belongs somewhere else but I don't know the code well enough to say where you should move it too. I renamed this function to IndexRelationGetParentRelation and muved to relcache.c Thanks, it looks less out of place there than it did in nbtinsert.c I don't call a quote_identifier on only data error properties like table_name or schema_name (but I am open to arguments for it or against it). The quote_identifier is used for column names, because there should be a more names and comma should be used inside name - and this is consistent with pg_get_indexdef_columns. Regards Okay. Pavel Stehule I'm going to mark this as ready for a committer.
Re: [HACKERS] Identifying no-op length coercions
On Tue, Jun 21, 2011 at 06:31:44PM +0300, Alexey Klyukin wrote: > Here is my review of this patch. Thanks! > The patch applies cleanly to the HEAD, produces no additional warnings. It > doesn't include additional regression tests. One can include a test, using the > commands like the ones included below. > > Changes to the documentation are limited to the a description of a new > pg_class attribute. It would probably make sense to document all the > exceptions to the table's rewrite on ALTER TABLE documentation page, although > it could wait for more such exceptions. I like the current level of detail in the ALTER TABLE page. Having EXPLAIN ALTER TABLE would help here, but that's a bigger project than this one. > postgres=# alter table test alter column name type varchar(10); > ALTER TABLE > postgres=# select relfilenode from pg_class where oid='test'::regclass; > relfilenode > - >66308 > (1 row) > postgres=# alter table test alter column name type varchar(65535); > ALTER TABLE > postgres=# select relfilenode from pg_class where oid='test'::regclass; > relfilenode > - >66308 > (1 row) A pg_regress test needs stable output, so we would do it roughly like this: CREATE TEMP TABLE relstorage AS SELECT 0::regclass AS oldnode; ... UPDATE relstorage SET oldnode = (SELECT relfilenode FROM pg_class WHERE oid = 'test'::regclass); ALTER TABLE test ALTER name TYPE varchar(65535); SELECT oldnode <> relfilenode AS rewritten FROM pg_class, relstorage WHERE oid = 'test'::regclass; I originally rejected that as too ugly to read. Perhaps not. > The only nitpick code-wise is these lines in varchar_transform: > > + int32 old_max = exprTypmod(source) - VARHDRSZ; > + int32 new_max = new_typmod - VARHDRSZ; > > I have a hard time understanding why VARHDRSZ is subtracted here, so I'd > assume that's a bug. We track the varchar typmod internally as (max length) + VARHDRSZ. -- 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
Alvaro Herrera writes: > Excerpts from Tom Lane's message of mar jun 21 11:06:22 -0400 2011: >> Please note that this position should not be regarded as support for >> Simon's proposed patch. I still think the right decision is to revert >> the ALTER TABLE feature, mainly because I do not believe this is the >> last bug in it. And the fact that there's a pre-existing bug with a >> vaguely similar symptom is no justification for introducing more bugs. > Note that this feature can be disabled by tweaking > AlterTableGetLockLevel so that it always returns AccessExclusive. I think Simon had also hacked a couple of other places such as CREATE TRIGGER, but yeah, I was thinking of just lobotomizing that function with an #ifdef. When and if we get these problems worked out, it'll be easy to re-enable the feature. 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
Excerpts from Tom Lane's message of mar jun 21 11:06:22 -0400 2011: > Please note that this position should not be regarded as support for > Simon's proposed patch. I still think the right decision is to revert > the ALTER TABLE feature, mainly because I do not believe this is the > last bug in it. And the fact that there's a pre-existing bug with a > vaguely similar symptom is no justification for introducing more bugs. Note that this feature can be disabled by tweaking AlterTableGetLockLevel so that it always returns AccessExclusive. -- Álvaro Herrera 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] Table Partitioning
On Tue, Jun 21, 2011 at 01:07:17PM -0400, Robert Haas wrote: > On Mon, Jun 20, 2011 at 5:42 PM, David Fetter wrote: > > I noticed that we have some nice new speed optimizations (more > > properly, de-pessimizations) for partitioned tables in 9.1. > > /me sticks tongue out at dfetter. > > > Anybody care to look over the table partitioning stuff on the wiki and > > check it for relevance? > > > > http://wiki.postgresql.org/wiki/Table_partitioning > > Itagaki Takahiro had a patch for this about a year ago, but I wasn't > happy with the system catalog representation he chose and I think > there were some other issues as well. In particular, I'm noticing things labeled 8.4 and 9.0 as future. > Still, I think a pretty clear > way forward here is to try to figure out a way to add some explicit > syntax for range partitions, so that you can say... > > foo_a is for all rows where foo starts with 'a' > foo_b is for all rows where foo starts with 'b' > ... > foo_xyz is for all rows where foo starts with 'xyz' > > If we have that data represented explicitly in the system catalog, > then we can look at doing built-in INSERT-routing and UPDATE-handling. > For an added bonus, it's a more natural syntax. Does someone else have such a syntax? Does The Standard™ have anything to say? Cheers, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TABLE lock strength reduction patch is unsafe
Excerpts from Robert Haas's message of mar jun 21 09:40:16 -0400 2011: > On Mon, Jun 20, 2011 at 6:55 PM, Simon Riggs wrote: > > 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. All I > > would say is that issuing a stream of ALTER TABLEs against the same > > table is not a common situation; if it were we would have seen more of > > the pre-existing bug. ALTER TABLE command encompasses many subcommands > > and we should evaluate each subcommand differently when we decide what > > to do. > > Well, my principal objection is that I think heavyweight locking is an > excessively expensive solution to this problem. I think the patch is > simple enough that I wouldn't object to applying it on those grounds > even at this late date, but I bet if we do some benchmarking on the > right workload we'll find a significant performance regression. Yeah, taking a hw lock at each relcache item build is likely to be prohibitively expensive. Heck, relation extension is expensive already in some loads. (I'm guessing that things will tank when there's a relcache reset). Still, this seems to be an overall better approach to the problem than what's been proposed elsewhere. Maybe we can do something with a map of relations that are protected by a bunch of lwlocks instead. (We could use that for relation extension too; that'd rock.) The patch may be simple, but is it complete? I think you need to have lock acquisition on create rule and create index too. Right now it only has locks on trigger stuff and alter table. -- Álvaro Herrera 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] Table Partitioning
On Mon, Jun 20, 2011 at 5:42 PM, David Fetter wrote: > I noticed that we have some nice new speed optimizations (more > properly, de-pessimizations) for partitioned tables in 9.1. /me sticks tongue out at dfetter. > Anybody care to look over the table partitioning stuff on the wiki and > check it for relevance? > > http://wiki.postgresql.org/wiki/Table_partitioning Itagaki Takahiro had a patch for this about a year ago, but I wasn't happy with the system catalog representation he chose and I think there were some other issues as well. Still, I think a pretty clear way forward here is to try to figure out a way to add some explicit syntax for range partitions, so that you can say... foo_a is for all rows where foo starts with 'a' foo_b is for all rows where foo starts with 'b' ... foo_xyz is for all rows where foo starts with 'xyz' If we have that data represented explicitly in the system catalog, then we can look at doing built-in INSERT-routing and UPDATE-handling. For an added bonus, it's a more natural syntax. -- 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] Fwd: Keywords in pg_hba.conf should be field-specific
Excerpts from Pavel Stehule's message of mar jun 21 11:04:11 -0400 2011: > 2011/6/21 Tom Lane : > > AFAICS, this is only important in places where the syntax allows either > > a keyword or an identifier. If only a keyword is possible, there is no > > value in rejecting it because it's quoted. And, when you do the test, > > I think you'll find that it would be breaking hba files that used to > > work (though admittedly, it's doubtful that there are any such in the > > field). > > It should be better documented. I don't think so this is good > solution, but this is not too important. On the contrary -- we should support it but not document it. I mean, what good would that do? If someone is so silly to uselessly quote keywords, let them do it, but let's not encourage it. -- Álvaro Herrera 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] Identifying no-op length coercions
Hi,
On Jun 19, 2011, at 2:10 PM, Noah Misch wrote:
> On Sat, Jun 18, 2011 at 11:32:20PM -0400, Robert Haas wrote:
>> On Sat, Jun 18, 2011 at 11:12 PM, Robert Haas wrote:
>>> On Sat, Jun 18, 2011 at 11:06 PM, Noah Misch wrote:
On Sat, Jun 18, 2011 at 10:57:13PM -0400, Robert Haas wrote:
> On Sat, Jun 11, 2011 at 5:13 PM, Noah Misch wrote:
>> Sounds good. ?Updated patch attached, incorporating your ideas. ?Before
>> applying
>> it, run this command to update the uninvolved pg_proc.h DATA entries:
>> ?perl -pi -e 's/PGUID(\s+\d+){4}/$& 0/' src/include/catalog/pg_proc.h
>
> This doesn't quite apply any more. ?I think the pgindent run broke it
> slightly.
Hmm, I just get two one-line offsets when applying it to current master.
?Note
that you need to run the perl invocation before applying the patch. ?Could
you
provide full output of your `patch' invocation, along with any reject
files?
>>>
>>> Ah, crap. ?You're right. ?I didn't follow your directions for how to
>>> apply the patch. ?Sorry.
>>
>> I think you need to update the comment in simplify_function() to say
>> that we have three strategies, rather than two. I think it would also
>> be appropriate to add a longish comment just before the test that
>> calls protransform, explaining what the charter of that function is
>> and why the mechanism exists.
>
> Good idea. See attached.
>
>> Documentation issues aside, I see very little not to like about this.
>
> Great! Thanks for reviewing.
>
Here is my review of this patch.
The patch applies cleanly to the HEAD, produces no additional warnings. It
doesn't include additional regression tests. One can include a test, using the
commands like the ones included below.
Changes to the documentation are limited to the a description of a new
pg_class attribute. It would probably make sense to document all the
exceptions to the table's rewrite on ALTER TABLE documentation page, although
it could wait for more such exceptions.
The feature works as intended and I haven't noticed any crashes or assertion
failures, i.e.
postgres=# create table test(id integer, name varchar);
CREATE TABLE
postgres=# insert into test values(1, 'test');
INSERT 0 1
postgres=# select relfilenode from pg_class where oid='test'::regclass;
relfilenode
-
66302
(1 row)
postgres=# alter table test alter column name type varchar(10);
ALTER TABLE
postgres=# select relfilenode from pg_class where oid='test'::regclass;
relfilenode
-
66308
(1 row)
postgres=# alter table test alter column name type varchar(65535);
ALTER TABLE
postgres=# select relfilenode from pg_class where oid='test'::regclass;
relfilenode
-
66308
(1 row)
The code looks good and takes into account all the previous suggestions.
The only nitpick code-wise is these lines in varchar_transform:
+ int32 old_max = exprTypmod(source) - VARHDRSZ;
+ int32 new_max = new_typmod - VARHDRSZ;
I have a hard time understanding why VARHDRSZ is subtracted here, so I'd
assume that's a bug.
Other than that, I haven't noticed any issues w/ this patch.
Sincerely,
Alexey.
--
Command Prompt, Inc. http://www.CommandPrompt.com
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] Re: [COMMITTERS] pgsql: Fixed string in German translation that causes segfault.
Excerpts from Magnus Hagander's message of mar jun 21 11:01:58 -0400 2011: > On Tue, Jun 21, 2011 at 15:36, Alvaro Herrera > wrote: > > If git submodules are so cool that we still want to use them, maybe we > > still can -- can a submodule be submodule of more than one module? If > > so, we could create one submodule for each subdir that the translations > > are stored in (about 20 currently), and then have a pgtranslation > > meta-project that binds them all together as submodules. > > Can you? Yes. But I doubt that would be very convenient to work with > that many submodules in general. If that's what we're looking at, what > we have now seems a lot more convenient. Yeah, after reading the manpage, I think it would be pretty inconvenient for us. Maybe if we were starting from scratch it would make more sense. (The main pain point is that you can't have the meta-project I suggested above: a submodule is un-updatable from the including tree, you have to check it out separately. So we would have to move all the po files into a single dir, as you said.) -- Álvaro Herrera 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] ALTER TABLE lock strength reduction patch is unsafe
Simon Riggs writes: > On Mon, Jun 20, 2011 at 10:56 PM, Tom Lane 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 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). 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. 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
Robert Haas writes: > On Mon, Jun 20, 2011 at 5:56 PM, Tom Lane wrote: >> Yeah. This behavior has been there since day zero, and there have been >> very few complaints about it. But note that there's only a risk for >> pg_class updates, not any other catalog, and there is exactly one kind >> of failure with very predictable consequences. The ALTER TABLE patch >> has greatly expanded the scope of the issue, and that *is* a regression >> compared to prior releases. > It's not entirely clear to me how many additional failure cases we've > bought ourselves with this patch. The particular one you've > demonstrated seems pretty similar to the on we already had, although > possibly the window for it is wider. It's not so much the probability of failure that is bothering me, as the variety of possible symptoms. There was exactly one failure mode before, namely "no such relation". I'm not sure how many possible symptoms there are now, but there's a lot, and most of them are going to be weird "what the heck was that??" behaviors. If we let 9.1 ship like this, we are going to be creating a support headache. Even worse, knowing that those bugs exist will tempt us to write off reports of weird cache lookup failures as being instances of this problem, when closer investigation might show that they're something else. Please note that this position should not be regarded as support for Simon's proposed patch. I still think the right decision is to revert the ALTER TABLE feature, mainly because I do not believe this is the last bug in it. And the fact that there's a pre-existing bug with a vaguely similar symptom is no justification for introducing more bugs. 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] Fwd: Keywords in pg_hba.conf should be field-specific
2011/6/21 Tom Lane : > Alvaro Herrera writes: >> Excerpts from Pavel Stehule's message of mar jun 21 00:59:44 -0400 2011: >>> because >>pamservice<< - is known keyword, but 'pamservice' is some >>> literal without any mean. You should to use a makro token_is_keyword >>> more often. > >> Yeah, I wondered about this too (same with auth types, i.e. do we accept >> quoted "hostssl" and so on or should that by rejected?). I opted for >> leaving it alone, but maybe this needs to be fixed. (Now that I think >> about it, what we should do first is verify whether it works with quotes >> in the unpatched code). > > AFAICS, this is only important in places where the syntax allows either > a keyword or an identifier. If only a keyword is possible, there is no > value in rejecting it because it's quoted. And, when you do the test, > I think you'll find that it would be breaking hba files that used to > work (though admittedly, it's doubtful that there are any such in the > field). It should be better documented. I don't think so this is good solution, but this is not too important. regards Pavel > > regards, tom lane > -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Fixed string in German translation that causes segfault.
On Tue, Jun 21, 2011 at 15:36, Alvaro Herrera wrote: > Excerpts from Magnus Hagander's message of mar jun 21 07:36:05 -0400 2011: >> On Tue, Jun 21, 2011 at 10:20, Michael Meskes wrote: >> > On Mon, Jun 20, 2011 at 04:44:20PM -0400, Tom Lane wrote: >> >> My recollection is that the current setup was created mainly so that >> >> translators wouldn't need to be given commit privileges on the main >> >> repo. Giving them a separate repo to work in might be all right, but >> >> of course whoever does the merges would have to be careful to only >> >> accept changes made to the .po files and not anything else. >> > >> > IIRC this is exactly what git submodules are for. We could do a git archive >> > only for translations as a submodule for our main git. That way translators >> > would only clone the translation git, while we still have the translations >> > in >> > the source tree in the main git. At least this is how I think it works. >> >> AFAIK (but I could be wrong), git submodules requires the files to be >> in *one* subdirectory. Our .po files are distributed all across the >> backend. So we'd have to make (and backpatch) som rather large changes >> in how these things are built in order to use that. > > If git submodules are so cool that we still want to use them, maybe we > still can -- can a submodule be submodule of more than one module? If > so, we could create one submodule for each subdir that the translations > are stored in (about 20 currently), and then have a pgtranslation > meta-project that binds them all together as submodules. Can you? Yes. But I doubt that would be very convenient to work with that many submodules in general. If that's what we're looking at, what we have now seems a lot more convenient. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fwd: Keywords in pg_hba.conf should be field-specific
Alvaro Herrera writes: > Excerpts from Pavel Stehule's message of mar jun 21 00:59:44 -0400 2011: >> because >>pamservice<< - is known keyword, but 'pamservice' is some >> literal without any mean. You should to use a makro token_is_keyword >> more often. > Yeah, I wondered about this too (same with auth types, i.e. do we accept > quoted "hostssl" and so on or should that by rejected?). I opted for > leaving it alone, but maybe this needs to be fixed. (Now that I think > about it, what we should do first is verify whether it works with quotes > in the unpatched code). AFAICS, this is only important in places where the syntax allows either a keyword or an identifier. If only a keyword is possible, there is no value in rejecting it because it's quoted. And, when you do the test, I think you'll find that it would be breaking hba files that used to work (though admittedly, it's doubtful that there are any such in the field). 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] Fwd: Keywords in pg_hba.conf should be field-specific
On Tue, Jun 21, 2011 at 10:15:50AM -0400, Alvaro Herrera wrote:
> Excerpts from Pavel Stehule's message of mar jun 21 10:04:26 -0400 2011:
> > 2011/6/21 Alvaro Herrera :
> > > Excerpts from Pavel Stehule's message of mar jun 21 00:59:44 -0400 2011:
> > >
> > >> yes - it has a sense. Quoting changes sense from keyword to literal.
> > >> But then I see a significant inconsistency - every know keywords
> > >> should be only tokens.
> > >>
> > >> else if (strcmp(token, "pamservice") == 0)
> > >> - {
> > >> - REQUIRE_AUTH_OPTION(uaPAM, "pamservice", "pam");
> > >> - parsedline->pamservice = pstrdup(c);
> > >> - }
> > >>
> > >> because >>pamservice<< - is known keyword, but 'pamservice' is some
> > >> literal without any mean. You should to use a makro token_is_keyword
> > >> more often.
> > >
> > > Yeah, I wondered about this too (same with auth types, i.e. do we accept
> > > quoted "hostssl" and so on or should that by rejected?). I opted for
> > > leaving it alone, but maybe this needs to be fixed. (Now that I think
> > > about it, what we should do first is verify whether it works with quotes
> > > in the unpatched code).
>
> I tested it and it works: This line
>
> "local" @dbs +b "trust"
>
> is accepted and it works in the unpatched code. I don't think we want
> to break people's existing pg_hba.conf files for no reason. I doubt
> that many people are using pg_hba.conf tokens with quotes, mind you, but
> there might be some ...
>
> In any case, if people here thinks we should tighten this, it's easy to
> do on top of this patch by changing the strcmp() calls to
> token_is_keyword, as you say. Let's not burden this patch with the
> responsibility of doing so, because that's likely to get it punted.
Hmm, would it be possible to add some deprecation warnings for this case
without making the code too messy? Perhaps with a macro
"token_should_be_keyword". That's the usual path to tightening syntax.
Ross
--
Ross Reedstrom, Ph.D. reeds...@rice.edu
Systems Engineer & Admin, Research Scientistphone: 713-348-6166
Connexions http://cnx.orgfax: 713-348-3665
Rice University MS-375, Houston, TX 77005
GPG Key fingerprint = F023 82C8 9B0E 2CC6 0D8E F888 D3AE 810E 88F0 BEDE
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fwd: Keywords in pg_hba.conf should be field-specific
2011/6/21 Alvaro Herrera :
> Excerpts from Pavel Stehule's message of mar jun 21 10:04:26 -0400 2011:
>> 2011/6/21 Alvaro Herrera :
>> > Excerpts from Pavel Stehule's message of mar jun 21 00:59:44 -0400 2011:
>> >
>> >> yes - it has a sense. Quoting changes sense from keyword to literal.
>> >> But then I see a significant inconsistency - every know keywords
>> >> should be only tokens.
>> >>
>> >> else if (strcmp(token, "pamservice") == 0)
>> >> - {
>> >> - REQUIRE_AUTH_OPTION(uaPAM, "pamservice", "pam");
>> >> - parsedline->pamservice = pstrdup(c);
>> >> - }
>> >>
>> >> because >>pamservice<< - is known keyword, but 'pamservice' is some
>> >> literal without any mean. You should to use a makro token_is_keyword
>> >> more often.
>> >
>> > Yeah, I wondered about this too (same with auth types, i.e. do we accept
>> > quoted "hostssl" and so on or should that by rejected?). I opted for
>> > leaving it alone, but maybe this needs to be fixed. (Now that I think
>> > about it, what we should do first is verify whether it works with quotes
>> > in the unpatched code).
>
> I tested it and it works: This line
>
> "local" @dbs +b "trust"
>
> is accepted and it works in the unpatched code. I don't think we want
> to break people's existing pg_hba.conf files for no reason. I doubt
> that many people are using pg_hba.conf tokens with quotes, mind you, but
> there might be some ...
>
> In any case, if people here thinks we should tighten this, it's easy to
> do on top of this patch by changing the strcmp() calls to
> token_is_keyword, as you say. Let's not burden this patch with the
> responsibility of doing so, because that's likely to get it punted.
It is time to discuss about it.
I thinking so current behave is strange and should be fixed - it
doesn't respect a description stored in pg_hba.conf. I agree, so this
will have impact on compatibility, but pg_hba is config file so this
impact is not too hard. The cleaning now can carry a benefit in
future, when pg_hba can be more complex.
Regards
Pavel
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fwd: Keywords in pg_hba.conf should be field-specific
Excerpts from Pavel Stehule's message of mar jun 21 10:04:26 -0400 2011:
> 2011/6/21 Alvaro Herrera :
> > Excerpts from Pavel Stehule's message of mar jun 21 00:59:44 -0400 2011:
> >
> >> yes - it has a sense. Quoting changes sense from keyword to literal.
> >> But then I see a significant inconsistency - every know keywords
> >> should be only tokens.
> >>
> >> else if (strcmp(token, "pamservice") == 0)
> >> - {
> >> - REQUIRE_AUTH_OPTION(uaPAM, "pamservice", "pam");
> >> - parsedline->pamservice = pstrdup(c);
> >> - }
> >>
> >> because >>pamservice<< - is known keyword, but 'pamservice' is some
> >> literal without any mean. You should to use a makro token_is_keyword
> >> more often.
> >
> > Yeah, I wondered about this too (same with auth types, i.e. do we accept
> > quoted "hostssl" and so on or should that by rejected?). I opted for
> > leaving it alone, but maybe this needs to be fixed. (Now that I think
> > about it, what we should do first is verify whether it works with quotes
> > in the unpatched code).
I tested it and it works: This line
"local" @dbs +b "trust"
is accepted and it works in the unpatched code. I don't think we want
to break people's existing pg_hba.conf files for no reason. I doubt
that many people are using pg_hba.conf tokens with quotes, mind you, but
there might be some ...
In any case, if people here thinks we should tighten this, it's easy to
do on top of this patch by changing the strcmp() calls to
token_is_keyword, as you say. Let's not burden this patch with the
responsibility of doing so, because that's likely to get it punted.
--
Álvaro Herrera
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] Fwd: Keywords in pg_hba.conf should be field-specific
2011/6/21 Alvaro Herrera :
> Excerpts from Pavel Stehule's message of mar jun 21 00:59:44 -0400 2011:
>
>> yes - it has a sense. Quoting changes sense from keyword to literal.
>> But then I see a significant inconsistency - every know keywords
>> should be only tokens.
>>
>> else if (strcmp(token, "pamservice") == 0)
>> - {
>> - REQUIRE_AUTH_OPTION(uaPAM, "pamservice", "pam");
>> - parsedline->pamservice = pstrdup(c);
>> - }
>>
>> because >>pamservice<< - is known keyword, but 'pamservice' is some
>> literal without any mean. You should to use a makro token_is_keyword
>> more often.
>
> Yeah, I wondered about this too (same with auth types, i.e. do we accept
> quoted "hostssl" and so on or should that by rejected?). I opted for
> leaving it alone, but maybe this needs to be fixed. (Now that I think
> about it, what we should do first is verify whether it works with quotes
> in the unpatched code).
>
It's question about compatibility - sure. But a description inside
pg_hba.conf speaks cleanly - quoting means a lost of original
semantic.
And if we allow a quoting somewhere, then I can't imagine a
description - "somewhere quoting means a string literal, somewhere
have not impact?"
Regards
Pavel Stehule
> --
> Álvaro Herrera
> 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] Fwd: Keywords in pg_hba.conf should be field-specific
Excerpts from Pavel Stehule's message of mar jun 21 00:59:44 -0400 2011:
> yes - it has a sense. Quoting changes sense from keyword to literal.
> But then I see a significant inconsistency - every know keywords
> should be only tokens.
>
> else if (strcmp(token, "pamservice") == 0)
> - {
> - REQUIRE_AUTH_OPTION(uaPAM, "pamservice", "pam");
> - parsedline->pamservice = pstrdup(c);
> - }
>
> because >>pamservice<< - is known keyword, but 'pamservice' is some
> literal without any mean. You should to use a makro token_is_keyword
> more often.
Yeah, I wondered about this too (same with auth types, i.e. do we accept
quoted "hostssl" and so on or should that by rejected?). I opted for
leaving it alone, but maybe this needs to be fixed. (Now that I think
about it, what we should do first is verify whether it works with quotes
in the unpatched code).
--
Álvaro Herrera
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] ALTER TABLE lock strength reduction patch is unsafe
On Mon, Jun 20, 2011 at 6:55 PM, Simon Riggs wrote: > 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. All I > would say is that issuing a stream of ALTER TABLEs against the same > table is not a common situation; if it were we would have seen more of > the pre-existing bug. ALTER TABLE command encompasses many subcommands > and we should evaluate each subcommand differently when we decide what > to do. Well, my principal objection is that I think heavyweight locking is an excessively expensive solution to this problem. I think the patch is simple enough that I wouldn't object to applying it on those grounds even at this late date, but I bet if we do some benchmarking on the right workload we'll find a significant performance regression. -- 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] ALTER TABLE lock strength reduction patch is unsafe
On Mon, Jun 20, 2011 at 5:56 PM, Tom Lane wrote: > Robert Haas writes: >> On Sun, Jun 19, 2011 at 5:13 PM, Simon Riggs wrote: >>> We scan pg_class in two ways: to rebuild a relcache entry based on a >>> relation's oid (easy fix). We also scan pg_class to resolve the name >>> to oid mapping. The name to oid mapping is performed *without* a lock >>> on the relation, since we don't know which relation to lock. So the >>> name lookup can fail if we are in the middle of a pg_class update. >>> This is an existing potential bug in Postgres unrelated to my patch. > >> If this is a pre-existing bug, then it's not clear to me why we need >> to do anything about it at all right now. > > Yeah. This behavior has been there since day zero, and there have been > very few complaints about it. But note that there's only a risk for > pg_class updates, not any other catalog, and there is exactly one kind > of failure with very predictable consequences. The ALTER TABLE patch > has greatly expanded the scope of the issue, and that *is* a regression > compared to prior releases. It's not entirely clear to me how many additional failure cases we've bought ourselves with this patch. The particular one you've demonstrated seems pretty similar to the on we already had, although possibly the window for it is wider. Did you run the same test you used on 9.1 on 9.0 for comparison? > BTW, it seems to me that this issue is closely related to what Noah is > trying to fix here: > http://archives.postgresql.org/message-id/20110612191843.gf21...@tornado.leadboat.com I think there are two general problems here. One, we use SnapshotNow to scan system catalogs, and SnapshotNow semantics are a mess. Two, even if we used an MVCC snapshot to scan the system catalogs, it's not necessarily safe or correct to latch onto an old row version, because the row update is combined with other actions that are not MVCC-safe, like removing files on disk. Breaking it down a bit more: A. The problem with using a DDL lock level < AccessExclusiveLock, AFAICS, is entirely due to SnapshotNow semantics. Any row version we can possibly see is OK, but we had better see exactly one. Or at least not less than one. B. The problem with name lookups failing in the middle of a pg_class update is also entirely due to SnapshotNow semantics. C. The problem Noah is complaining about is NOT due to SnapshotNow semantics. If someone does BEGIN; DROP TABLE foo; CREATE TABLE foo(); COMMIT, it is no longer OK for anyone to be looking at the old foo. An MVCC snapshot is enough to guarantee that we'll see some row, but examining the old one won't cut it. -- 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] Re: [COMMITTERS] pgsql: Fixed string in German translation that causes segfault.
Excerpts from Magnus Hagander's message of mar jun 21 07:36:05 -0400 2011: > On Tue, Jun 21, 2011 at 10:20, Michael Meskes wrote: > > On Mon, Jun 20, 2011 at 04:44:20PM -0400, Tom Lane wrote: > >> My recollection is that the current setup was created mainly so that > >> translators wouldn't need to be given commit privileges on the main > >> repo. Giving them a separate repo to work in might be all right, but > >> of course whoever does the merges would have to be careful to only > >> accept changes made to the .po files and not anything else. > > > > IIRC this is exactly what git submodules are for. We could do a git archive > > only for translations as a submodule for our main git. That way translators > > would only clone the translation git, while we still have the translations > > in > > the source tree in the main git. At least this is how I think it works. > > AFAIK (but I could be wrong), git submodules requires the files to be > in *one* subdirectory. Our .po files are distributed all across the > backend. So we'd have to make (and backpatch) som rather large changes > in how these things are built in order to use that. If git submodules are so cool that we still want to use them, maybe we still can -- can a submodule be submodule of more than one module? If so, we could create one submodule for each subdir that the translations are stored in (about 20 currently), and then have a pgtranslation meta-project that binds them all together as submodules. -- Álvaro Herrera 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] Re: [COMMITTERS] pgsql: Fixed string in German translation that causes segfault.
On Tue, Jun 21, 2011 at 01:36:05PM +0200, Magnus Hagander wrote: > AFAIK (but I could be wrong), git submodules requires the files to be > in *one* subdirectory. Our .po files are distributed all across the > backend. So we'd have to make (and backpatch) som rather large changes > in how these things are built in order to use that. Ah, good point. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org Jabber: michael.meskes at googlemail dot com VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL -- 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] pika buildfarm member failure on isolationCheck tests
On 21.06.2011 05:18, Dan Ports wrote: The first patch addresses this bug by re-adding SXACT_FLAG_ROLLED_BACK, in a more limited form than its previous incarnation. We need to be able to distinguish transactions that have already called ReleasePredicateLocks and are thus eligible for cleanup from those that have been merely marked for abort by other backends. Transactions that are ROLLED_BACK are excluded from SxactGlobalXmin calculations, but those that are merely DOOMED need to be included. Also update a couple of assertions to ensure we only try to clean up ROLLED_BACK transactions. Thanks, committed. The second patch fixes a bug in PreCommit_CheckForSerializationFailure. This function checks whether there's a dangerous structure of the form far ---> near ---> me where neither the "far" or "near" transactions have committed. If so, it aborts the "near" transaction by marking it as DOOMED. However, that transaction might already be PREPARED. We need to check whether that's the case and, if so, abort the transaction that's trying to commit instead. Yep, committed. All the other places where we set the DOOMED flag seem to handle that already. In the long term, I'm not sure this is the best way to handle this. It feels a bit silly to set the flag, release the SerializableXactHashLock, and reacquire it later to remove the transaction from the hash table. Surely that could be done in some more straightforward way. But I don't feel like fiddling with it this late in the release cycle. One of the prepared_xacts regression tests actually hits this bug. I removed the anomaly from the duplicate-gids test so that it fails in the intended way, and added a new test to check serialization failures with a prepared transaction. Hmm, I have ran "make installcheck" with default_transaction_isolation='serializable' earlier. I wonder why I didn't see 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] proposal: a validator for configuration files
On Jun 20, 2011, at 6:22 PM, Florian Pflug wrote: > On Jun20, 2011, at 17:02 , Alexey Klyukin wrote: >> >> I don't think it has changed at all. Previously, we did goto cleanup_list (or >> cleanup_exit in ParseConfigFp) right after the first error, no matter whether >> that was a postmaster or its child. What I did in my patch is removing the >> goto for the postmaster's case. It was my intention to exit after the initial >> error for the postmaster's child, to avoid complaining about all errors both >> in the postmaster and in the normal backend (imagine seeing 100 errors from >> the postmaster and the same 100 from each of the backends if your log level >> is >> DEBUG2). I think the postmaster's child case won't cause any problems, since >> we do exactly what we used to do before. > > Hm, I think you miss-understood what I was trying to say, probably because I > explained it badly. Let me try again. > > I fully agree that there *shouldn't* be any difference in behaviour, because > it *shouldn't* matter whether we abort early or not - we won't have applied > any of the settings anway. > > But. > > The code the actually implements the "check settings first, apply later" logic > isn't easy to read. Now, assume that this code has a bug. Then, with your > patch applied, we might end up with the postmaster applying a setting (because > it didn't abort early) but the backend ignoring it (because they did abort > early). > This is obviously bad. Depending on the setting, the consequences may range > from slightly confusing behaviour to outright crashes I guess... > > Now, the risk of that happening might be very small. But on the other hand, > the benefit is pretty small also - you get a little less output for log level > DEBUG2, that's it. A level that people probably don't use for the production > databases anyway. This convinced me that the risk/benefit ratio isn't high > enough > to warrant the early abort. > > Another benefit of removing the check is that it reduces code complexity. > Maybe > not when measured in line counts, but it's one less outside factor that > changes > ProcessConfigFiles()'s behaviour and thus one thing less you need to think > when > you modify that part again in the future. Again, it's a small benefit, but > IMHO > it still outweights the benefit. While I agree that making the code potentially less bug prone is a good idea, I don't agree with the statement that since DEBUG2 output gets rarely turned on we can make it less usable, hoping that nobody would use in production. > > Having said that, this is my personal opinion and whoever will eventually > commit this may very will assess the cost/benefit ratio differently. So, if > after this more detailed explanations of my reasoning, you still feel that > it makes sense to keep the early abort, then feel free to mark the > patch "Ready for Committer" nevertheless. I'd say that this issue is a judgement call. Depending on a point of view, both arguments are valid. I've marked this patch as 'Ready for Committer' w/o removing the early abort stuff, but if there will be more people willing to remove them - I'll do that. Thank you, Alexey. -- Command Prompt, Inc. http://www.CommandPrompt.com 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] Re: [COMMITTERS] pgsql: Fixed string in German translation that causes segfault.
On Tue, Jun 21, 2011 at 10:20, Michael Meskes wrote: > On Mon, Jun 20, 2011 at 04:44:20PM -0400, Tom Lane wrote: >> My recollection is that the current setup was created mainly so that >> translators wouldn't need to be given commit privileges on the main >> repo. Giving them a separate repo to work in might be all right, but >> of course whoever does the merges would have to be careful to only >> accept changes made to the .po files and not anything else. > > IIRC this is exactly what git submodules are for. We could do a git archive > only for translations as a submodule for our main git. That way translators > would only clone the translation git, while we still have the translations in > the source tree in the main git. At least this is how I think it works. AFAIK (but I could be wrong), git submodules requires the files to be in *one* subdirectory. Our .po files are distributed all across the backend. So we'd have to make (and backpatch) som rather large changes in how these things are built in order to use that. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] hstore - Implementation and performance issues around its operators
Hi!
On Tue, Jun 21, 2011 at 12:04 PM, Stefan Keller wrote:
> 1. Obviously the '@>' has to be used in order to let use the GiST index.
> Why is the '->' operator not supported by GiST ('->' is actually
> mentioned in all examples of the doc.)?
>
I believe it's an architecture problem. You actually need not '->' operator
to be supported by GiST but "column -> 'field_name' = value" expression.
Probably, I'm missing something, but I think supporting of this require
significant catalog changes.
--
With best regards,
Alexander Korotkov.
Re: [HACKERS] WIP: Fast GiST index build
Hi! I've created section about testing in project wiki page: http://wiki.postgresql.org/wiki/Fast_GiST_index_build_GSoC_2011#Testing_results Do you have any notes about table structure? As you can see I found that CPU usage might be much higher with gist_trgm_ops. I believe it's due to relatively expensive penalty method in that opclass. But, probably index build can be still faster when index doesn't fit cache even for gist_trgm_ops. Also with that opclass index quality is slightly worse but the difference is not dramatic. -- With best regards, Alexander Korotkov.
Re: [HACKERS] Latch implementation that wakes on postmaster death on both win32 and Unix
Thanks for giving this your attention Fujii. Attached patch addresses
your concerns.
On 20 June 2011 05:53, Fujii Masao wrote:
> 'hifd' should be initialized to 'selfpipe_readfd' before the above
> 'if' block. Otherwise,
> 'hifd = postmaster_alive_fds[POSTMASTER_FD_WATCH]' might have no effect.
That's an oversight that I should have caught. Fixed.
> Why does the archive still need to wake up periodically?
That is consistent with its earlier behaviour..."she wakes up
occasionally to allow herself to be proactive". This comment does not
refer to the frequent updates that currently occur within the tight
polling loop. I think any concern about that would apply equally to
the original, unpatched code.
> Is the variable 'flag' really required? It's not used by fcntl() to
> set the fd nonblocking.
Yes, it's superfluous. Removed.
> Is FNONBLOCK equal to O_NONBLOCK? If yes, we should use O_NONBLOCK
> for the sake of consistency? In other code (e.g., noblock.c), O_NONBLOCK is
> used
> rather than FNONBLOCK.
FNONBLOCK is just an alias for O_NONBLOCK, so it seems reasonable to
be consistent in which variant we use. I have found suggestions that
it might break the build on OSX, so if that's true there's an
excellent reason to prefer the latter.
> I think that it's worth that walsender checks the postmaster death event. No?
It does check it, but only in the same way that it always has (a tight
polling loop). I would like to make walsender use the new
functionality. That is another patch though, that I thought best to
have independently reviewed, only when this patch is committed. I've
only made the walsender use the new interface, changing as little as
possible and not affecting walsender's behaviour, as a stopgap towards
that patch.
--
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 aa0b029..691ac42 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -10161,7 +10161,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..f97d838 100644
--- a/src/backend/port/unix_latch.c
+++ b/src/backend/port/unix_latch.c
@@ -93,7 +93,9 @@
#endif
#include "miscadmin.h"
+#include "postmaster/postmaster.h"
#include "storage/latch.h"
+#include "storage/pmsignal.h"
#include "storage/shmem.h"
/* Are we currently in WaitLatch? The signal handler would like to know. */
@@ -188,22 +190,25 @@ DisownLatch(volatile Latch *latch)
* backend-local latch initialized with InitLatch, or a shared latch
* associated with the current process by calling OwnLatch.
*
- * Returns 'true' if the latch was set, or 'false' if timeout was reached.
+ * Returns bit field indicating which condition(s) caused the wake-up.
+ *
+ * Note that there is no guarantee that callers will have all wake-up conditions
+ * returned, but we will report at least one.
*/
-bool
-WaitLatch(volatile Latch *latch, long timeout)
+int
+WaitLatch(volatile Latch *latch, int wakeEvents, long timeout)
{
- return WaitLatchOrSocket(latch, PGINVALID_SOCKET, false, false, timeout) > 0;
+ return WaitLatchOrSocket(latch, wakeEvents, PGINVALID_SOCKET, timeout);
}
/*
* Like WaitLatch, but will also return when there's data available in
- * 'sock' for reading or writing. Returns 0 if timeout was reached,
- * 1 if the latch was set, 2 if the socket became readable or writable.
+ * 'sock' for reading or writing.
+ *
+ * Returns same bit mask and makes same guarantees as WaitLatch.
*/
int
-WaitLatchOrSocket(volatile Latch *latch, pgsocket sock, bool forRead,
- bool forWrite, long timeout)
+WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket sock, long timeout)
{
struct timeval tv,
*tvp = NULL;
@@ -211,12 +216,13 @@ WaitLatchOrSocket(volatile Latch *latch, pgsocket sock, bool forRead,
fd_set output_mask;
int rc;
int result = 0;
+ bool found = false;
if (latch->owner_pid != MyProcPid)
elog(ERROR, "cannot wait on a latch owned by another process");
/* Initialize timeout */
- if (timeout >= 0)
+ if (timeout >= 0 && (wakeEvents & WL_TIMEOUT))
{
tv.tv_sec = timeout / 100L;
tv.tv_usec = timeout % 100L;
@@ -224,7 +230,7 @@ WaitLatchOrSocket(volatile Latch *latch, pgsocket sock, bool forRead,
}
waiting = true;
- for (;;)
+ do
{
int hifd;
@@ -235,16 +241,31 @@ WaitLatchOrSocket(volatile Latch *latch, pgsocket sock, bool forRead,
* do that), and the select() will return immediately.
*/
drainSelfPipe();
- if (latch->is_set)
+ if (latch->is_set && (wakeEvents & WL_L
Re: [HACKERS] Range Types and extensions
On Mon, 2011-06-20 at 13:43 -0400, Tom Lane wrote: > The other viable alternative seems to be to require those two properties > (btree opclass and collation) to be part of a specific range type > definition. The complaint about that seemed to be that we couldn't > infer an ANYRANGE type given only ANYELEMENT, but could we alleviate > that by identifying one range type as the default for the base type, > and then using that one in cases where we have no ANYRANGE input? Yes, that sounds similar to Florian's suggestion, and I think there may be a solution down this path. However, if we're going to have range types with non-default orderings, then we need a way to construct them. I suggested that, if constructors are the primary problem case, then just generate non-polymorphic constructors at range type definition time, named after the range type name. I'll look into that approach. 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] Range Types and extensions
On Mon, 2011-06-20 at 12:54 -0700, Darren Duncan wrote: > That DOMAIN-based solution ostensibly sounds like a good one then, under the > circumstances. It's not bad from a theoretical standpoint, but it does require some extra type annotation, which is not really the "SQL way". > What I *don't* want to see is for things like ranges to have > their own collations and the like. I'm not 100% sure what you mean here. If you mean that you don't want range types to pay attention to COLLATE clauses, etc., then I agree. I would also agree if you mean that range values should not carry the collation with them. However, it looks like we might try to make the opclass/collation pair a property of the range type definition. That seems nice, because it allows us to keep the nice properties of ranges as well as the type inference and polymorphism for everything except the constructors. 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] Re: [COMMITTERS] pgsql: Fixed string in German translation that causes segfault.
On Mon, Jun 20, 2011 at 04:44:20PM -0400, Tom Lane wrote: > My recollection is that the current setup was created mainly so that > translators wouldn't need to be given commit privileges on the main > repo. Giving them a separate repo to work in might be all right, but > of course whoever does the merges would have to be careful to only > accept changes made to the .po files and not anything else. IIRC this is exactly what git submodules are for. We could do a git archive only for translations as a submodule for our main git. That way translators would only clone the translation git, while we still have the translations in the source tree in the main git. At least this is how I think it works. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org Jabber: michael.meskes at googlemail dot com VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] hstore - Implementation and performance issues around its operators
Hi,
We did a benchmark comparing a Key-Value-Pairs stored as EAV db schema
versus hstore. The results are promising in favor of hstore but there are some
question which remain.
1. Obviously the '@>' has to be used in order to let use the GiST index.
Why is the '->' operator not supported by GiST ('->' is actually
mentioned in all examples of the doc.)?
2. Currently the hstore elements are stored in order as they are
coming from the insert statement / constructor.
Why are the elements not ordered i.e. why is the hstore not cached in
all hstore functions (like hstore_fetchval etc.)?
3. In the source code 'hstore_io.c' one finds the following enigmatic
note: "... very large hstore values can't be output. this could be
fixed, but many other data types probably have the same issue."
What is the max. length of a hstore (i.e. the max. length of the sum
of all elements in text representation)?
4. Last, I don't fully understand the following note in the hstore
doc. (http://www.postgresql.org/docs/current/interactive/hstore.html
):
> Notice that the old names are reversed from the convention
> formerly followed by the core geometric data types!
Why names? Why not rather 'operators' or 'functions'?
What does this "reversed from the convention" mean concretely?
Yours, Stefan
P.S. I already tried to ask these questions to postgres-performance
and to the hstore authors without success...
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
