Re: [HACKERS] Commitfest app not exporting "moved to another commitfest" to rss
Brar Piening wrote: I use rss to follow up on patches that I'm interested in and it's the second time I was wonering where my patch has gone in the commitfest app due to $Topic. Just after pushing the send button my RSS-feed got updated and contained the relevant information. Sorry for the noise! I don't actually understand the delay in my feed getting updated despite the fact that I'm regularly updating it on my side. Regards, Brar -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Commitfest app not exporting "moved to another commitfest" to rss
I use rss to follow up on patches that I'm interested in and it's the second time I was wonering where my patch has gone in the commitfest app due to $Topic. Is this a known limitation? If yes: Is there a way to change this? If yes: Can/shall I help? If yes: Where should I start? Regards, Brar -- 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
Sorry. I was not previously able to answer fujii's all comments. This is the remaining answers. > + LWLockAcquire(WALInsertLock, LW_EXCLUSIVE); > + XLogCtl->Insert.fullPageWrites = fullPageWrites; > + LWLockRelease(WALInsertLock); > > I don't think WALInsertLock needs to be hold here because there is no > concurrently running process which can access Insert.fullPageWrites. > For example, Insert->currpos and Insert->LogwrtResult are also changed > without the lock there. > Yes. > The source comment of XLogReportParameters() needs to be modified. Yes, too. 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
Re: [HACKERS] Online base backup from the hot-standby
> > > ERROR: full_page_writes on master is set invalid at least once since > > > latest checkpoint > > > > > > I think this error should be rewritten as > > > ERROR: full_page_writes on master has been off at some point since > > > latest checkpoint > > > > > > We should be using 'off' instead of 'invalid' since that is what is what > > > the user sets it to. > > > > Sure. > > What about the following message? It sounds more precise to me. > > ERROR: WAL generated with full_page_writes=off was replayed since last > restartpoint Okay, I changes the patch to this messages. If someone says there is a idea better than it, I will consider again. > > I updated to patch corresponded above-comments. > > Thanks for updating the patch! Here are the comments: > >* don't yet have the insert lock, forcePageWrites could change under > us, >* but we'll recheck it once we have the lock. >*/ > - doPageWrites = fullPageWrites || Insert->forcePageWrites; > + doPageWrites = Insert->fullPageWrites || Insert->forcePageWrites; > > The source comment needs to be modified. > >* just turned off, we could recompute the record without full pages, > but >* we choose not to bother.) >*/ > - if (Insert->forcePageWrites && !doPageWrites) > + if ((Insert->fullPageWrites || Insert->forcePageWrites) && > !doPageWrites) > > Same as above. Sure. > XLogReportParameters() should skip writing WAL if full_page_writes has not > been > changed by SIGHUP. > > XLogReportParameters() should skip updating pg_control if any parameter > related > to hot standby has not been changed. YES. > In checkpoint, XLogReportParameters() is called only when wal_level is > hot_standby. > OTOH, in walwriter, it's always called even when wal_level is not hot_standby. > Can't we skip calling XLogReportParameters() whenever wal_level is not > hot_standby? Yes, It is possible. > In do_pg_start_backup() and do_pg_stop_backup(), the spinlock must be held to > see XLogCtl->lastFpwDisabledLSN. Yes. > What about changing the error message to: > ERROR: WAL generated with full_page_writes=off was replayed during online > backup Okay, too. 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
Re: [HACKERS] pl/perl example in the doc no longer works in 9.1
On Wed, Oct 12, 2011 at 15:33, Alex Hunsaker wrote:
> On Wed, Oct 12, 2011 at 15:00, Tom Lane wrote:
>> The core of the problem seems to be that if SvROK(sv) then
>> the code assumes that it must be intended to convert that to an array or
>> composite, no matter whether the declared result type of the function is
>> compatible with such a thing.
>
> Hrm, well 9.0 and below did not get this "right" either:
> create or replace function test_hash() returns text as $$ return
> {'a'=>1}; $$ language plperl;
> select test_array();
> test_array
> ---
> ARRAY(0x7fd92384dcb8)
> (1 row)
>
> create or replace function test_hash() returns text as $$ return
> {'a'=>1}; $$ language plperl;
> select test_hash();
> test_hash
> --
> HASH(0x7fd92387f848)
> (1 row)
>
> Given the output above (both pre 9.1 and post) it seems unless the
> type is a set or composite we should throw an error. Maybe "PL/Perl
> function returning type %s must not return a reference" ?
>
>> It would be more appropriate to drive the
>> cases off the nature of the function result type, perhaps.
>
> Ill see if I can cook up something that's not too invasive.
PFA my attempt at a fix.
This gets rid of of most of the if/else chain and the has_retval crap
in plperl_handl_func(). Instead we let plperl_sv_to_datum() do most of
the lifting. It also now handles VOIDOID and checks that the request
result oid can be converted from the perl structure. For example if
you passed in a hashref with a result oid that was not an rowtype it
will error out with "PL/Perl cannot convert hash to non rowtype %s".
Arrays behave similarly.
One side effect is you can now return a composite literal where you
could not before. ( We already let you return array literals )
The comments might still be a bit sparse-- Im hoping the added errors
make things a bit more self explanatory.
A large portion of the diff is added regression tests, testing what
happens when you return various references.
Comments?
plperl_returns.patch.gz
Description: GNU Zip compressed 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] [v9.2] make_greater_string() does not return a string in some cases
Hello, the work is finished.
Version 4 of the patch is attached to this message.
- Add rough description of the algorithm as comment to
pg_utf8_increment() and pg_eucjp_increment().
- Fixed a bug of pg_utf8_increment() found while
working. pg_(utf8|eucjp)_increment are retested on whole valid
code points to be properly handled.
- The comment previously pointed out as being wrong in grammar
is left untouched. I'm sorry to bother you with my poor
English.
At Tue, 11 Oct 2011 16:55:00 +0900 (JST), Kyotaro HORIGUCHI
wrote
> > One thing I still think it would be useful to add,
> > though, is some comments to pg_utf8_increment() and
> > pg_eucjp_increment() describing the algorithm being used. Can you
> > take a crack at that?
>
> Yes I'll do it in a day or two.
Regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index 8ceea82..59f8c37 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -5664,6 +5664,19 @@ pattern_selectivity(Const *patt, Pattern_Type ptype)
/*
+ * This function is "character increment" function for bytea used in
+ * make_greater_string() that has same interface with pg_wchar_tbl.charinc.
+ */
+static bool
+byte_increment(unsigned char *ptr, int len)
+{
+ if (*ptr >= 255) return false;
+
+ (*ptr)++;
+ return true;
+}
+
+/*
* Try to generate a string greater than the given string or any
* string it is a prefix of. If successful, return a palloc'd string
* in the form of a Const node; else return NULL.
@@ -5702,6 +5715,7 @@ make_greater_string(const Const *str_const, FmgrInfo *ltproc, Oid collation)
int len;
Datum cmpstr;
text *cmptxt = NULL;
+ character_incrementer charincfunc;
/*
* Get a modifiable copy of the prefix string in C-string format, and set
@@ -5763,29 +5777,33 @@ make_greater_string(const Const *str_const, FmgrInfo *ltproc, Oid collation)
}
}
+ if (datatype == BYTEAOID)
+ charincfunc = &byte_increment;
+ else
+ charincfunc = pg_database_encoding_character_incrementer();
+
while (len > 0)
{
- unsigned char *lastchar = (unsigned char *) (workstr + len - 1);
- unsigned char savelastchar = *lastchar;
+ int charlen = 1;
+ unsigned char *lastchar;
+ Const *workstr_const;
+
+ if (datatype != BYTEAOID)
+ charlen = len - pg_mbcliplen(workstr, len, len - 1);
+
+ lastchar = (unsigned char *) (workstr + len - charlen);
/*
- * Try to generate a larger string by incrementing the last byte.
+ * Try to generate a larger string by incrementing the last byte or
+ * character.
*/
- while (*lastchar < (unsigned char) 255)
- {
- Const *workstr_const;
-
- (*lastchar)++;
- if (datatype != BYTEAOID)
- {
-/* do not generate invalid encoding sequences */
-if (!pg_verifymbstr(workstr, len, true))
- continue;
-workstr_const = string_to_const(workstr, datatype);
- }
- else
+ if (charincfunc(lastchar, charlen))
+ {
+ if (datatype == BYTEAOID)
workstr_const = string_to_bytea_const(workstr, len);
+ else
+workstr_const = string_to_const(workstr, datatype);
if (DatumGetBool(FunctionCall2Coll(ltproc,
collation,
@@ -5804,20 +5822,10 @@ make_greater_string(const Const *str_const, FmgrInfo *ltproc, Oid collation)
pfree(workstr_const);
}
- /* restore last byte so we don't confuse pg_mbcliplen */
- *lastchar = savelastchar;
-
/*
- * Truncate off the last character, which might be more than 1 byte,
- * depending on the character encoding.
+ * Truncate off the last character or byte.
*/
- if (datatype != BYTEAOID && pg_database_encoding_max_length() > 1)
- len = pg_mbcliplen(workstr, len, len - 1);
- else
- len -= 1;
-
- if (datatype != BYTEAOID)
- workstr[len] = '\0';
+ len -= charlen;
}
/* Failed... */
diff --git a/src/backend/utils/mb/wchar.c b/src/backend/utils/mb/wchar.c
index f23732f..a213636 100644
--- a/src/backend/utils/mb/wchar.c
+++ b/src/backend/utils/mb/wchar.c
@@ -1336,6 +1336,264 @@ pg_utf8_islegal(const unsigned char *source, int length)
/*
*---
+ * character incrementer
+ *
+ * These functions accept "charptr", a pointer to the first byte of a
+ * maybe-multibyte character. Try `increment' the character and return
+ * true if successed. If these functions returns false, the character
+ * should be untouched. These functions must be implemented in
+ * correspondence with verifiers, in other words, the rewrited
+ * character by this function must pass the check by pg_*_verifier()
+ * if returns true.
+ * ---
+ */
+
+#ifndef FRONTEND
+static bool
+pg_generic_charinc(unsigned char *charptr, int len)
+{
+ unsigned char *lastchar = (unsigned char *) (charptr + len - 1);
+ unsigned char savelastchar = *lastchar;
+ const char *const_char
Re: [HACKERS] unite recovery.conf and postgresql.conf
Simon, I haven't see a response from you on a proposed way to keep backwards compatibility with recovery.conf as a trigger file, while also eliminating its trigger status as an unmanagable misfeature. As far as I can tell, that's the one area where we *cannot* maintain backwards compatibility. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_comments (was: Allow \dd to show constraint comments)
On Wed, Oct 12, 2011 at 2:49 PM, Robert Haas wrote: > So, I think the critical question for this patch is "do we want > this?". Yep. Or put another way, are the gains worth having another system view we'll have to maintain forever? > Tom didn't like it, In [1], Tom seemed to be mainly angling for fixing up psql instead, which has been done now. I didn't see a specific reason against adding the view, other than it "cannot be changed without an initdb". That's a valid concern of course, but it applies equally well to other system views. [snip] > On the third hand, Josh's previous batch of changes to clean up > psql's behavior in this area are clearly a huge improvement: you can > now display the comment for nearly anything by running the appropriate > \d command for whatever the object type is. So ... is this still > a good idea, or should we just forget about it? I think this question is a part of a broader concern, namely do we want to create and support system views for easier access to information which is already available in different ways through psql commands, or by manually digging around in the catalogs? I believe there are at least several examples of existing views we maintain which are very similar to pg_comments: pg_seclabel seems quite similar, for instance. Josh -- [1] http://archives.postgresql.org/pgsql-hackers/2010-09/msg01081.php -- 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] loss of transactions in streaming replication
On Wed, Oct 12, 2011 at 10:29 PM, Robert Haas wrote: > On Wed, Oct 12, 2011 at 5:45 AM, Fujii Masao wrote: >> In 9.2dev and 9.1, when walreceiver detects an error while sending data to >> WAL stream, it always emits ERROR even if there are data available in the >> receive buffer. This might lead to loss of transactions because such >> remaining data are not received by walreceiver :( > > Won't it just reconnect? Yes if the master is running normally. OTOH, if the master is not running (i.e., failover case), the standby cannot receive again the data which it failed to receive. I found this issue when I shut down the master. When the master shuts down, it sends the shutdown checkpoint record, but I found that the standby failed to receive it. 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] Patch: Perl xsubpp
On Wed, Oct 12, 2011 at 17:53, David E. Wheeler wrote: > On Sep 15, 2011, at 3:04 PM, Alex Hunsaker wrote: > >> Close, seems I was wrong about the typemap ExtUtils::ParseXS does not >> install a new one so we still need to point to the one in privlib. >> Also xsubpp is not executable so the test should be -r or something. >> >> Also don't think we should change the configure switch tests to test >> XSUBPPDIR. >> >> Find those plus some minor typos fixed in the attached. >> >> -- > > Doesn't look like this has been applied yet. I think it ought to be > backported, too, frankly. DId I miss it? Nah, probably should add it to the next commit fest so it does not get forgotten. -- 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: Perl xsubpp
On Sep 15, 2011, at 3:04 PM, Alex Hunsaker wrote: > Close, seems I was wrong about the typemap ExtUtils::ParseXS does not > install a new one so we still need to point to the one in privlib. > Also xsubpp is not executable so the test should be -r or something. > > Also don't think we should change the configure switch tests to test > XSUBPPDIR. > > Find those plus some minor typos fixed in the attached. > > -- Doesn't look like this has been applied yet. I think it ought to be backported, too, frankly. DId I miss it? Best, David -- 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: move dumpUserConfig call in dumpRoles function of pg_dumpall.c
Phil Sorber writes: > Then there is a separate section of code that is called as a separate > function 'dumpUserConfig()' that does other role attributes like > 'ALTER ROLE bob SET role TO charlie'. These are the ALTER's that can > have dependencies on other roles. Right. Phrased that way, this is an obvious improvement, and I concur that it doesn't look like it could break anything that works now. The more general problem remains to be solved, but we might as well apply this. 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 EXTENSION .. ADD/DROP weirdness
I wrote: > Hmm. I'm afraid that's going to break something, because I had had it > like that originally and changed it in commit > 988620dd8c16d77f88ede167b22056176324. However, I'm not quite sure > *what* it will break, because it seems like in general extension > dependencies ought to act pretty nearly like owner dependencies. > In a quick look, this seems to be the only place where we're doing it > differently (without a clear reason) for recordDependencyOnOwner and > recordDependencyOnCurrentExtension. After studying the code a bit more, I think I was worrying about some corner cases involving shell type replacement; but they're not interesting enough to justify making the main-line cases harder to work with. So I think this is a good fix, and I applied it with some comment adjustments. 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] pl/perl example in the doc no longer works in 9.1
On Wed, Oct 12, 2011 at 15:00, Tom Lane wrote:
> "David E. Wheeler" writes:
>> On Oct 12, 2011, at 9:15 AM, Tom Lane wrote:
>>> Well, the real question is why a function declared to return VOID cares
>>> at all about what the last command in its body is. If this has changed
>>> since previous versions then I think it's a bug and we should fix it,
>>> not just change the example.
>
>> It has indeed changed, either in 9.0 or 9.1 (the latter, I think). I had to
>> add a bunch of bare return statements to existing functions.
[ For those that missed it, 9.0 is OK, it is indeed a bug in 9.1. ]
> This appears to have gotten broken in commit
> 87bb2ade2ce646083f39d5ab3e3307490211ad04, which changed the function
> return code to go through plperl_sv_to_datum, which is making
> unwarranted assumptions ... but since it's utterly bereft of comments,
> it's hard for a non Perl hacker to identify exactly what it should do
> instead.
Yeah, its a mess.
> The core of the problem seems to be that if SvROK(sv) then
> the code assumes that it must be intended to convert that to an array or
> composite, no matter whether the declared result type of the function is
> compatible with such a thing.
Hrm, well 9.0 and below did not get this "right" either:
create or replace function test_hash() returns text as $$ return
{'a'=>1}; $$ language plperl;
select test_array();
test_array
---
ARRAY(0x7fd92384dcb8)
(1 row)
create or replace function test_hash() returns text as $$ return
{'a'=>1}; $$ language plperl;
select test_hash();
test_hash
--
HASH(0x7fd92387f848)
(1 row)
9.1 does this:
select test_array();
test_array
\x01
(1 row)
select test_hash();
ERROR: type text is not composite
CONTEXT: PL/Perl function "test_hash"
> So I think this probably broke not only
> VOID-result cases, but also cases where a Perl array or hash is supposed
> to be returned as, say, text.
Given the output above (both pre 9.1 and post) it seems unless the
type is a set or composite we should throw an error. Maybe "PL/Perl
function returning type %s must not return a reference" ?
> It would be more appropriate to drive the
> cases off the nature of the function result type, perhaps.
Ill see if I can cook up something that's not too invasive.
[ I have a patch to fix the VOID issues. it gets rid of that horrid
has_retval variable/logic and makes it look much closer to 9.0's code.
Unfortunately its on my laptop at home as I was hacking on it before I
went to work... ]
--
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] ts_rank
Bruce Momjian wrote: > Mark wrote: > > >There's some potentially useful information here: > > >http://www.postgresql.org/docs/9.0/interactive/textsearch-controls.html#TEXTSEARCH-RANKING > > > > Thanks for reply. I was reading the documentation of PostgreSQL, but there > > it is not written the name of the used methods. Everywhere there is written, > > that ts_rank use standard ranking function. But it is difficult to say which > > is the standard function. > > Somewhere I found that it is maybe based on Vector space model and it seems > > to be truth, because in the code of tsrank.c is counted the frequency of > > words, but I am not sure of that :-( > > Oleg, Teodor, can you give me a description of how ts_rank decided how > to rank items? Thanks. Any news on this question? -- 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] Overhead cost of Serializable Snapshot Isolation
On Wed, Oct 12, 2011 at 1:44 PM, Florian Pflug wrote: > On Oct11, 2011, at 23:35 , Simon Riggs wrote: >> On Tue, Oct 11, 2011 at 10:30 PM, Florian Pflug wrote: >> >>> That experience has taught me that backwards compatibility, while very >>> important in a lot of cases, has the potential to do just as much harm >>> if overdone. >> >> Agreed. Does my suggestion represent overdoing it? I ask for balance, >> not an extreme. > > It's my belief that an "off" switch for true serializability is overdoing > it, yes. > > With such a switch, every application that relies on true serializability > for > correctness would be prone to silent data corruption should the switch ever > get set to "off" accidentally. > > Without such a switch, OTOH, all that will happen are a few more aborts due > to > serialization errors in application who request SERIALIZABLE when they > really > only need REPEATABLE READ. Which, in the worst case, is a performance issue, > but never an issue of correctness. That's a good argument and I accept it. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER EXTENSION .. ADD/DROP weirdness
Robert Haas writes: > On Mon, Oct 10, 2011 at 2:52 PM, Tom Lane wrote: >> The underlying issue here is whether objects dependent on an extension >> member should have direct dependencies on the extension too, and if not, >> how do we prevent that? The recordDependencyOnCurrentExtension calls >> don't have enough information to know what to do, I think. > After looking at this code, it seems that we've generally made that > the caller's problem - e.g. in heap_create_with_catalog(), we skip > recordDependencyOnCurrentExtension() if we're dealing with a composite > type. So I think the fix here is just to move the > recordDependencyOnCurrentExtension() call in pg_type.c inside the > if-block that precedes it, as in the attached patch. Hmm. I'm afraid that's going to break something, because I had had it like that originally and changed it in commit 988620dd8c16d77f88ede167b22056176324. However, I'm not quite sure *what* it will break, because it seems like in general extension dependencies ought to act pretty nearly like owner dependencies. In a quick look, this seems to be the only place where we're doing it differently (without a clear reason) for recordDependencyOnOwner and recordDependencyOnCurrentExtension. Let me poke at it a bit more. The proposed patch is a bit short on comment fixes, anyway. 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] Dumping roles improvements?
> In that case you do "pg_dumpall -r" first and then pg_dump your > individual database. I thought you were looking for something that > would dump only roles referenced in the particular database, which > is why it sounded like an intermediate case. > > I know that the division of labor between pg_dumpall and pg_dump could > use rethinking, but it needs to be actually rethought, in toto, not > hacked one minor feature at a time. Sure. Maybe I should start a wiki page for that? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] *.sql contrib files contain unresolvable MODULE_PATHNAME
Tom Lane writes: > regression=# \i ~/postgres/share/extension/cube--1.0.sql > Use "CREATE EXTENSION cube" to load this file. > regression=# Great work, thank you! -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et 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 EXTENSION .. ADD/DROP weirdness
On Mon, Oct 10, 2011 at 2:52 PM, Tom Lane wrote: > Robert Haas writes: >> But there's a bigger problem: it seems to me that we have an >> inconsistency between what happens when you create an extension from >> scratch and when you upgrade it from unpackaged. Both pg_buffercache >> and pg_stat_statements just do this in the "upgrade from unpackaged" >> case: > >> ALTER EXTENSION ADD view ; > >> They do *not* add the type and the array type. But when the "1.0" >> script is run, the type and array type end up belonging to the >> extension. This seems bad. > > Hmm, yeah, we need to make those consistent. > > The underlying issue here is whether objects dependent on an extension > member should have direct dependencies on the extension too, and if not, > how do we prevent that? The recordDependencyOnCurrentExtension calls > don't have enough information to know what to do, I think. After looking at this code, it seems that we've generally made that the caller's problem - e.g. in heap_create_with_catalog(), we skip recordDependencyOnCurrentExtension() if we're dealing with a composite type. So I think the fix here is just to move the recordDependencyOnCurrentExtension() call in pg_type.c inside the if-block that precedes it, as in the attached patch. Of course, this won't fix any damage already done, but it seems like the right thing going forward. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company extension-type-dependencies.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] pl/perl example in the doc no longer works in 9.1
"David E. Wheeler" writes: > On Oct 12, 2011, at 9:15 AM, Tom Lane wrote: >> Well, the real question is why a function declared to return VOID cares >> at all about what the last command in its body is. If this has changed >> since previous versions then I think it's a bug and we should fix it, >> not just change the example. > It has indeed changed, either in 9.0 or 9.1 (the latter, I think). I had to > add a bunch of bare return statements to existing functions. This appears to have gotten broken in commit 87bb2ade2ce646083f39d5ab3e3307490211ad04, which changed the function return code to go through plperl_sv_to_datum, which is making unwarranted assumptions ... but since it's utterly bereft of comments, it's hard for a non Perl hacker to identify exactly what it should do instead. The core of the problem seems to be that if SvROK(sv) then the code assumes that it must be intended to convert that to an array or composite, no matter whether the declared result type of the function is compatible with such a thing. So I think this probably broke not only VOID-result cases, but also cases where a Perl array or hash is supposed to be returned as, say, text. It would be more appropriate to drive the cases off the nature of the function result type, perhaps. 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] pgindent weirdness
Tom Lane wrote:
> Bruce Momjian writes:
> > Tom Lane wrote:
> >> Now having said that, there seems to be a pgindent bug here too, in that
> >> it's throwing a space into
> >>
> >> Buffer
> >> RelationGetBufferForTuple(Relation relation, Size len,
> >> Buffer otherBuffer, int options,
> >> struct BulkInsertStateData * bistate)
> >>
> >> Whether BulkInsertStateData is flagged as a typedef or not, surely it
> >> ought to understand that "struct BulkInsertStateData" is a type name.
>
> > Uh, I think we have this listed as a known bug at the top of the
> > pgindent script:
>
> Hm. I guess the third observation is that in the current state of the
> code, there's no very good reason to be using "struct" in
> RelationGetBufferForTuple's prototype anyway, since the typedef is
> declared right above it. Maybe we should just change that and not
> risk fooling with pgindent.
Changed as you suggested. I didn't see any other obvious cases.
--
Bruce Momjian http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
new file mode 100644
index 26db1e3..beecc90
*** a/src/backend/access/heap/hio.c
--- b/src/backend/access/heap/hio.c
*** GetVisibilityMapPins(Relation relation,
*** 213,219
Buffer
RelationGetBufferForTuple(Relation relation, Size len,
Buffer otherBuffer, int options,
! struct BulkInsertStateData * bistate,
Buffer *vmbuffer, Buffer *vmbuffer_other)
{
bool use_fsm = !(options & HEAP_INSERT_SKIP_FSM);
--- 213,219
Buffer
RelationGetBufferForTuple(Relation relation, Size len,
Buffer otherBuffer, int options,
! BulkInsertState bistate,
Buffer *vmbuffer, Buffer *vmbuffer_other)
{
bool use_fsm = !(options & HEAP_INSERT_SKIP_FSM);
diff --git a/src/include/access/hio.h b/src/include/access/hio.h
new file mode 100644
index 6eac97e..aefd679
*** a/src/include/access/hio.h
--- b/src/include/access/hio.h
*** extern void RelationPutHeapTuple(Relatio
*** 38,44
HeapTuple tuple);
extern Buffer RelationGetBufferForTuple(Relation relation, Size len,
Buffer otherBuffer, int options,
! struct BulkInsertStateData * bistate,
Buffer *vmbuffer, Buffer *vmbuffer_other);
#endif /* HIO_H */
--- 38,44
HeapTuple tuple);
extern Buffer RelationGetBufferForTuple(Relation relation, Size len,
Buffer otherBuffer, int options,
! BulkInsertState bistate,
Buffer *vmbuffer, Buffer *vmbuffer_other);
#endif /* HIO_H */
--
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] Formatting Curmudgeons WAS: MMAP Buffers
Andrew Dunstan wrote: > Now we could certainly make this quite a bit slicker. Apart from > anything else, we should change the indent source code tarball so it > unpacks into its own directory. Having it unpack into the current Yes, done. > directory is ugly and unfriendly. And we should get rid of the "make > clean" line in the install target of entab's makefile, which just seems > totally ill-conceived. Yes, fixed. > It might also be worth setting it up so that instead of having to pass a > path to a typedefs file on the command line, we default to a file > sitting in, say, /usr/local/etc. Then you'd just be able to say > "pgindent my_file.c". Yes, also done. -- 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] branching for 9.2devel
Alvaro Herrera wrote: > Excerpts from Tom Lane's message of lun abr 25 20:48:42 -0300 2011: > > Andrew Dunstan writes: > > > > Well, that way you'll have a handful of -Ttypdef parameters for each > > > invocation of indent instead of a gazillion of them. No more command > > > line length issues. > > > > Well, -Ttypedef is wrong on its face. Right would be a switch > > specifying the name of the file to read the typedef list from. > > Then you don't need massive script-level infrastructure to try > > to spoonfeed that data to the program doing the work. > > I gather that Andrew will be working on replacing the pgindent shell > script with a Perl script, but this new script will still rely on our > patched BSD indent, right? > > Of course, it would make sense to further patch indent so that it > accepts typedefs in a file instead of thousands of -T switches, but that > seems orthogonal. I have done as you suggested, modifying our version of BSD indent to allow a file of typedefs to be processed. I also renamed the download and binary to 'pg_bsd_indent' so it can be installed on a system that already has 'indent'. It should propogate to the ftp mirrors in a few hours. Even after we go to Perl, this is still a necessary change. I have modified the pgindent script to use this new flag, and applied those changes, attached. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + diff --git a/src/tools/pgindent/README b/src/tools/pgindent/README new file mode 100644 index e81e85d..7504650 *** a/src/tools/pgindent/README --- b/src/tools/pgindent/README *** pgindent *** 6,31 This can format all PostgreSQL *.c and *.h files, but excludes *.y, and *.l files. ! 1) Change directory to the top of the build tree. ! 2) Download the typedef file from the buildfarm: wget -O src/tools/pgindent/typedefs.list http://buildfarm.postgresql.org/cgi-bin/typedefs.pl ! 3) Remove all derived files (pgindent has trouble with one of the flex macros): gmake maintainer-clean ! 4) Run pgindent: find . -name '*.[ch]' -type f -print | \ egrep -v -f src/tools/pgindent/exclude_file_patterns | \ xargs -n100 pgindent src/tools/pgindent/typedefs.list ! 5) Remove any files that generate errors and restore their original versions. ! 6) Do a full test build: run configure # stop is only necessary if it's going to install in a location with an --- 6,33 This can format all PostgreSQL *.c and *.h files, but excludes *.y, and *.l files. ! 1) Install pg_bsd_indent (see below for details) ! 2) Change directory to the top of the build tree. ! ! 3) Download the typedef file from the buildfarm: wget -O src/tools/pgindent/typedefs.list http://buildfarm.postgresql.org/cgi-bin/typedefs.pl ! 4) Remove all derived files (pgindent has trouble with one of the flex macros): gmake maintainer-clean ! 5) Run pgindent: find . -name '*.[ch]' -type f -print | \ egrep -v -f src/tools/pgindent/exclude_file_patterns | \ xargs -n100 pgindent src/tools/pgindent/typedefs.list ! 6) Remove any files that generate errors and restore their original versions. ! 7) Do a full test build: run configure # stop is only necessary if it's going to install in a location with an *** This can format all PostgreSQL *.c and * *** 38,54 --- ! We have standardized on NetBSD's indent. We have fixed a few bugs which ! requre the NetBSD source to be patched with indent.bsd.patch patch. A ! fully patched version is available at ftp://ftp.postgresql.org/pub/dev. GNU indent, version 2.2.6, has several problems, and is not recommended. These bugs become pretty major when you are doing >500k lines of code. If you don't believe me, take a directory and make a copy. Run pgindent on the copy using GNU indent, and do a diff -r. You will see what I ! mean. GNU indent does some things better, but mangles too. ! Notes about excluded files: src/include/storage/s_lock.h is excluded because it contains assembly code that pgindent tends to mess up. --- 40,67 --- ! BSD indent ! -- ! ! We have standardized on NetBSD's indent, and renamed it pg_bsd_indent. ! We have fixed a few bugs which requre the NetBSD source to be patched ! with indent.bsd.patch patch. A fully patched version is available at ! ftp://ftp.postgresql.org/pub/dev. GNU indent, version 2.2.6, has several problems, and is not recommended. These bugs become pretty major when you are doing >500k lines of code. If you don't believe me, take a directory and make a copy. Run pgindent on the copy using GNU indent, and do a diff -r. You will see what I ! mean. GNU indent does
Re: [HACKERS] patch: move dumpUserConfig call in dumpRoles function of pg_dumpall.c
On Wed, Oct 12, 2011 at 3:48 PM, Robert Haas wrote: > On Wed, Oct 12, 2011 at 2:27 PM, Phil Sorber wrote: >> I am going to remove that patch from the commit fest because we all >> agree that it does not solve the problem satisfactorily. I would like >> to re-iterate a few points, and submit a new patch. >> >> First off, there are two distinct problems here that we have been >> lumping into one. There is the issue of the 'ALTER DATABASE SET ROLE' >> and then there is the 'ALTER ROLE SET ROLE' case. The former is the >> one that has been causing us so many problems, and the latter is the >> one that I really care about. >> >> Also, there are more people that are hitting this issue as well: >> >> http://archives.postgresql.org/pgsql-hackers/2011-02/msg02362.php >> >> My proposal would be to table the discussion about the 'ALTER DATABASE >> SET ROLE' case because there seems to be a bit of a philosophical >> debate behind this that needs to be sorted out first. >> >> If we focus only on the 'ALTER ROLE' case I think there is a trivial >> solution. We already separate the CREATE from the ALTER in a single >> loop. We also already separate out the user config in a separate >> function called from this same loop. The problem is that the user >> config can be dependent upon a later CREATE. So all I suggest is to >> move the user config dumping into a new loop afterward so that the >> user config ALTER's come after all the other CREATE's and ALTER's. It >> amounts to a 7 line change and solves our problem rather elegantly. > > I'm not sure I completely follow this explanation of the problem, but > it does seem better to me to set all the role attributes after dumping > all the create role statements, and I don't see how that can break > anything that works now. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > Just to be clear, this doesn't move all the ALTER's. It will still do CREATE/ALTER pair's for attributes that could go in the CREATE. Example: CREATE ROLE bob; ALTER ROLE bob WITH LOGIN PASSWORD 'blah'; You could turn those in to one statement, but for various reasons you do not want to. None of these have any dependencies other than the actual creation of the role. Then there is a separate section of code that is called as a separate function 'dumpUserConfig()' that does other role attributes like 'ALTER ROLE bob SET role TO charlie'. These are the ALTER's that can have dependencies on other roles. I agree this is a little confusing, and if you prefer to see all the ALTER's in one section together directly after the CREATE statements I can make the patch do that, but it isn't necessary to solve the problem. -- 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] Dumping roles improvements?
Josh Berkus writes: > On 10/11/11 9:43 PM, Tom Lane wrote: >> I don't find this terribly convincing. I can see the rationales for two >> endpoint cases: (1) restore these objects into exactly the same >> ownership/permissions environment that existed before, and (2) restore >> these objects with the absolute minimum of ownership/permissions >> assumptions. The latter case seems to me to be covered already by >> --no-owner --no-privileges. > But what I'm asking for is (1). The problem is that the roles don't > ship in the per-database pgdump file. In that case you do "pg_dumpall -r" first and then pg_dump your individual database. I thought you were looking for something that would dump only roles referenced in the particular database, which is why it sounded like an intermediate case. I know that the division of labor between pg_dumpall and pg_dump could use rethinking, but it needs to be actually rethought, in toto, not hacked one minor feature at a time. 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] patch: move dumpUserConfig call in dumpRoles function of pg_dumpall.c
On Wed, Oct 12, 2011 at 2:27 PM, Phil Sorber wrote: > I am going to remove that patch from the commit fest because we all > agree that it does not solve the problem satisfactorily. I would like > to re-iterate a few points, and submit a new patch. > > First off, there are two distinct problems here that we have been > lumping into one. There is the issue of the 'ALTER DATABASE SET ROLE' > and then there is the 'ALTER ROLE SET ROLE' case. The former is the > one that has been causing us so many problems, and the latter is the > one that I really care about. > > Also, there are more people that are hitting this issue as well: > > http://archives.postgresql.org/pgsql-hackers/2011-02/msg02362.php > > My proposal would be to table the discussion about the 'ALTER DATABASE > SET ROLE' case because there seems to be a bit of a philosophical > debate behind this that needs to be sorted out first. > > If we focus only on the 'ALTER ROLE' case I think there is a trivial > solution. We already separate the CREATE from the ALTER in a single > loop. We also already separate out the user config in a separate > function called from this same loop. The problem is that the user > config can be dependent upon a later CREATE. So all I suggest is to > move the user config dumping into a new loop afterward so that the > user config ALTER's come after all the other CREATE's and ALTER's. It > amounts to a 7 line change and solves our problem rather elegantly. I'm not sure I completely follow this explanation of the problem, but it does seem better to me to set all the role attributes after dumping all the create role statements, and I don't see how that can break anything that works now. -- 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] Dumping roles improvements?
On 10/12/2011 03:16 PM, Josh Berkus wrote: On 10/11/11 9:43 PM, Tom Lane wrote: I don't find this terribly convincing. I can see the rationales for two endpoint cases: (1) restore these objects into exactly the same ownership/permissions environment that existed before, and (2) restore these objects with the absolute minimum of ownership/permissions assumptions. The latter case seems to me to be covered already by --no-owner --no-privileges. But what I'm asking for is (1). The problem is that the roles don't ship in the per-database pgdump file. I think Tom's (1) assumes you already have that environment, not that it will be created on the fly by pg_restore. 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] Dumping roles improvements?
On 10/12/2011 12:43 AM, Tom Lane wrote: Josh Berkus writes: The reason I want to have the dependant roles created as part of a database dump is so that we can ship around dump files as a single file, and restore them with a single command. This is considerably simpler than the current requirements, which are: 1. pg_dumpall the roles 2. pg_dump the database 3. tar both files 4. ship file 5. untar both files 6. psql the role file 7. pg_restore the database file I don't find this terribly convincing. I can see the rationales for two endpoint cases: (1) restore these objects into exactly the same ownership/permissions environment that existed before, and (2) restore these objects with the absolute minimum of ownership/permissions assumptions. The latter case seems to me to be covered already by --no-owner --no-privileges. Cases in between those endpoints seem pretty special-purpose, and I don't want to buy into the assumption that we should fix them by creating a plethora of --do-it-joshs-way switches. Can we invent something comparable to the --list/--use-list mechanism, that can handle a range of use cases with a bit more manual effort? Not easily, that I can think of. The cleanest way I can imagine would be to have explicit ROLE objects in the TOC. TWe can easily get a list of object owners and turn that into a set of "create role" statements, because owner names are in the metadata, but getting a list of roles mentioned in ACL items can only be done by textually analysing them - the information just isn't kept anywhere else currently. I do think there's a case for doing "create if not exists role foo" (I know we don't have that right now) for owners and roles mentioned in ACLs. The hair in the ointment here comes when we consider how far to go with that. In particular, would we follow role membership recursively? OTOH, notwithstanding Josh's reasonable need, I'm not sure the ROI here is high enough. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Object access hooks with arguments support (v1)
On Thu, Sep 29, 2011 at 4:52 PM, Kohei KaiGai wrote: > I noticed that the previous revision does not provide any way to inform > the modules name of foreign server, even if foreign table was created, > on the OAT_POST_CREATE hook. > So, I modified the invocation at heap_create_with_catalog to deliver > this information to the modules. > > Rest of parts were unchanged, except for rebasing to the latest git > master. I've never really been totally sanguine with the idea of making object access hooks take arguments, and all of my general concerns seem to apply to the way you've set this patch up. In particular: 1. Type safety goes out the window. What you're essentially proposing here is that we should have one hook function can be used for almost anything at all and can be getting up to 9 arguments of any type whatsoever. The hook will need to know how to interpret all of its arguments depending on the context in which it was called. The compiler will be totally unable to do any static type-checking, so there will be absolutely no warning if the interface is changed incompatibly on either side. Instead, you'll probably just crash the database server at runtime. 2. The particular choice of data being passed to the object access hooks appears capricious and arbitrary. Here's an example: /* Post creation hook for new relation */ - InvokeObjectAccessHook(OAT_POST_CREATE, RelationRelationId, relid, 0); + InvokeObjectAccessHook4(OAT_POST_CREATE, + RelationRelationId, relid, 0, + PointerGetDatum(new_rel_tup), + PointerGetDatum(tupdesc), + BoolGetDatum(is_select_into), + CStringGetDatum(fdw_srvname)); Now, I am sure that you have good reasons for wanting to pass those particular things to the object access hook rather than any other local variable or argument that might happen to be lying around at this point in the code, but they are not documented. If someone adds a new argument to this function, or removes an argument that's being passed, they will have no idea what to do about this. Moreover, if you did document it, I think it would boil down to "this is what sepgsql happens to need", and I don't think that's an acceptable answer. We have repeatedly refused to adopt that approach in the past. (This particular case is worse than average, because new_rel_tup is coming from InsertPgClassTuple, which therefore has to be modified, along with AddNewRelationTuple and index_create, to get the tuple back up to the call site where, apparently, it is needed.) I am not exactly sure what the right way to solve this problem is, but I don't think this is 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] Dumping roles improvements?
On 10/11/11 9:43 PM, Tom Lane wrote: > I don't find this terribly convincing. I can see the rationales for two > endpoint cases: (1) restore these objects into exactly the same > ownership/permissions environment that existed before, and (2) restore > these objects with the absolute minimum of ownership/permissions > assumptions. The latter case seems to me to be covered already by > --no-owner --no-privileges. But what I'm asking for is (1). The problem is that the roles don't ship in the per-database pgdump file. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch: move dumpUserConfig call in dumpRoles function of pg_dumpall.c
On Mon, Oct 10, 2011 at 11:54 AM, Robert Haas wrote: > On Thu, Aug 4, 2011 at 2:04 PM, Robert Haas wrote: >> On Thu, Aug 4, 2011 at 1:53 PM, Phil Sorber wrote: >>> Ok, here is the patch that just moves the ALTER/SET pieces to the end. >>> Can we get this included in the next commit fest? >> >> Yep, just make yourself an account and add it. > > Unfortunately, it doesn't look like anyone ever replied to this > thread, but Tom posted some thoughts on another thread that seem to me > to be a serious problem for this patch: > > http://archives.postgresql.org/message-id/13764.1315094...@sss.pgh.pa.us > > I don't see any easy way around that problem, so I'm going to mark > this patch Returned with Feedback for now. It strikes me as craziness > to try to guess which settings we should restore at the beginning and > which at the end, so I think we need a better idea. I don't really > understand why it's not OK to just have pg_dump issue RESET ROLE at > appropriate points in the process; that seems like it would be > sufficient and not particularly ugly. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > I am going to remove that patch from the commit fest because we all agree that it does not solve the problem satisfactorily. I would like to re-iterate a few points, and submit a new patch. First off, there are two distinct problems here that we have been lumping into one. There is the issue of the 'ALTER DATABASE SET ROLE' and then there is the 'ALTER ROLE SET ROLE' case. The former is the one that has been causing us so many problems, and the latter is the one that I really care about. Also, there are more people that are hitting this issue as well: http://archives.postgresql.org/pgsql-hackers/2011-02/msg02362.php My proposal would be to table the discussion about the 'ALTER DATABASE SET ROLE' case because there seems to be a bit of a philosophical debate behind this that needs to be sorted out first. If we focus only on the 'ALTER ROLE' case I think there is a trivial solution. We already separate the CREATE from the ALTER in a single loop. We also already separate out the user config in a separate function called from this same loop. The problem is that the user config can be dependent upon a later CREATE. So all I suggest is to move the user config dumping into a new loop afterward so that the user config ALTER's come after all the other CREATE's and ALTER's. It amounts to a 7 line change and solves our problem rather elegantly. diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c new file mode 100644 index b5f64e8..ee597d5 *** a/src/bin/pg_dump/pg_dumpall.c --- b/src/bin/pg_dump/pg_dumpall.c *** dumpRoles(PGconn *conn) *** 804,814 buf, "ROLE", rolename); fprintf(OPF, "%s", buf->data); - - if (server_version >= 70300) - dumpUserConfig(conn, rolename); } PQclear(res); fprintf(OPF, "\n\n"); --- 804,815 buf, "ROLE", rolename); fprintf(OPF, "%s", buf->data); } + if (server_version >= 70300) + for (i = 0; i < PQntuples(res); i++) + dumpUserConfig(conn, PQgetvalue(res, i, i_rolname)); + PQclear(res); fprintf(OPF, "\n\n"); -- 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] [BUGS] *.sql contrib files contain unresolvable MODULE_PATHNAME
Andrew Dunstan writes: > On 10/12/2011 02:21 PM, Magnus Hagander wrote: >> I guess the failure scenario is if someone has an extension from 9.1.2 >> and tries to load it into 9.1.1 or earlier, in which case they will >> get a syntax error or somehing when trying to run the CREATE EXTENSION >> command, right? I doubt that's something worth dealing with - it's a >> lot less likely to happen. > As long as we are going to apply it for 9.1 and not wait for 9.2 I don't > think there will be much problem. I think this is one of the rare cases > where we should apply a change to the stable release. By 9.2 doing this would be rather pointless, likely. Also, the earlier we get it in the easier it will be for third-party devs to rely on it working. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_comments (was: Allow \dd to show constraint comments)
On Sun, Sep 11, 2011 at 10:11 AM, Josh Kupershmidt wrote: > On Sat, Sep 10, 2011 at 7:47 PM, Thom Brown wrote: >> Just tested this out on current master. I tried this on every object >> capable of having a comment, and the view reports all of them with the >> correct details. Doc changes look fine, except for some reason you >> removed a full-stop (period) from after "For all other object types, >> this column is zero." > > Thanks for the review. Looks like I got confused about where I was > whacking around in catalogs.sgml, good catch of a spurious change. > Fixed patch attached. So, I think the critical question for this patch is "do we want this?". Tom didn't like it, and I have to admit I'm somewhat demoralized by the discovery that we can't make effective use of this in psql. On the flip side, rooting through pg_description and pg_shdescription with home-grown queries is un-fun, and it's not clear that \dd solves the problem well enough that we don't need anything else. On the third hand, Josh's previous batch of changes to clean up psql's behavior in this area are clearly a huge improvement: you can now display the comment for nearly anything by running the appropriate \d command for whatever the object type is. So ... is this still a good idea, or should we just forget about 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] [BUGS] *.sql contrib files contain unresolvable MODULE_PATHNAME
On 10/12/2011 02:21 PM, Magnus Hagander wrote: On Wed, Oct 12, 2011 at 19:36, Tom Lane wrote: regression=# \i ~/postgres/share/extension/cube--1.0.sql Use "CREATE EXTENSION cube" to load this file. regression=# which is about as good as one could hope for. Looks great to me. Yes, me too. I guess the failure scenario is if someone has an extension from 9.1.2 and tries to load it into 9.1.1 or earlier, in which case they will get a syntax error or somehing when trying to run the CREATE EXTENSION command, right? I doubt that's something worth dealing with - it's a lot less likely to happen. As long as we are going to apply it for 9.1 and not wait for 9.2 I don't think there will be much problem. I think this is one of the rare cases where we should apply a change to the stable release. 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] COUNT(*) and index-only scans
On Wed, Oct 12, 2011 at 03:16:54PM +0100, Greg Stark wrote: > On Wed, Oct 12, 2011 at 2:52 PM, Tom Lane wrote: > > I think it's overkill, and possibly unpleasantly unstable as well. > > For the initial attack on this we should just have VACUUM and ANALYZE > > count the number of all-visible blocks and store that in pg_class along > > with the tuple-count statistics. There's no reason to think that this > > will be any worse than the way we deal with dead tuple counts, for > > instance. > > So I have a theory. > > Assuming you're in a steady-state situation the amount of all-visible > blocks will fluctuate from a high just after vacuum to a low just > before the next vacuum. There are other ways a block can be marked > all-visible but for the most part I would expect the fraction to go > steadily down until vacuum comes along and cleans things up. > > So if vacuum tracked the fraction of blocks marked all-visible > *before* it processed them and the fraction it marked all-visible > after processing we have an upper and lower bound. If we knew how long > it's been since vacuum we could interpolate between those, or we could > just take the mean, or we could take the lower bound as a conservative > estimate. > > > What I suggest as a first cut for that is: simply derate the visibility > > fraction as the fraction > > of the table expected to be scanned gets smaller. > > I think there's a statistically more rigorous way of accomplishing the > same thing. If you treat the pages we estimate we're going to read as > a random sample of the population of pages then your expected value is > the fraction of the overall population that is all-visible but your > 95th percentile confidence interval will be, uh, a simple formula we > can compute but I don't recall off-hand. Incidentally, I had a similar idea at PGCon relating to planning... My idea was to compute not just the cost but the sensitivity of the cost an estimate for each plan. The sensitivity is the derivate of the cost. So, if the total cost was n^2 the sensitivity would be 2n. If you picked a tolerance (like 2 standard deviations) of the observed distribution. You could compare the expected cost and the expected 'unlucky cost' for plans. (Basically, this is parametric error propagation) I know very little about the planner... I don't know how how often it would lead to picking a better plan (it might not be worth the cost to compute), but it seemed like an interesting approach to me. Garick -- 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] [BUGS] *.sql contrib files contain unresolvable MODULE_PATHNAME
Magnus Hagander writes: > On Wed, Oct 12, 2011 at 19:36, Tom Lane wrote: >> PFA, a sample patch for this --- I've only bothered to change one script >> file here, but will of course do the rest if there are no further >> objections. > I guess the failure scenario is if someone has an extension from 9.1.2 > and tries to load it into 9.1.1 or earlier, in which case they will > get a syntax error or somehing when trying to run the CREATE EXTENSION > command, right? I doubt that's something worth dealing with - it's a > lot less likely to happen. Hmm, yeah, you're right. But it doesn't seem like a big problem to me, certainly not as big as the problem we're trying to solve. > We might want to document this for other third-party extension > developers to use as a trick as well? Yes, I will add something to the docs. 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] [BUGS] *.sql contrib files contain unresolvable MODULE_PATHNAME
On Wed, Oct 12, 2011 at 19:36, Tom Lane wrote: > Magnus Hagander writes: >> On Wed, Oct 12, 2011 at 17:40, Tom Lane wrote: >>> The only thing the \echo approach will cost us is a few more lines of C >>> code in execute_extension_script(), and I think it's more than worth >>> that given the evident scope of the problem. > >> +1. > > PFA, a sample patch for this --- I've only bothered to change one script > file here, but will of course do the rest if there are no further > objections. The technique actually works even better than I expected, > because of the seemingly nowhere documented fact that \quit in a script > file doesn't terminate psql, only processing of the script. So what > I get is > > regression=# \i ~/postgres/share/extension/cube--1.0.sql > Use "CREATE EXTENSION cube" to load this file. > regression=# > > which is about as good as one could hope for. Looks great to me. I guess the failure scenario is if someone has an extension from 9.1.2 and tries to load it into 9.1.1 or earlier, in which case they will get a syntax error or somehing when trying to run the CREATE EXTENSION command, right? I doubt that's something worth dealing with - it's a lot less likely to happen. We might want to document this for other third-party extension developers to use as a trick as well? -- 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] pg_ctl restart - behaviour based on wrong instance
Magnus Hagander wrote: > > I looked over this issue and I don't thinking having pg_ctl restart fall > > back to 'start' is a good solution. ?I am concerned about cases where we > > start a different server without shutting down the old server, for some > > reason. ?When they say 'restart', I think we have to assume they want a > > restart. > > > > What I did do was to document that not backing up postmaster.pid and > > postmaster.opts might help prevent pg_ctl from getting confused. > > Should we exclude postmaster.opts from streaming base backups? We > already exclude postmaster.pid... Uh, I think so, unless my analysis was wrong. -- 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] [BUGS] *.sql contrib files contain unresolvable MODULE_PATHNAME
Magnus Hagander writes:
> On Wed, Oct 12, 2011 at 17:40, Tom Lane wrote:
>> The only thing the \echo approach will cost us is a few more lines of C
>> code in execute_extension_script(), and I think it's more than worth
>> that given the evident scope of the problem.
> +1.
PFA, a sample patch for this --- I've only bothered to change one script
file here, but will of course do the rest if there are no further
objections. The technique actually works even better than I expected,
because of the seemingly nowhere documented fact that \quit in a script
file doesn't terminate psql, only processing of the script. So what
I get is
regression=# \i ~/postgres/share/extension/cube--1.0.sql
Use "CREATE EXTENSION cube" to load this file.
regression=#
which is about as good as one could hope for.
(Looks like a patch to the psql docs is upcoming, too.)
regards, tom lane
diff --git a/contrib/cube/cube--1.0.sql b/contrib/cube/cube--1.0.sql
index ee9febe005315bf13e93a9ef216a7411fc13a445..0d259c0969d9df4a00f160bf1fc00407273e2b1d 100644
*** a/contrib/cube/cube--1.0.sql
--- b/contrib/cube/cube--1.0.sql
***
*** 1,5
--- 1,7
/* contrib/cube/cube--1.0.sql */
+ \echo Use "CREATE EXTENSION cube" to load this file. \quit
+
-- Create the user-defined type for N-dimensional boxes
CREATE FUNCTION cube_in(cstring)
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 3af15dd38bb7044ec6f733787cad6897b204ccd3..ba1e2c45cd97c89265912d338a98f52bf48ea4b0 100644
*** a/src/backend/commands/extension.c
--- b/src/backend/commands/extension.c
***
*** 33,38
--- 33,39
#include "catalog/indexing.h"
#include "catalog/namespace.h"
#include "catalog/objectaccess.h"
+ #include "catalog/pg_collation.h"
#include "catalog/pg_depend.h"
#include "catalog/pg_extension.h"
#include "catalog/pg_namespace.h"
*** execute_extension_script(Oid extensionOi
*** 855,880
CurrentExtensionObject = extensionOid;
PG_TRY();
{
! char *sql = read_extension_script_file(control, filename);
/*
* If it's not relocatable, substitute the target schema name for
* occcurrences of @extschema@.
*
! * For a relocatable extension, we just run the script as-is. There
! * cannot be any need for @extschema@, else it wouldn't be
! * relocatable.
*/
if (!control->relocatable)
{
const char *qSchemaName = quote_identifier(schemaName);
! sql = text_to_cstring(
! DatumGetTextPP(
! DirectFunctionCall3(replace_text,
! CStringGetTextDatum(sql),
! CStringGetTextDatum("@extschema@"),
! CStringGetTextDatum(qSchemaName;
}
/*
--- 856,894
CurrentExtensionObject = extensionOid;
PG_TRY();
{
! char *c_sql = read_extension_script_file(control, filename);
! Datum t_sql;
!
! /* We use various functions that want to operate on text datums */
! t_sql = CStringGetTextDatum(c_sql);
!
! /*
! * Reduce any lines beginning with "\echo" to empty. This allows
! * scripts to contain messages telling people not to run them via
! * psql, which has been found to be necessary due to old habits.
! */
! t_sql = DirectFunctionCall4Coll(textregexreplace,
! C_COLLATION_OID,
! t_sql,
! CStringGetTextDatum("^echo.*$"),
! CStringGetTextDatum(""),
! CStringGetTextDatum("ng"));
/*
* If it's not relocatable, substitute the target schema name for
* occcurrences of @extschema@.
*
! * For a relocatable extension, we needn't do this. There cannot be
! * any need for @extschema@, else it wouldn't be relocatable.
*/
if (!control->relocatable)
{
const char *qSchemaName = quote_identifier(schemaName);
! t_sql = DirectFunctionCall3(replace_text,
! t_sql,
! CStringGetTextDatum("@extschema@"),
! CStringGetTextDatum(qSchemaName));
}
/*
*** execute_extension_script(Oid extensionOi
*** 883,897
*/
if (control->module_pathname)
{
! sql = text_to_cstring(
! DatumGetTextPP(
! DirectFunctionCall3(replace_text,
! CStringGetTextDatum(sql),
! CStringGetTextDatum("MODULE_PATHNAME"),
! CStringGetTextDatum(control->module_pathname;
}
! execute_sql_string(sql, filename);
}
PG_CATCH();
{
--- 897,912
*/
if (control->module_pathname)
{
! t_sql = DirectFunctionCall3(replace_text,
! t_sql,
! CStringGetTextDatum("MODULE_PATHNAME"),
! CStringGetTextDatum(control->module_pathname));
}
! /* And now back to C string */
! c_sql = text_to_cstring(DatumGetTextPP(t_sql));
!
! execute_sql_string(c_sql, filename);
}
PG_CATCH();
{
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your
Re: [HACKERS] pg_ctl restart - behaviour based on wrong instance
On Tue, Oct 11, 2011 at 23:35, Bruce Momjian wrote: > Robert Haas wrote: >> On Wed, Mar 23, 2011 at 1:48 AM, Fujii Masao wrote: >> > On Sat, Mar 19, 2011 at 10:20 AM, Robert Haas >> > wrote: >> >> On Fri, Mar 18, 2011 at 1:19 PM, Erik Rijkers wrote: >> >>> This is OK and expected. ?But then it continues (in the logfile) with: >> >>> >> >>> FATAL: ?lock file "postmaster.pid" already exists >> >>> HINT: ?Is another postmaster (PID 20519) running in data directory >> >>> "/var/data1/pg_stuff/pg_installations/pgsql.vanilla_1/data"? >> >>> >> >>> So, complaints about the *other* instance. ?It doesn't happen once a >> >>> successful start (with pg_ctl >> >>> start) has happened. >> >> >> >> I'm guessing that leftover postmaster.pid contents might be >> >> responsible for this? >> > >> > The cause is that "pg_ctl restart" uses the postmaster.opts which was >> > created in the primary. Since its content was something like >> > "pg_ctl -D vanilla_1/data", vanilla_1/data/postmaster.pid was checked >> > wrongly. >> > >> > The simple workaround is to exclude postmaster.opts from the backup >> > as well as postmaster.pid. But when postmaster.opts doesn't exist, >> > "pg_ctl restart" cannot start up the server. We might also need to change >> > the code of "pg_ctl restart" so that it does just "pg_ctl start" when >> > postmaster.opts doesn't exist. >> >> Sounds reasonable. > > I looked over this issue and I don't thinking having pg_ctl restart fall > back to 'start' is a good solution. I am concerned about cases where we > start a different server without shutting down the old server, for some > reason. When they say 'restart', I think we have to assume they want a > restart. > > What I did do was to document that not backing up postmaster.pid and > postmaster.opts might help prevent pg_ctl from getting confused. Should we exclude postmaster.opts from streaming base backups? We already exclude postmaster.pid... -- 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] index-only scans
I wrote: >> I was also toying with the notion of pushing the slot fill-in into the >> AM, so that the AM API is to return a filled TupleSlot not an >> IndexTuple. This would not save any cycles AFAICT --- at least in >> btree, we still have to make a palloc'd copy of the IndexTuple so that >> we can release lock on the index page. The point of it would be to >> avoid the assumption that the index's internal storage has exactly the >> format of IndexTuple. Don't know whether we'd ever have any actual use >> for that flexibility, but it seems like it wouldn't cost much to >> preserve the option. > BTW, I concluded that that would be a bad idea, because it would involve > the index AM in some choices that we're likely to want to change. In > particular it would foreclose ever doing anything with expression > indexes, without an AM API change. Better to just define the AM's > responsibility as to hand back a tuple defined according to the index's > columns. Although this aspect of the code is now working well enough for btree, I realized that it's going to have a problem if/when we add GiST support. The difficulty is that the index rowtype includes "storage" datatypes, not underlying-heap-column datatypes, for opclasses where those are different. This is not going to do for cases where we need to reconstruct a heap value from the index contents, as in Alexander's example of gist point_ops using a box as the underlying storage. What we actually want back from the index AM is a rowtype that matches the list of values submitted for indexing (ie, the original output of FormIndexDatum), and only for btree is it the case that that's represented more or less exactly as the IndexTuple stored in the index. So what I'm now thinking is to go back to the idea of having the index AM fill in a TupleTableSlot. For btree this would just amount to moving the existing StoreIndexTuple function into the AM. But it would give GiST the opportunity to do some computation, and it would avoid the problem of the index's rowtype not being a suitable intermediate format. The objection I voiced above is misguided, because it confuses the set of column types that's needed with the format distinction between a Slot and an IndexTuple. BTW, right at the moment I'm not that excited about actually doing any work on GiST itself for index-only scans. Given the current list of available opclasses there don't seem to be very many for which index-only scans would be possible, so the amount of work needed seems rather out of proportion to the benefit. But I don't mind fixing AM API details that are needed to make this workable. I'd rather have the API as right as possible in the first release. 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] [BUGS] *.sql contrib files contain unresolvable MODULE_PATHNAME
On Wed, Oct 12, 2011 at 17:40, Tom Lane wrote: > Heikki Linnakangas writes: >> On 12.10.2011 18:20, Tom Lane wrote: >>> Well, it can't be a comment, but what about a real psql command? >>> See my suggestion of using \echo. > >> Frankly I think a comment is sufficient. We can make it more complicated >> later if people are still confused. > > The thing is that this will be the third time we've gone back to try to > make it more apparent that you should use CREATE EXTENSION, and I no > longer believe that mere documentation is really going to get the job > done. Putting in a comment will only stop the bug reports from people > who bother to examine the script contents before filing a report, but > the kind of folks who don't read the release notes probably won't do > that either. In fact, if we just put in a comment, I confidently > predict we'll be coming back to revisit this issue again in future. That's exactly my concern - I strongly doubt those not bothering to read that even for a major release, aren't going to review the source of the SQL scrpit either. > The only thing the \echo approach will cost us is a few more lines of C > code in execute_extension_script(), and I think it's more than worth > that given the evident scope of the problem. +1. -- 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] COUNT(*) and index-only scans
On Wed, Oct 12, 2011 at 11:59 AM, Tom Lane wrote: > Robert Haas writes: >> I'm not concerned about an index scan vs. a sequential scan here. I'm >> concerned about it being impossible for the DBA to get an index-only >> scan when s/he wants it very badly. The current (stupid) formula >> handles this case just about perfectly - it will prefer a smaller >> index over a larger one, except when a covering index is available, in >> which case it will prefer the smallest covering index. That sounds >> exactly right to me. We get that behavior because the 10% of heap >> fetches that we're assuming we'll get to skip is larger than the >> penalty for using a bigger index. If we take out 10% and replace it >> by all_visible_percentage * fraction_of_tuples_fetched, then that 10% >> is going to drop to some infinitesmally small value on single row >> fetches from giant tables. But that's exactly one of the cases for >> which people want index-only scans in the first place. It's no better >> to be overly pessimistic here than it is to be overly optimistic. If >> the table is 90% all-visible, the probability of our finding an >> all-visible row is probably not 90%. But it's probably not 0.01% or >> 0.0001% either. > > I think you're overstating the size of the problem. Given that fact > pattern, the thing will choose an indexscan anyway, because it's the > cheapest alternative even under traditional costing rules. And it will > choose to use an index-only scan if the index is covering. It doesn't > matter what the exact cost estimate is. Maybe I'm not being clear. The case I'm worried about is when you have a table with an id column (an integer) and a name column (text). You have a unique index on (id), and an index on (id, name). You then do SELECT name FROM foo WHERE id = ?. I understand it's going to pick *an* index-scan, but which index is it going to pick? The unique index, because it's smaller, or the other one, because it's covering? I think if the table is large your proposal will lead to ignoring the covering index in favor of the smaller one, and I submit that's not what we want, because, on the average, the index-only approach is going to succeed often enough to > The place where the decision is actually somewhat hard, IMO, is where > you're pulling a small part of the table but significantly more than one > row, and the traditional best choice would be a bitmap scan. Now we > have to guess whether possibly avoiding heap fetches is better than > batching the fetches, and it doesn't seem open-and-shut to me. Yes, I agree. I was actually wondering if there is some way we could make index-only scans work for bitmap index scans. Something like this: If the index is covering, then as we retrieve each tuple, we check whether the page is all-visible. If so, we return the data from the index tuple. If not, we save the TID for later. Then, when we get done scanning the index, we go back and fetch all the pages containing saved TIDs in ascending block number order. The trouble is that I think you could get in some trouble if you use a TID bitmap here, because if you lossify the bitmap then you have to make sure you can't return a tuple that you already handled with the index-only approach (imagine that the visibility map bit gets cleared partway through the scan). All in all this seems pretty complicated... > But having said that, I see some credibility in Aidan's suggestion that > pages that actually have to be fetched from disk are disproportionately > likely to be all-visible. That would counteract the history-table > effect of correlation between current reads and recent changes, > probably not perfectly, but to a considerable extent. So right at the > moment I'm inclined to just apply the most-recently-measured visibility > fraction at face value. We shouldn't complicate matters more than that > until we have more field experience to tell us what really happens. Fetches from disk aren't the only problem; thrashing shared_buffers is pretty expensive, too. But it's an interesting point. I guess we could give it a try and see what happens. -- 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] pl/perl example in the doc no longer works in 9.1
On Oct 12, 2011, at 9:15 AM, Tom Lane wrote: > Well, the real question is why a function declared to return VOID cares > at all about what the last command in its body is. If this has changed > since previous versions then I think it's a bug and we should fix it, > not just change the example. It has indeed changed, either in 9.0 or 9.1 (the latter, I think). I had to add a bunch of bare return statements to existing functions. Best, David -- 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] pl/perl example in the doc no longer works in 9.1
"David E. Wheeler"
writes:
> On Oct 12, 2011, at 2:16 AM, Amit Khandekar wrote:
>> CREATE OR REPLACE FUNCTION myfuncs() RETURNS void AS $$
>> $_SHARED{myquote} = sub {
>> my $arg = shift;
>> $arg =~ s/(['\\])/\\$1/g;
>> return "'$arg'";
>> };
>> $$ LANGUAGE plperl;
>>
>> SELECT myfuncs(); /* initializes the function */
>>
>> ERROR: PL/Perl function must return reference to hash or array
>> CONTEXT: PL/Perl function "myfuncs"
>>
>> Not sure if this is now an expected behaviour. Is it? Accordingly, I
>> can open this in pgsql-bugs or put this issue in pgsql-docs.
> Seems like there should be a bar return at the end of the function, otherwise
> it returns the last expression, which happens to be a code reference. Not
> very useful in a function that should return VOID. New version:
Well, the real question is why a function declared to return VOID cares
at all about what the last command in its body is. If this has changed
since previous versions then I think it's a bug and we should fix it,
not just change the example.
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] pl/perl example in the doc no longer works in 9.1
On Oct 12, 2011, at 2:16 AM, Amit Khandekar wrote:
> CREATE OR REPLACE FUNCTION myfuncs() RETURNS void AS $$
>$_SHARED{myquote} = sub {
>my $arg = shift;
>$arg =~ s/(['\\])/\\$1/g;
>return "'$arg'";
>};
> $$ LANGUAGE plperl;
>
> SELECT myfuncs(); /* initializes the function */
>
> ERROR: PL/Perl function must return reference to hash or array
> CONTEXT: PL/Perl function "myfuncs"
>
> Not sure if this is now an expected behaviour. Is it? Accordingly, I
> can open this in pgsql-bugs or put this issue in pgsql-docs.
Seems like there should be a bar return at the end of the function, otherwise
it returns the last expression, which happens to be a code reference. Not very
useful in a function that should return VOID. New version:
CREATE OR REPLACE FUNCTION myfuncs() RETURNS void AS $$
$_SHARED{myquote} = sub {
my $arg = shift;
$arg =~ s/(['\\])/\\$1/g;
return "'$arg'";
};
return;
$$ LANGUAGE plperl;
Best,
David
--
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] COUNT(*) and index-only scans
Robert Haas writes: > I'm not concerned about an index scan vs. a sequential scan here. I'm > concerned about it being impossible for the DBA to get an index-only > scan when s/he wants it very badly. The current (stupid) formula > handles this case just about perfectly - it will prefer a smaller > index over a larger one, except when a covering index is available, in > which case it will prefer the smallest covering index. That sounds > exactly right to me. We get that behavior because the 10% of heap > fetches that we're assuming we'll get to skip is larger than the > penalty for using a bigger index. If we take out 10% and replace it > by all_visible_percentage * fraction_of_tuples_fetched, then that 10% > is going to drop to some infinitesmally small value on single row > fetches from giant tables. But that's exactly one of the cases for > which people want index-only scans in the first place. It's no better > to be overly pessimistic here than it is to be overly optimistic. If > the table is 90% all-visible, the probability of our finding an > all-visible row is probably not 90%. But it's probably not 0.01% or > 0.0001% either. I think you're overstating the size of the problem. Given that fact pattern, the thing will choose an indexscan anyway, because it's the cheapest alternative even under traditional costing rules. And it will choose to use an index-only scan if the index is covering. It doesn't matter what the exact cost estimate is. The place where the decision is actually somewhat hard, IMO, is where you're pulling a small part of the table but significantly more than one row, and the traditional best choice would be a bitmap scan. Now we have to guess whether possibly avoiding heap fetches is better than batching the fetches, and it doesn't seem open-and-shut to me. But having said that, I see some credibility in Aidan's suggestion that pages that actually have to be fetched from disk are disproportionately likely to be all-visible. That would counteract the history-table effect of correlation between current reads and recent changes, probably not perfectly, but to a considerable extent. So right at the moment I'm inclined to just apply the most-recently-measured visibility fraction at face value. We shouldn't complicate matters more than that until we have more field experience to tell us what really happens. 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] COUNT(*) and index-only scans
On Wed, Oct 12, 2011 at 10:37 AM, Tom Lane wrote: > Robert Haas writes: >> On Wed, Oct 12, 2011 at 9:52 AM, Tom Lane wrote: >>> What bothers me considerably more is the issue about how specific >>> queries might see an all-visible fraction that's very substantially >>> different from the table's overall ratio, > >> - Suppose VACUUM processes the table and makes it all-visible. Then, >> somebody comes along and updates one tuple on every page, making them >> all not-all-visible, but not trigger VACUUM because we're nowhere >> close the 20% threshold. Now COUNT(*) will think it should use an >> index-scan, but really... not so much. In fact, even if it's only >> that a tuple has been updated on 25% of the pages, we're probably in >> trouble. > > Yeah, but that would be a pretty unlucky pattern, and in any case the > fix for it is going to be to make autovacuum more aggressive. Hmm, maybe. >> - Suppose the table has a million rows and we're going to read 100 of >> them, or 0.01%. Now it might appear that a covering index has a >> negligible advantage over a non-covering index, but in fact I think we >> still want to err on the side of trying to use the covering index. > > Given that fact pattern we still will, I think. We'll still prefer an > indexscan over a seqscan, for sure. In any case, if you believe the > assumption that those 100 rows are more likely to be recently-dirtied > than the average row, I'm not sure why you think we should be trying to > force an assumption that index-only will succeed here. I'm not concerned about an index scan vs. a sequential scan here. I'm concerned about it being impossible for the DBA to get an index-only scan when s/he wants it very badly. The current (stupid) formula handles this case just about perfectly - it will prefer a smaller index over a larger one, except when a covering index is available, in which case it will prefer the smallest covering index. That sounds exactly right to me. We get that behavior because the 10% of heap fetches that we're assuming we'll get to skip is larger than the penalty for using a bigger index. If we take out 10% and replace it by all_visible_percentage * fraction_of_tuples_fetched, then that 10% is going to drop to some infinitesmally small value on single row fetches from giant tables. But that's exactly one of the cases for which people want index-only scans in the first place. It's no better to be overly pessimistic here than it is to be overly optimistic. If the table is 90% all-visible, the probability of our finding an all-visible row is probably not 90%. But it's probably not 0.01% or 0.0001% either. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] *.sql contrib files contain unresolvable MODULE_PATHNAME
Heikki Linnakangas writes: > On 12.10.2011 18:20, Tom Lane wrote: >> Well, it can't be a comment, but what about a real psql command? >> See my suggestion of using \echo. > Frankly I think a comment is sufficient. We can make it more complicated > later if people are still confused. The thing is that this will be the third time we've gone back to try to make it more apparent that you should use CREATE EXTENSION, and I no longer believe that mere documentation is really going to get the job done. Putting in a comment will only stop the bug reports from people who bother to examine the script contents before filing a report, but the kind of folks who don't read the release notes probably won't do that either. In fact, if we just put in a comment, I confidently predict we'll be coming back to revisit this issue again in future. The only thing the \echo approach will cost us is a few more lines of C code in execute_extension_script(), and I think it's more than worth that given the evident scope of the problem. 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] [BUGS] *.sql contrib files contain unresolvable MODULE_PATHNAME
On 12.10.2011 18:20, Tom Lane wrote: Heikki Linnakangas writes: On 12.10.2011 17:33, Magnus Hagander wrote: How about adding something like -- \\psql_hates_this -- rest of comment and then at least have new versions of psql find that and stop processing the file with a more useful error at that point? Or maybe that's overengineering.. Overengineering IMHO. Besides, if a psql poison comment like that exists, then we'd have to be careful not to emit one elsewhere. Think pg_dump, if someone puts that comment in a function body... Well, it can't be a comment, but what about a real psql command? See my suggestion of using \echo. Frankly I think a comment is sufficient. We can make it more complicated later if people are still confused. -- 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] [BUGS] *.sql contrib files contain unresolvable MODULE_PATHNAME
On Wed, Oct 12, 2011 at 11:24 AM, Heikki Linnakangas wrote: >>> Overengineering IMHO. Besides, if a psql poison comment like that >>> exists, then we'd have to be careful not to emit one elsewhere. Think >>> pg_dump, if someone puts that comment in a function body... >> >> Well, it can't be a comment, but what about a real psql command? >> See my suggestion of using \echo. > > Frankly I think a comment is sufficient. We can make it more complicated > later if people are still confused. +1. -- 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] COUNT(*) and index-only scans
Aidan Van Dyk writes: > The elephant in the room is that the index-only-scan really doesn't > save a *whole* lot if the heap pages are already in shared buffers. > But it matters a *lot* when they heap pages are not in shared buffers > (both ways, saving IO, or causing lots of random IO) > Can we hope that if pages are not in shared buffers, they are not > recently modified, so hopefully both all visible, and have the VM > bit?set? Or does the table-based nature of vacuum mean there is no > value there? Hmm, that's an interesting point. If you suppose that recently-modified pages are likely to still be in memory then it could well be that an index-only scan is relatively cheap (i.e., not many actual disk reads) no matter whether it hits recently-modified pages or not. So maybe the first cut should just be to measure the overall visibility fraction and use that at face value. 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] COUNT(*) and index-only scans
On Wed, Oct 12, 2011 at 4:26 PM, Kevin Grittner wrote: >> But it matters a *lot* when they heap pages are not in shared >> buffers > > Yeah, obviously it matters more if you actually need to add a random > disk read. To be fair the indexes are also random I/O. So the case that really matters is when the index fits in RAM but the heap does not. -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] COUNT(*) and index-only scans
Aidan Van Dyk wrote: > The elephant in the room is that the index-only-scan really > doesn't save a *whole* lot if the heap pages are already in shared > buffers. It's not hard to create a simple test case where it's about three times slower to go to cached heap pages than to use the values from the index. That was just my first try, so it's not likely to be a real "worst case", although was using the default shared_memory size, so a lot of the heap pages probably came from the OS cache, rather than being in shared memory. > But it matters a *lot* when they heap pages are not in shared > buffers Yeah, obviously it matters more if you actually need to add a random disk read. -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] *.sql contrib files contain unresolvable MODULE_PATHNAME
On 10/12/2011 11:15 AM, Tom Lane wrote: \echo Use "CREATE EXTENSION foo" to load this file. \quit +1 for this. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] *.sql contrib files contain unresolvable MODULE_PATHNAME
Heikki Linnakangas writes: > On 12.10.2011 17:33, Magnus Hagander wrote: >> How about adding something like >> -- \\psql_hates_this >> -- rest of comment >> >> and then at least have new versions of psql find that and stop >> processing the file with a more useful error at that point? Or maybe >> that's overengineering.. > Overengineering IMHO. Besides, if a psql poison comment like that > exists, then we'd have to be careful not to emit one elsewhere. Think > pg_dump, if someone puts that comment in a function body... Well, it can't be a comment, but what about a real psql command? See my suggestion of using \echo. 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] COUNT(*) and index-only scans
Greg Stark writes: > On Wed, Oct 12, 2011 at 3:29 PM, Tom Lane wrote: >> The problem is precisely that the pages a query is going to read are >> likely to *not* be a random sample, but to be correlated with >> recently-dirtied pages. > Sure, but I was suggesting aiming for the nth percentile rather than a > linear factor which I don't know has any concrete meaning. Well, I have no problem with using a more complicated estimation equation, but it might be nice to get some field experience with the thing before we start complicating matters. 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] [BUGS] *.sql contrib files contain unresolvable MODULE_PATHNAME
Aidan Van Dyk writes: > On Wed, Oct 12, 2011 at 10:50 AM, Tom Lane wrote: >> Hmm ... >> \echo You should use CREATE EXTENSION foo to load this file! > Decorate them with a marker like: >\extension > And make the CREATE EXTENSION skip (or verify) it? > It will make psql stop on the \extension command. No, the point is not to stop or fail, it is to print out an unmistakable user instruction. Otherwise we'll still be getting "cube.sql failed to load for me" bug reports. So I think \echo is entirely sufficient, and we should not rely on psql features that aren't there yet. Ideally this should do what we want even in older psql releases. \echo has been there at least since 7.0. It strikes me that we could get rid of the error message clutter I worried about before if we coded like this: /* contrib/foo--1.0.sql */ \echo Use "CREATE EXTENSION foo" to load this file. \quit ... SQL commands here ... The forced \quit is a bit unfriendly maybe but it will get the job done. And again, this isn't making any assumptions about which psql version you're using. 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] [BUGS] *.sql contrib files contain unresolvable MODULE_PATHNAME
On Wed, Oct 12, 2011 at 10:50 AM, Tom Lane wrote: > Andrew Dunstan writes: >> *shrug* ok. Another thought I had was to have the file raise an error >> and have that filtered out by the extension mechanism. But I'm not sure >> if it's worth the trouble. > > Hmm ... > > \echo You should use CREATE EXTENSION foo to load this file! > > and teach CREATE EXTENSION to drop any line beginning with \echo? > The latter part seems easy enough, but I'm not quite sure about the > wording or placement of the \echo command. Putting it at the top > feels natural but the message might scroll offscreen due to errors... Decorate them with a marker like: \extension And make the CREATE EXTENSION skip (or verify) it? It will make psql stop on the \extension command. a. -- Aidan Van Dyk Create like a god, ai...@highrise.ca command like a king, http://www.highrise.ca/ work like a slave. -- 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] [BUGS] *.sql contrib files contain unresolvable MODULE_PATHNAME
On 12.10.2011 17:33, Magnus Hagander wrote: How about adding something like -- \\psql_hates_this -- rest of comment and then at least have new versions of psql find that and stop processing the file with a more useful error at that point? Or maybe that's overengineering.. Overengineering IMHO. Besides, if a psql poison comment like that exists, then we'd have to be careful not to emit one elsewhere. Think pg_dump, if someone puts that comment in a function body... -- 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] COUNT(*) and index-only scans
On Wed, Oct 12, 2011 at 3:29 PM, Tom Lane wrote: >>> What I suggest as a first cut for that is: simply derate the visibility >>> fraction as the fraction >>> of the table expected to be scanned gets smaller. > >> I think there's a statistically more rigorous way of accomplishing the >> same thing. If you treat the pages we estimate we're going to read as >> a random sample of the population of pages then your expected value is >> the fraction of the overall population that is all-visible but your >> 95th percentile confidence interval will be, uh, a simple formula we >> can compute but I don't recall off-hand. > > The problem is precisely that the pages a query is going to read are > likely to *not* be a random sample, but to be correlated with > recently-dirtied pages. Sure, but I was suggesting aiming for the nth percentile rather than a linear factor which I don't know has any concrete meaning. -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] COUNT(*) and index-only scans
On Wed, Oct 12, 2011 at 10:37 AM, Tom Lane wrote: >> - Suppose the table has a million rows and we're going to read 100 of >> them, or 0.01%. Now it might appear that a covering index has a >> negligible advantage over a non-covering index, but in fact I think we >> still want to err on the side of trying to use the covering index. > > Given that fact pattern we still will, I think. We'll still prefer an > indexscan over a seqscan, for sure. In any case, if you believe the > assumption that those 100 rows are more likely to be recently-dirtied > than the average row, I'm not sure why you think we should be trying to > force an assumption that index-only will succeed here. The elephant in the room is that the index-only-scan really doesn't save a *whole* lot if the heap pages are already in shared buffers. But it matters a *lot* when they heap pages are not in shared buffers (both ways, saving IO, or causing lots of random IO) Can we hope that if pages are not in shared buffers, they are not recently modified, so hopefully both all visible, and have the VM bit?set? Or does the table-based nature of vacuum mean there is no value there? -- Aidan Van Dyk Create like a god, ai...@highrise.ca command like a king, http://www.highrise.ca/ work like a slave. -- 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] [BUGS] *.sql contrib files contain unresolvable MODULE_PATHNAME
Andrew Dunstan writes: > *shrug* ok. Another thought I had was to have the file raise an error > and have that filtered out by the extension mechanism. But I'm not sure > if it's worth the trouble. Hmm ... \echo You should use CREATE EXTENSION foo to load this file! and teach CREATE EXTENSION to drop any line beginning with \echo? The latter part seems easy enough, but I'm not quite sure about the wording or placement of the \echo command. Putting it at the top feels natural but the message might scroll offscreen due to errors... 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] COUNT(*) and index-only scans
Robert Haas writes: > On Wed, Oct 12, 2011 at 9:52 AM, Tom Lane wrote: >> What bothers me considerably more is the issue about how specific >> queries might see an all-visible fraction that's very substantially >> different from the table's overall ratio, > - Suppose VACUUM processes the table and makes it all-visible. Then, > somebody comes along and updates one tuple on every page, making them > all not-all-visible, but not trigger VACUUM because we're nowhere > close the 20% threshold. Now COUNT(*) will think it should use an > index-scan, but really... not so much. In fact, even if it's only > that a tuple has been updated on 25% of the pages, we're probably in > trouble. Yeah, but that would be a pretty unlucky pattern, and in any case the fix for it is going to be to make autovacuum more aggressive. > - Suppose the table has a million rows and we're going to read 100 of > them, or 0.01%. Now it might appear that a covering index has a > negligible advantage over a non-covering index, but in fact I think we > still want to err on the side of trying to use the covering index. Given that fact pattern we still will, I think. We'll still prefer an indexscan over a seqscan, for sure. In any case, if you believe the assumption that those 100 rows are more likely to be recently-dirtied than the average row, I'm not sure why you think we should be trying to force an assumption that index-only will succeed here. 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] [BUGS] *.sql contrib files contain unresolvable MODULE_PATHNAME
On Wed, Oct 12, 2011 at 16:31, Andrew Dunstan wrote: > > > On 10/12/2011 10:12 AM, Tom Lane wrote: >> >> Robert Haas writes: >>> >>> We could do that, but I think Heikki's idea of adding a comment would >>> help a lot. >> >> +1. Simple, easy, should help significantly. >> >> Also, I disagree with the position that the files "aren't SQL files". >> Sure they are. You'd want them treated as SQL by your editor, for >> example. So changing the extension is just wrong. >> > > *shrug* ok. Another thought I had was to have the file raise an error and > have that filtered out by the extension mechanism. But I'm not sure if it's > worth the trouble. How about adding something like -- \\psql_hates_this -- rest of comment and then at least have new versions of psql find that and stop processing the file with a more useful error at that point? Or maybe that's overengineering.. -- 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] [BUGS] *.sql contrib files contain unresolvable MODULE_PATHNAME
On 10/12/2011 10:12 AM, Tom Lane wrote: Robert Haas writes: We could do that, but I think Heikki's idea of adding a comment would help a lot. +1. Simple, easy, should help significantly. Also, I disagree with the position that the files "aren't SQL files". Sure they are. You'd want them treated as SQL by your editor, for example. So changing the extension is just wrong. *shrug* ok. Another thought I had was to have the file raise an error and have that filtered out by the extension mechanism. But I'm not sure if it's worth the trouble. 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] COUNT(*) and index-only scans
Greg Stark writes: > Assuming you're in a steady-state situation the amount of all-visible > blocks will fluctuate from a high just after vacuum to a low just > before the next vacuum. There are other ways a block can be marked > all-visible but for the most part I would expect the fraction to go > steadily down until vacuum comes along and cleans things up. > So if vacuum tracked the fraction of blocks marked all-visible > *before* it processed them and the fraction it marked all-visible > after processing we have an upper and lower bound. If we knew how long > it's been since vacuum we could interpolate between those, or we could > just take the mean, or we could take the lower bound as a conservative > estimate. I thought of that too, but we don't do the comparable thing for dead tuple counts, and I am not convinced that we should do it for visibility. I'd rather have a simple rule that "it's right immediately after VACUUM", so that at least trivial cases like read-only tables work correctly. >> What I suggest as a first cut for that is: simply derate the visibility >> fraction as the fraction >> of the table expected to be scanned gets smaller. > I think there's a statistically more rigorous way of accomplishing the > same thing. If you treat the pages we estimate we're going to read as > a random sample of the population of pages then your expected value is > the fraction of the overall population that is all-visible but your > 95th percentile confidence interval will be, uh, a simple formula we > can compute but I don't recall off-hand. The problem is precisely that the pages a query is going to read are likely to *not* be a random sample, but to be correlated with recently-dirtied pages. > ... It currently uses all expected values but in many > cases it would be valuable if the planner knew what the standard > deviation of those estimates was. No doubt, but I'm not volunteering to fix that before we can have a non-toy estimate for index-only scans. 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] COUNT(*) and index-only scans
On Wed, Oct 12, 2011 at 9:52 AM, Tom Lane wrote: > What bothers me considerably more is the issue about how specific > queries might see an all-visible fraction that's very substantially > different from the table's overall ratio, especially in examples such as > historical-data tables where most of the update and query activity has > to do with recently-added rows. I don't see any practical way to attack > that problem with statistics; we're just going to have to adopt some > estimation rule. What I suggest as a first cut for that is: simply > derate the visibility fraction as the fraction of the table expected to > be scanned gets smaller. That is, if the query fetches nearly all of > the table, take the stored visibility ratio at face value; if it fetches > only one block, never believe that that will be an all-visible block; > and in general if we're expecting to read a fraction f of the pages, > multiply the whole-table visibility ratio by f before using it in the > cost estimate. This amounts to assuming that the historical-data case > is the usual case, but I'm not sure that's unfair. I don't think that's an unfair assumption -- in fact I think it's exactly the right assumption -- but I'm worried about how the math works out with that specific proposal. - Suppose VACUUM processes the table and makes it all-visible. Then, somebody comes along and updates one tuple on every page, making them all not-all-visible, but not trigger VACUUM because we're nowhere close the 20% threshold. Now COUNT(*) will think it should use an index-scan, but really... not so much. In fact, even if it's only that a tuple has been updated on 25% of the pages, we're probably in trouble. - Suppose the table has a million rows and we're going to read 100 of them, or 0.01%. Now it might appear that a covering index has a negligible advantage over a non-covering index, but in fact I think we still want to err on the side of trying to use the covering index. In fact, even if we're only reading a single row, we probably still generally want to pick up the covering index, to cater to the case where someone is doing primary key fetches against a gigantic table and hoping that index-only scans will save them from random I/O hell. -- 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] COUNT(*) and index-only scans
On Wed, Oct 12, 2011 at 2:52 PM, Tom Lane wrote: > I think it's overkill, and possibly unpleasantly unstable as well. > For the initial attack on this we should just have VACUUM and ANALYZE > count the number of all-visible blocks and store that in pg_class along > with the tuple-count statistics. There's no reason to think that this > will be any worse than the way we deal with dead tuple counts, for > instance. So I have a theory. Assuming you're in a steady-state situation the amount of all-visible blocks will fluctuate from a high just after vacuum to a low just before the next vacuum. There are other ways a block can be marked all-visible but for the most part I would expect the fraction to go steadily down until vacuum comes along and cleans things up. So if vacuum tracked the fraction of blocks marked all-visible *before* it processed them and the fraction it marked all-visible after processing we have an upper and lower bound. If we knew how long it's been since vacuum we could interpolate between those, or we could just take the mean, or we could take the lower bound as a conservative estimate. > What I suggest as a first cut for that is: simply derate the visibility > fraction as the fraction > of the table expected to be scanned gets smaller. I think there's a statistically more rigorous way of accomplishing the same thing. If you treat the pages we estimate we're going to read as a random sample of the population of pages then your expected value is the fraction of the overall population that is all-visible but your 95th percentile confidence interval will be, uh, a simple formula we can compute but I don't recall off-hand. This gets back to a discussion long-ago of what estimates the planner should be using. It currently uses all expected values but in many cases it would be valuable if the planner knew what the standard deviation of those estimates was. It might sometimes be better to pick a plan that's slightly worse on average but is less likely to be much worse. -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] *.sql contrib files contain unresolvable MODULE_PATHNAME
Robert Haas writes: > We could do that, but I think Heikki's idea of adding a comment would > help a lot. +1. Simple, easy, should help significantly. Also, I disagree with the position that the files "aren't SQL files". Sure they are. You'd want them treated as SQL by your editor, for example. So changing the extension is just wrong. 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] COUNT(*) and index-only scans
Robert Haas writes: > On Wed, Oct 12, 2011 at 2:50 AM, Jeff Davis wrote: >> On Tue, 2011-10-11 at 13:22 -0400, Robert Haas wrote: >>> The real issue is that the costing estimates need to be accurate, and >>> that's where the rubber hits the road. >> Can you send stats messages to keep track when you unset a bit in the >> VM? That might allow it to be more up-to-date. > In theory, that seems like it would work, although I'm a little > worried about the overhead. I think it's overkill, and possibly unpleasantly unstable as well. For the initial attack on this we should just have VACUUM and ANALYZE count the number of all-visible blocks and store that in pg_class along with the tuple-count statistics. There's no reason to think that this will be any worse than the way we deal with dead tuple counts, for instance. What bothers me considerably more is the issue about how specific queries might see an all-visible fraction that's very substantially different from the table's overall ratio, especially in examples such as historical-data tables where most of the update and query activity has to do with recently-added rows. I don't see any practical way to attack that problem with statistics; we're just going to have to adopt some estimation rule. What I suggest as a first cut for that is: simply derate the visibility fraction as the fraction of the table expected to be scanned gets smaller. That is, if the query fetches nearly all of the table, take the stored visibility ratio at face value; if it fetches only one block, never believe that that will be an all-visible block; and in general if we're expecting to read a fraction f of the pages, multiply the whole-table visibility ratio by f before using it in the cost estimate. This amounts to assuming that the historical-data case is the usual case, but I'm not sure that's unfair. 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] [BUGS] *.sql contrib files contain unresolvable MODULE_PATHNAME
On Wed, Oct 12, 2011 at 4:42 AM, Magnus Hagander wrote: >> 9.1 has been out for only a couple of months, and we've seen a lot of people >> trying to do that already. In hindsight, we probably should've chosen a >> different filename extension for those files, to make it clear that you >> can't just run them in psql. It's too late for that, but a comment at the >> top of the .sql files would be good: >> >> --- a/contrib/intarray/intarray--1.0.sql >> +++ b/contrib/intarray/intarray--1.0.sql >> @@ -1,4 +1,8 @@ >> -/* contrib/intarray/intarray--1.0.sql */ >> +/* >> + * contrib/intarray/intarray--1.0.sql >> + * >> + * Script file to be run by CREATE EXTENSION. >> + */ >> >> -- >> -- Create the user-defined type for the 1-D integer arrays (_int4) >> >> The people trying to run these files with psql look inside the file when >> they get the error, so mentioning "CREATE EXTENSION" should give a hint on >> what to do. > > Hmm. is there some way that we could make it do something that would > affect only psql? I guess not, because any kind of \-command would > actually break the CREATE EXTENSION running of the script, right? > > But it would be useful to be able to inject something there that psql > would notice but the backend would ignore... We could do that, but I think Heikki's idea of adding a comment would help a lot. When people try to run the file through psql and it fails due to the MODULE_PATHNAME stuff, the next thing they're probably going to do is look at the file contents. If they see something there telling them to use CREATE EXTENSION, that will help. -- 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] COUNT(*) and index-only scans
On Wed, Oct 12, 2011 at 2:50 AM, Jeff Davis wrote: > On Tue, 2011-10-11 at 13:22 -0400, Robert Haas wrote: >> The real issue is that the costing estimates need to be accurate, and >> that's where the rubber hits the road. Otherwise, even if we pick the >> right way to scan the table, we may do silly things up the line when >> we go to start constructing the join order. I think we need to beef >> up ANALYZE to gather statistics on the fraction of the pages that are >> marked all-visible, or maybe VACUUM should gather that information. >> The trouble is that if we VACUUM and then ANALYZE, we'll often get >> back a value very close to 100%, but then the real value may diminish >> quite a bit before the next auto-analyze fires. I think if we can >> figure out what to do about that problem we'll be well on our way... > > Can you send stats messages to keep track when you unset a bit in the > VM? That might allow it to be more up-to-date. In theory, that seems like it would work, although I'm a little worried about the overhead. -- 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] loss of transactions in streaming replication
On Wed, Oct 12, 2011 at 5:45 AM, Fujii Masao wrote: > In 9.2dev and 9.1, when walreceiver detects an error while sending data to > WAL stream, it always emits ERROR even if there are data available in the > receive buffer. This might lead to loss of transactions because such > remaining data are not received by walreceiver :( Won't it just reconnect? -- 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] Overhead cost of Serializable Snapshot Isolation
On Wed, Oct 12, 2011 at 8:44 AM, Florian Pflug wrote: > With such a switch, every application that relies on true serializability for > correctness would be prone to silent data corruption should the switch ever > get set to "off" accidentally. Agreed. > Without such a switch, OTOH, all that will happen are a few more aborts due to > serialization errors in application who request SERIALIZABLE when they really > only need REPEATABLE READ. Which, in the worst case, is a performance issue, > but never an issue of correctness. Right. And, in fairness: 1. The benchmark that I did was probably close to a worst-case scenario for SSI. Since there are no actual writes, there is no possibility of serialization conflicts, but the system must still be prepared for the possibility of a write (and, thus, potentially, a conflict) at any time. In addition, all of the transactions are very short, magnifying the effect of transaction start and cleanup overhead. In real life, people who have this workload are unlikely to use serializable mode in the first place. The whole point of serializability (not just SSI) is that it helps prevent anomalies when you have complex transactions that could allow subtle serialization anomalies to creep in. Single-statement transactions that read (or write) values based on a primary key are not the workload where you have that problem. You'd probably be happy to turn off MVCC altogether if we had an option for that. 2. Our old SERIALIZABLE behavior (now REPEATABLE READ) is a pile of garbage. Since Kevin started beating the drum about SSI, I've come across (and posted about) situations where REPEATABLE READ read causes serialization anomalies that don't exist at the READ COMMITTED level (which is exactly the opposite of what is really supposed to happen - REPEATABLE READ is supposed to provide more isolation, not less); and Kevin's pointed out many situations where REPEATABLE READ utterly fails to deliver serializable behavior. I'm not exactly thrilled with these benchmark results, but going back to a technology that doesn't work is not better. If individual users want to request that defective behavior for their applications, I am fine with giving them that option, and we have. But if people actually want serializability and we given them REPEATABLE READ, then they're going to get wrong behavior. The fact that we've been shipping that wrong behavior for years and years for lack of anything better is not a reason to continue doing it. I agree with Tom's comment upthread that the best thing to do here is put some effort into improving SSI. I think it's probably going to perform adequately for the workloads where people actually need it, but I'd certainly like to see us make it better. -- 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] [BUGS] *.sql contrib files contain unresolvable MODULE_PATHNAME
On 10/12/2011 04:39 AM, Heikki Linnakangas wrote: On 12.10.2011 10:58, Stefan Kaltenbrunner wrote: On 10/12/2011 09:53 AM, Martin Pitt wrote: Hello all, In https://launchpad.net/bugs/835502 it was reported that the 9.1.1 contrib *.sql files contain the token "MODULE_PATHNAME", which is unknown: psql test< /usr/share/postgresql/9.1/extension/intarray--1.0.sql This fails with a ton of errors about the file "MODULE_PATHNAME" not existing. When I replace this with "$libdir/_int", it works: sed 's!MODULE_PATHNAME!$libdir/_int!g' /usr/share/postgresql/9.1/extension/intarray--1.0.sql | psql test Is that something I'm doing wrong in the packaging, or should the contrib Makefiles be fixed to do this substitution? It doesn't only affect intarray, but pretty much all *.sql files. uh - the reason is that contrib is now packaged as extensions and that you are supposed to use "CREATE EXTENSION intarray;" on the SQL level instead of manually loading sql-scripts through psql. 9.1 has been out for only a couple of months, and we've seen a lot of people trying to do that already. In hindsight, we probably should've chosen a different filename extension for those files, to make it clear that you can't just run them in psql. It's too late for that, but a comment at the top of the .sql files would be good: I've made this mistake myself in an unthinking moment. I suggest that we deprecate calling them something.sql and add code ASAP providing for some other suffix ( .xtn ?) with legacy support for falling back to .sql. I'd almost be inclined to backpatch it while there are so few third party extensions out there. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
2011/10/12 Robert Haas :
> On Wed, Oct 12, 2011 at 8:07 AM, Kohei KaiGai wrote:
>> I'm currently trying to revise my patches according to your suggestions,
>> but I'm facing a trouble about error messages when user tries to drop
>> a non-exists object.
>>
>> Because the ObjectProperty array has an entry for each catalogs, it is
>> unavailable to hold the name of object type (such as "table" or "index")
>> when multiple object types are associated with a particular system
>> catalog, such as pg_class, pg_type or pg_proc.
>>
>> How should I implement the following block?
>>
>> if (!OidIsValid(address.objectId))
>> {
>> ereport(NOTICE,
>> (errmsg("%s \"%s\" does not exist, skipping",
>> get_object_property_typetext(stmt->removeType),
>> NameListToString(objname;
>> continue;
>> }
>>
>> One idea is to add a separated array to translate from OBJECT_* to
>> its text representation. (Maybe, it can be used to translattions with
>> opposite direction.)
>
> For reasons of translation, you can't do something like "%s \"%s\"
> does not exist, skipping". Instead I think you need an array that
> works something like dropmsgstringarray[], but based on the OBJECT_*
> constants rather than the RELKIND_* constants. IOW, it maps the
> object type to the full error message, not just the name of the object
> type.
>
OK, I'll revise the code based on this idea.
Thanks,
--
KaiGai Kohei
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
On Wed, Oct 12, 2011 at 8:07 AM, Kohei KaiGai wrote:
> I'm currently trying to revise my patches according to your suggestions,
> but I'm facing a trouble about error messages when user tries to drop
> a non-exists object.
>
> Because the ObjectProperty array has an entry for each catalogs, it is
> unavailable to hold the name of object type (such as "table" or "index")
> when multiple object types are associated with a particular system
> catalog, such as pg_class, pg_type or pg_proc.
>
> How should I implement the following block?
>
> if (!OidIsValid(address.objectId))
> {
> ereport(NOTICE,
> (errmsg("%s \"%s\" does not exist, skipping",
> get_object_property_typetext(stmt->removeType),
> NameListToString(objname;
> continue;
> }
>
> One idea is to add a separated array to translate from OBJECT_* to
> its text representation. (Maybe, it can be used to translattions with
> opposite direction.)
For reasons of translation, you can't do something like "%s \"%s\"
does not exist, skipping". Instead I think you need an array that
works something like dropmsgstringarray[], but based on the OBJECT_*
constants rather than the RELKIND_* constants. IOW, it maps the
object type to the full error message, not just the name of the object
type.
--
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] Overhead cost of Serializable Snapshot Isolation
On Oct11, 2011, at 23:35 , Simon Riggs wrote: > On Tue, Oct 11, 2011 at 10:30 PM, Florian Pflug wrote: > >> That experience has taught me that backwards compatibility, while very >> important in a lot of cases, has the potential to do just as much harm >> if overdone. > > Agreed. Does my suggestion represent overdoing it? I ask for balance, > not an extreme. It's my belief that an "off" switch for true serializability is overdoing it, yes. With such a switch, every application that relies on true serializability for correctness would be prone to silent data corruption should the switch ever get set to "off" accidentally. Without such a switch, OTOH, all that will happen are a few more aborts due to serialization errors in application who request SERIALIZABLE when they really only need REPEATABLE READ. Which, in the worst case, is a performance issue, but never an issue of correctness. best regards, Florian Pflug -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
Robert,
I'm currently trying to revise my patches according to your suggestions,
but I'm facing a trouble about error messages when user tries to drop
a non-exists object.
Because the ObjectProperty array has an entry for each catalogs, it is
unavailable to hold the name of object type (such as "table" or "index")
when multiple object types are associated with a particular system
catalog, such as pg_class, pg_type or pg_proc.
How should I implement the following block?
if (!OidIsValid(address.objectId))
{
ereport(NOTICE,
(errmsg("%s \"%s\" does not exist, skipping",
get_object_property_typetext(stmt->removeType),
NameListToString(objname;
continue;
}
One idea is to add a separated array to translate from OBJECT_* to
its text representation. (Maybe, it can be used to translattions with
opposite direction.)
Thanks,
2011/10/11 Robert Haas :
> On Mon, Oct 10, 2011 at 1:38 PM, Kohei KaiGai wrote:
>> I'm sorry again. I tought it was obvious from the filenames.
>
> I guess I got confused because you re-posted part 2 without the other
> parts, and I got mixed up and thought you were reposting part one.
>
> I've committed a stripped-down version of the part one patch, which
> had several mistakes even in just the part I committed - e.g., you
> forgot TABLESPACEOID. I also did some renaming for clarity.
>
> I'm going to throw this back to you for rebasing at this point, which
> I realize is going to be somewhat of an unenjoyable task given the way
> I cut up your changes to objectaddress.c, but I wasn't very confident
> that all of the entries were correct (the one for attributes seemed
> clearly wrong to me, for example), and I didn't want to commit a bunch
> of stuff that wasn't going to be exercised. I suggest that you merge
> the remainder of the part-one changes into part-two. On the flip
> side, I think you should take the stuff that deals with dropping
> relations OUT of part two. I don't see what good it does us to try to
> centralize the drop logic if we still have to have special cases for
> relations, so let's just leave that separate for now until we figure
> out a better approach, or at least split it off as a separate patch so
> that it doesn't hold up all the other changes.
>
> I think get_object_namespace() needs substantial revision. Instead of
> passing the object type and the object address, why not just pass the
> object address? You should be able to use the classId in the address
> to figure out everything you need to know. Since this function is
> private to objectaddress.c, there's no reason for it to use those
> accessor functions - it can just iterate through the array just as
> object_exists() does. That way you also avoid iterating through the
> array multiple times. I also think that we probably ought to revise
> AlterObjectNamespace() to make use of this new machinery, instead of
> making the caller pass in all the same information that
> objectaddress.c is now learning how to provide. That would possibly
> open the way to a bunch more consolidation of the SET SCHEMA code; in
> fact, we might want to clean that up first, before dealing with the
> DROP stuff.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
--
KaiGai Kohei
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] loss of transactions in streaming replication
Hi, In 9.2dev and 9.1, when walreceiver detects an error while sending data to WAL stream, it always emits ERROR even if there are data available in the receive buffer. This might lead to loss of transactions because such remaining data are not received by walreceiver :( To prevent transaction loss, I'm thinking to change walreceiver so that it always ignores an error (specifically, emits COMMERROR instead of ERROR) during sending data. Then walreceiver receives data if available. If an error occurrs during receiving data, walreceiver can emit ERROR this time. Comments? Better ideas? 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
[HACKERS] pl/perl example in the doc no longer works in 9.1
Hi,
An example in the PG documentation gives an error:
http://www.postgresql.org/docs/9.1/interactive/plperl-global.html
CREATE OR REPLACE FUNCTION myfuncs() RETURNS void AS $$
$_SHARED{myquote} = sub {
my $arg = shift;
$arg =~ s/(['\\])/\\$1/g;
return "'$arg'";
};
$$ LANGUAGE plperl;
SELECT myfuncs(); /* initializes the function */
ERROR: PL/Perl function must return reference to hash or array
CONTEXT: PL/Perl function "myfuncs"
Not sure if this is now an expected behaviour. Is it? Accordingly, I
can open this in pgsql-bugs or put this issue in pgsql-docs.
--
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] [BUGS] *.sql contrib files contain unresolvable MODULE_PATHNAME
On Wed, Oct 12, 2011 at 10:39, Heikki Linnakangas wrote: > On 12.10.2011 10:58, Stefan Kaltenbrunner wrote: >> >> On 10/12/2011 09:53 AM, Martin Pitt wrote: >>> >>> Hello all, >>> >>> In https://launchpad.net/bugs/835502 it was reported that the 9.1.1 >>> contrib *.sql files contain the token "MODULE_PATHNAME", which is >>> unknown: >>> >>> psql test< /usr/share/postgresql/9.1/extension/intarray--1.0.sql >>> >>> This fails with a ton of errors about the file "MODULE_PATHNAME" not >>> existing. >>> >>> When I replace this with "$libdir/_int", it works: >>> >>> sed 's!MODULE_PATHNAME!$libdir/_int!g' >>> /usr/share/postgresql/9.1/extension/intarray--1.0.sql | psql test >>> >>> Is that something I'm doing wrong in the packaging, or should the >>> contrib Makefiles be fixed to do this substitution? >>> >>> It doesn't only affect intarray, but pretty much all *.sql files. >> >> uh - the reason is that contrib is now packaged as extensions and that >> you are supposed to use "CREATE EXTENSION intarray;" on the SQL level >> instead of manually loading sql-scripts through psql. > > 9.1 has been out for only a couple of months, and we've seen a lot of people > trying to do that already. In hindsight, we probably should've chosen a > different filename extension for those files, to make it clear that you > can't just run them in psql. It's too late for that, but a comment at the > top of the .sql files would be good: > > --- a/contrib/intarray/intarray--1.0.sql > +++ b/contrib/intarray/intarray--1.0.sql > @@ -1,4 +1,8 @@ > -/* contrib/intarray/intarray--1.0.sql */ > +/* > + * contrib/intarray/intarray--1.0.sql > + * > + * Script file to be run by CREATE EXTENSION. > + */ > > -- > -- Create the user-defined type for the 1-D integer arrays (_int4) > > The people trying to run these files with psql look inside the file when > they get the error, so mentioning "CREATE EXTENSION" should give a hint on > what to do. Hmm. is there some way that we could make it do something that would affect only psql? I guess not, because any kind of \-command would actually break the CREATE EXTENSION running of the script, right? But it would be useful to be able to inject something there that psql would notice but the backend would ignore... -- 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] Overhead cost of Serializable Snapshot Isolation
On Wed, Oct 12, 2011 at 8:50 AM, Peter Eisentraut wrote: > On tis, 2011-10-11 at 21:50 +0100, Simon Riggs wrote: >> I'm keen to ensure people enjoy the possibility of upgrading to the >> latest release. The continual need to retest applications mean that >> very few users upgrade quickly or with anywhere near the frequency >> with which we put out new releases. What is the point of rushing out >> software that nobody can use? pg_upgrade doesn't change your >> applications, so there isn't a fast path to upgrade in the way you >> seem to think. > > This is a valid concern, which I share, but I think adding a few > configuration parameters of the nature, "this setting really means what > this setting meant in the old release" is only the tip of the iceberg. > Ensuring full compatibility between major releases would require an > extraordinary amount of effort, including a regression test suite that > would be orders of magnitude larger than what we currently have. I > frankly don't see the resources to do that. > > The workaround strategy is that we maintain backbranches, so that users > are not forced to upgrade to incompatible releases. > > Actually, I'm currently personally more concerned about the breakage we > introduce in minor releases. We'd need to solve that problem before we > can even begin to think about dealing with the major release issue. Thanks, these look like reasonable discussion points with no personal comments added. I agree that config parameters don't solve the whole problem, though much of the iceberg is already covered with them. Currently about half of our parameters are either on/off behaviour switches. Right now we are inconsistent about whether we add a parameter for major features: sync_scans, hot_standby, partial vacuum all had ways of disabling them if required - virtually all features can be disabled, bgwriter, autovacuum etc even though it is almost never a recommendation to do so. I can't see a good argument for including some switches, but not others. SSI is a complex new feature and really should have an off switch. Right now, we've had one report and a benchmark that show severe performance degradation and that might have benefited from turning it off. That is not sufficient at this point to convince some people, so I am happy to wait to see if further reports emerge. SSI doesn't affect everybody. Yes, I agree that the only really good answer in the general case is to maintain back branches, or provide enhanced versions as some vendors choose to do. That is not my first thought, and try quite hard to put my (/our) best work into mainline Postgres. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] *.sql contrib files contain unresolvable MODULE_PATHNAME
On 12.10.2011 10:58, Stefan Kaltenbrunner wrote: On 10/12/2011 09:53 AM, Martin Pitt wrote: Hello all, In https://launchpad.net/bugs/835502 it was reported that the 9.1.1 contrib *.sql files contain the token "MODULE_PATHNAME", which is unknown: psql test< /usr/share/postgresql/9.1/extension/intarray--1.0.sql This fails with a ton of errors about the file "MODULE_PATHNAME" not existing. When I replace this with "$libdir/_int", it works: sed 's!MODULE_PATHNAME!$libdir/_int!g' /usr/share/postgresql/9.1/extension/intarray--1.0.sql | psql test Is that something I'm doing wrong in the packaging, or should the contrib Makefiles be fixed to do this substitution? It doesn't only affect intarray, but pretty much all *.sql files. uh - the reason is that contrib is now packaged as extensions and that you are supposed to use "CREATE EXTENSION intarray;" on the SQL level instead of manually loading sql-scripts through psql. 9.1 has been out for only a couple of months, and we've seen a lot of people trying to do that already. In hindsight, we probably should've chosen a different filename extension for those files, to make it clear that you can't just run them in psql. It's too late for that, but a comment at the top of the .sql files would be good: --- a/contrib/intarray/intarray--1.0.sql +++ b/contrib/intarray/intarray--1.0.sql @@ -1,4 +1,8 @@ -/* contrib/intarray/intarray--1.0.sql */ +/* + * contrib/intarray/intarray--1.0.sql + * + * Script file to be run by CREATE EXTENSION. + */ -- -- Create the user-defined type for the 1-D integer arrays (_int4) The people trying to run these files with psql look inside the file when they get the error, so mentioning "CREATE EXTENSION" should give a hint on what to do. -- 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] Overhead cost of Serializable Snapshot Isolation
On Wed, Oct 12, 2011 at 6:34 AM, Heikki Linnakangas wrote: > On 11.10.2011 23:21, Simon Riggs wrote: >> >> If the normal default_transaction_isolation = read committed and all >> transactions that require serializable are explicitly marked in the >> application then there is no way to turn off SSI without altering the >> application. That is not acceptable, since it causes changes in >> application behaviour and possibly also performance issues. > > I don't get that. If all the transactions that require serializability are > marked as such, why would you disable SSI for them? That would just break > the application, since the transactions would no longer be serializable. > > If they don't actually need serializability, but repeatable read is enough, > then mark them that way. Obviously, if apps require serializability then turning serializability off would break them. That is not what I have said, nor clearly not what I would mean by "turning off SSI". The type of serializability we had in the past is now the same as repeatable read. So the request is for a parameter that changes a request for serializable into a request for repeatable read, so that applications are provided with exactly what they had before, in 9.0 and earlier. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Online base backup from the hot-standby
2011/10/12 Jun Ishiduka :
> > ERROR: full_page_writes on master is set invalid at least once since
> > latest checkpoint
> >
> > I think this error should be rewritten as
> > ERROR: full_page_writes on master has been off at some point since
> > latest checkpoint
> >
> > We should be using 'off' instead of 'invalid' since that is what is what
> > the user sets it to.
>
> Sure.
What about the following message? It sounds more precise to me.
ERROR: WAL generated with full_page_writes=off was replayed since last
restartpoint
> I updated to patch corresponded above-comments.
Thanks for updating the patch! Here are the comments:
* don't yet have the insert lock, forcePageWrites could change under
us,
* but we'll recheck it once we have the lock.
*/
- doPageWrites = fullPageWrites || Insert->forcePageWrites;
+ doPageWrites = Insert->fullPageWrites || Insert->forcePageWrites;
The source comment needs to be modified.
* just turned off, we could recompute the record without full pages,
but
* we choose not to bother.)
*/
- if (Insert->forcePageWrites && !doPageWrites)
+ if ((Insert->fullPageWrites || Insert->forcePageWrites) &&
!doPageWrites)
Same as above.
+ LWLockAcquire(WALInsertLock, LW_EXCLUSIVE);
+ XLogCtl->Insert.fullPageWrites = fullPageWrites;
+ LWLockRelease(WALInsertLock);
I don't think WALInsertLock needs to be hold here because there is no
concurrently running process which can access Insert.fullPageWrites.
For example, Insert->currpos and Insert->LogwrtResult are also changed
without the lock there.
The source comment of XLogReportParameters() needs to be modified.
XLogReportParameters() should skip writing WAL if full_page_writes has not been
changed by SIGHUP.
XLogReportParameters() should skip updating pg_control if any parameter related
to hot standby has not been changed.
+ if (!fpw_manager)
+ {
+ LWLockAcquire(WALInsertLock, LW_EXCLUSIVE);
+ fpw = XLogCtl->Insert.fullPageWrites;
+ LWLockRelease(WALInsertLock);
It's safe to take WALInsertLock with shared mode here.
In checkpoint, XLogReportParameters() is called only when wal_level is
hot_standby.
OTOH, in walwriter, it's always called even when wal_level is not hot_standby.
Can't we skip calling XLogReportParameters() whenever wal_level is not
hot_standby?
In do_pg_start_backup() and do_pg_stop_backup(), the spinlock must be held to
see XLogCtl->lastFpwDisabledLSN.
+ /* check whether the master's FPW is 'off' since pg_start_backup. */
+ if (recovery_in_progress && XLByteLE(startpoint,
XLogCtl->lastFpwDisabledLSN))
+ ereport(ERROR,
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("full_page_writes on master has been off at
some point
during online backup")));
What about changing the error message to:
ERROR: WAL generated with full_page_writes=off was replayed during online backup
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] Overhead cost of Serializable Snapshot Isolation
On tis, 2011-10-11 at 21:50 +0100, Simon Riggs wrote: > I'm keen to ensure people enjoy the possibility of upgrading to the > latest release. The continual need to retest applications mean that > very few users upgrade quickly or with anywhere near the frequency > with which we put out new releases. What is the point of rushing out > software that nobody can use? pg_upgrade doesn't change your > applications, so there isn't a fast path to upgrade in the way you > seem to think. This is a valid concern, which I share, but I think adding a few configuration parameters of the nature, "this setting really means what this setting meant in the old release" is only the tip of the iceberg. Ensuring full compatibility between major releases would require an extraordinary amount of effort, including a regression test suite that would be orders of magnitude larger than what we currently have. I frankly don't see the resources to do that. The workaround strategy is that we maintain backbranches, so that users are not forced to upgrade to incompatible releases. Actually, I'm currently personally more concerned about the breakage we introduce in minor releases. We'd need to solve that problem before we can even begin to think about dealing with the major release issue. -- 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
> > Some testing notes
> > --
> > select pg_start_backup('x');
> > ERROR: full_page_writes on master is set invalid at least once since
> > latest checkpoint
> >
> > I think this error should be rewritten as
> > ERROR: full_page_writes on master has been off at some point since
> > latest checkpoint
> >
> > We should be using 'off' instead of 'invalid' since that is what is what
> > the user sets it to.
>
> Sure.
> > Minor typo above at 'CHECKPOTNT'
>
> Yes.
I updated to patch corresponded above-comments.
Regards.
Jun Ishizuka
NTT Software Corporation
TEL：045-317-7018
E-Mail: ishizuka@po.ntts.co.jp
standby_online_backup_09base-03fpw.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] Proposal: casts row to array and array to row
2011/10/11 Tom Lane : > Pavel Stehule writes: >> 2011/10/11 Robert Haas : >>> On Tue, Oct 11, 2011 at 4:40 AM, Pavel Stehule >>> wrote: What do you think about this idea? > > It's a bad one. > >>> Well, a ROW can contain values of different types; an ARRAY can't. > >> yes, I know - but it should be problem only in few cases - when is not >> possible to cast a row field to array field. > > This idea is basically the same as "data types don't matter", which is > not SQL-ish and certainly not Postgres-ish. This proposal is not about this. The data types are important and I don't propose a universal data type or some automatic datatype. Result of cast op has know type defined in planner time. Proposal is more about respect to datatypes than now. A some row based operations are based on serialization and deserialization to text. This is in PLPerl or PLpgSQL, on user level or system level. When you have to do some task, then you have to solve quoting, NULL replacement, ... Casts between array and rows just remove these ugly hacks - so work can be faster and more robust (without string operations (when is possible) and without quoting string ops at least). unfortunately I am not able to solve these requests on custom functions level, because I can't to specify a target type from function (I am missing a some polymorphic type like "anytype"). Regards Pavel Stehule > > 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
