Re: [HACKERS] XIDs and big boxes again ...
Joshua D. Drake wrote: Hans-Juergen Schoenig wrote: regards, tom lane overhead is not an issue here - if i lose 10 or 15% i am totally fine as long as i can reduce vacuum overhead to an absolute minimum. overhead will vary with row sizes anyway - this is not the point. I am not buying this argument. If you have a 5TB database, I am going to assume you put it on enterprise class hardware. Enterprise class hardware can handle the I/O required to appropriately run vacuum. We have a customer that is constantly running 5 autovacuum workers on only 28 spindles. We are in the process of upgrading them to 50 spindles at which point I will likely try 10 autovacuum workers. i forgot to mention - i am on 8.1 here. so, VACUUM is not so smart yet. my changes are pretty much random I/O - so tuple header does not contribute to a lot more I/O as i have to read entire blocks anway. this is why i said - it is not that kind of an issue. and no, updating is not a 5 min task ... hans -- Cybertec Schönig Schönig GmbH PostgreSQL Solutions and Support Gröhrmühlgasse 26, A-2700 Wiener Neustadt Tel: +43/1/205 10 35 / 340 www.postgresql-support.de, www.postgresql-support.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] [0/4] Proposal of SE-PostgreSQL patches
Tom Lane wrote:
KaiGai Kohei [EMAIL PROTECTED] writes:
Some of the test fails contains minor differences from expected results,
like:
| SELECT '' AS xxx, *
| FROM J1_TBL t1 (a, b, c) NATURAL JOIN J2_TBL t2 (d, a);
|xxx | a | b | c | d
| -+---+---+--+---
| - | 0 | | zero |
|| 2 | 3 | two | 2
|| 4 | 1 | four | 2
| + | 0 | | zero |
| (3 rows)
Yeah, I remember those. What needs to be looked at here is *why* the
output is changing. For a patch that allegedly does not touch the
planner, it's fairly disturbing that you don't get the same results.
SE-PostgreSQL does not touch the planner, but it modifies given query
to filter violated tuples for the current user.
Thus, the above query is dealt as if the following query is inputed.
SELECT '' AS xxx, *
FROM J1_TBL t1 (a, b, c) NATURAL JOIN J2_TBL t2 (d, a)
ON sepgsql_tuple_perms(t1.security_context, SEPGSQL_PERMS_SELECT, ...)
and sepgsql_tuple_perms(t2.security_context, SEPGSQL_PERMS_SELECT, ...);
sepgsql_tuple_perms() returns true, if the security policy allows user
to access a given tuple. It returns false, if not so.
The result of EXPLAIN shows it more clearly:
| kaigai=# EXPLAIN SELECT '' AS xxx, * FROM J1_TBL t1 (a, b, c) NATURAL JOIN
J2_TBL t2 (d, a);
| QUERY PLAN
|
---
| Hash Join (cost=29023.54..82599.93 rows=1380 width=44)
|Hash Cond: (t2.a = t1.a)
|- Seq Scan on j2_tbl t2 (cost=0.00..53526.05 rows=713 width=8)
| Filter: pg_catalog.sepgsql_tuple_perms(tableoid, security_context,
12288, t2.*)
|- Hash (cost=29018.70..29018.70 rows=387 width=40)
| - Seq Scan on j1_tbl t1 (cost=0.00..29018.70 rows=387 width=40)
|Filter: pg_catalog.sepgsql_tuple_perms(tableoid,
security_context, 12288, t1.*)
| (7 rows)
and, some of them are trivial ones, like:
| SELECT p1.oid, p1.typname
| FROM pg_type as p1
| WHERE p1.typtype in ('b','e') AND p1.typname NOT LIKE E'\\_%' AND NOT
EXISTS
| (SELECT 1 FROM pg_type as p2
|WHERE p2.typname = ('_' || p1.typname)::name AND
| p2.typelem = p1.oid and p1.typarray = p2.oid);
| - oid | typname
| --+-
| - 210 | smgr
| - 705 | unknown
| -(2 rows)
| + oid |typname
| +--+
| + 210 | smgr
| + 705 | unknown
| + 3403 | security_label
| +(3 rows)
Are you sure that the security_label type should not have an array type?
I do not offhand see a good argument for that. If it really shouldn't,
we can change the expected output here, but you've got to make that
case first.
Yes, security_label type should not have an array type.
It is defined with typelem=0 and typarray=0.
The purpose of this type is to represent the new system column of
security attribute (security_context column).
So, I think it is a case that a new expented output should be modified.
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei [EMAIL PROTECTED]
--
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] XIDs and big boxes again ...
Hans-Juergen Schoenig [EMAIL PROTECTED] writes: i forgot to mention - i am on 8.1 here. so, VACUUM is not so smart yet. So even if we added 64-bit xids it wouldn't be useful to you. You would have to update (at which point you get all the other improvements which make it less useful.) Or at the very least rebuild with the patch and dump and reload which is just as hard. my changes are pretty much random I/O - so tuple header does not contribute to a lot more I/O as i have to read entire blocks anway. this is why i said - it is not that kind of an issue. TPCC experiments show that even on random access you get the same performance hit from bloat. I'm not entirely sure what the mechanism is unless it's simply the cache hit rate being hurt by the wasted memory. and no, updating is not a 5 min task ... I do hope you mean 8.1.11 btw. Updating your binaries should be a 5 minute job and there are real bugs fixed in those releases. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Get trained by Bruce Momjian - ask me about EnterpriseDB's PostgreSQL training! -- 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] bloated heapam.h
Alvaro Herrera wrote: Alvaro Herrera wrote: Oops :-( I just noticed that I removed bufmgr.h from bufpage.h, which is a change you had objected to previously :-( However, it seems the right fix is to move BufferGetPageSize and BufferGetPage from bufpage.h to bufmgr.h. +1 It makes more sense. (Digging further, it seems like bufpage.h should also include transam.h in order to get TransactionIdIsNormal ... I start to wonder how many problems of this nature we have on our headers. Without having a way to detect whether the defined macros are valid, it seems hard to check programatically, however.) I attached script which should check it. In first step it runs C preprocessor on each header (postgres.h is included as well). The output from first step is processed again with preprocessor and define.h is included. Define.h contains all used macros in following format: #define SIGABRT NOT_EXPANDED_SIGABRT Main problem is how to generate define.h. I used following magic: grep ^#define `find . -name *.h` | cut -d -f 2 | cut -f 1 | cut -f 1 -d( but it generates some noise as well. Maybe some Perl or AWK magic should be better. Zdenek test.sh Description: application/shellscript -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Stack depth exceeded error
Hello, I have custom postgres code. I get the error below for the query select l_orderkey as a from tpcd.orders, tpcd.lineitem where o_orderkey=l_orderkey and l_partkey100 and l_linestatus='F'; ERROR: stack depth limit exceeded HINT: Increase the configuration parameter max_stack_depth. However, the same code runs fine with one condition in where clause, but fails with the error above in case of multiple conditions. Whats the cause of this error ? I tried increasing the stack limit; but it doesnt help. -- Suresh Iyengar - Be a better friend, newshound, and know-it-all with Yahoo! Mobile. Try it now.
Re: [HACKERS] Stack depth exceeded error
Suresh [EMAIL PROTECTED] writes: Hello, I have custom postgres code. What kind of code is this? The error below is typical if you create new threads in the server. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's 24x7 Postgres 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] Stack depth exceeded error
Hi, The code uses Asynchronous I/O for fetching certain tids. The code works fine if I use only one condition in where condition, but fails if I use multiple condition. -- Suresh Iyengar Gregory Stark [EMAIL PROTECTED] wrote: Suresh writes: Hello, I have custom postgres code. What kind of code is this? The error below is typical if you create new threads in the server. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's 24x7 Postgres support! - Be a better friend, newshound, and know-it-all with Yahoo! Mobile. Try it now.
Re: [HACKERS] constraint exclusion analysis caching
Andrew, * Andrew Dunstan ([EMAIL PROTECTED]) wrote: For example, because it put *my* address in the list for your message above, it caused my MUA quite correctly to add a To: line to myself, which I certainly didn't want to do. Honestly, I suspect thunderbird just doesn't know your addresses if it's adding your address back in. Adding your address isn't for you- it's for other people. The, completely reasonable, assumption is that if your address was included in a To or Cc that you're not on the list and stripping that out would mean you'd be left out. And it's completely unnecessary. For example, I have set my majordomo preferences for the postgresql.org lists not to send me copies of emails where I am also in the To: or Cc: lines. After doing that I get no duplicates. This doesn't help at all, actually. As I pointed out previously, I *want* the mail through the list, what I *don't* want is people sending list mail directly to me. And I don't casue anyone else to have to edit the addresses when they reply to my mail. Are you sure thunderbird recognizes the email address you use for posting as a local identity/account? Mutt has a specific 'alternates' configuration to let it know what addresses are local. If you want to ensure that you reply to a list, use an MUA that has a reply-to-list command - I see you use mutt, which has such a command IIRC. Indeed, and it's exactly what I use when replying to list mail. The issue isn't making sure that *I* reply to a list, it's asking other people to reply through the list rather than to me. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] constraint exclusion analysis caching
Stephen Frost wrote: Andrew, * Andrew Dunstan ([EMAIL PROTECTED]) wrote: For example, because it put *my* address in the list for your message above, it caused my MUA quite correctly to add a To: line to myself, which I certainly didn't want to do. Honestly, I suspect thunderbird just doesn't know your addresses if it's adding your address back in. Adding your address isn't for you- it's for other people. The, completely reasonable, assumption is that if your address was included in a To or Cc that you're not on the list and stripping that out would mean you'd be left out. And it's completely unnecessary. For example, I have set my majordomo preferences for the postgresql.org lists not to send me copies of emails where I am also in the To: or Cc: lines. After doing that I get no duplicates. This doesn't help at all, actually. As I pointed out previously, I *want* the mail through the list, what I *don't* want is people sending list mail directly to me. And I don't casue anyone else to have to edit the addresses when they reply to my mail. Are you sure thunderbird recognizes the email address you use for posting as a local identity/account? Mutt has a specific 'alternates' configuration to let it know what addresses are local. If you want to ensure that you reply to a list, use an MUA that has a reply-to-list command - I see you use mutt, which has such a command IIRC. Indeed, and it's exactly what I use when replying to list mail. The issue isn't making sure that *I* reply to a list, it's asking other people to reply through the list rather than to me. a. I don't use Thunderbird. b. Of couse the MUA knows what my address is. c. Yours are pretty much the *only* settings of all the users of this list that cause me issues. Judging by your own words I am not alone in being thus inconvenienced (otherwise, why would an amazing number of people ask you about it?). If you don't care about that then there's nothing much I can do. Alvaro used to have a similar setup. When I complained he very kindly fixed it. d. Your completely reasonable assumption above is, of course, bogus. Most people when replying to a list reply to all adresses. Assuming that the non-list addresses are for people not on the list is nonsense. 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] [0/4] Proposal of SE-PostgreSQL patches
KaiGai Kohei [EMAIL PROTECTED] writes:
Tom Lane wrote:
Yeah, I remember those. What needs to be looked at here is *why* the
output is changing. For a patch that allegedly does not touch the
planner, it's fairly disturbing that you don't get the same results.
SE-PostgreSQL does not touch the planner, but it modifies given query
to filter violated tuples for the current user.
Hmm. Is that really a good idea, compared to hard-wiring the checks
into nodeSeqscan and friends? I didn't look at the query-rewriting
portion of the patch in any detail, but I'd tend not to trust such
a technique very far: getting it right is going to be quite complex
and probably bug prone.
Are you sure that the security_label type should not have an array type?
Yes, security_label type should not have an array type.
You didn't provide one ounce of justification for making it not obey the
expected behavior, so I'm not accepting this position. It doesn't seem
to me to be all that unlikely that users would want to compute with
arrays of security labels. As an example:
select ... where security_label in ('foo', 'bar')
which will become an = ANY(ARRAY[]) construct under the hood.
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] [0/4] Proposal of SE-PostgreSQL patches
Tom Lane wrote: KaiGai Kohei [EMAIL PROTECTED] writes: Tom Lane wrote: Yeah, I remember those. What needs to be looked at here is *why* the output is changing. For a patch that allegedly does not touch the planner, it's fairly disturbing that you don't get the same results. SE-PostgreSQL does not touch the planner, but it modifies given query to filter violated tuples for the current user. Hmm. Is that really a good idea, compared to hard-wiring the checks into nodeSeqscan and friends? I didn't look at the query-rewriting portion of the patch in any detail, but I'd tend not to trust such a technique very far: getting it right is going to be quite complex and probably bug prone. My eyebrows went up when I read this too. Presumably, if it's hardwired like you suggest then the planner can't take any account of the filter, though. Do we want it to? OTOH, I'm not happy about silently rewriting queries, either - it would make optimising queries a lot harder, I suspect. 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] constraint exclusion analysis caching
Stephen Frost wrote: And it's completely unnecessary. For example, I have set my majordomo preferences for the postgresql.org lists not to send me copies of emails where I am also in the To: or Cc: lines. After doing that I get no duplicates. This doesn't help at all, actually. As I pointed out previously, I *want* the mail through the list, what I *don't* want is people sending list mail directly to me. Wouldn't it make sense, then, to filter any email which is Cc'ed to a list, into that list's folder? Add to that a bit of duplicate removal (say, procmail's, or whatever you use) and you're set. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- 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] [0/4] Proposal of SE-PostgreSQL patches
Andrew Dunstan [EMAIL PROTECTED] writes: Tom Lane wrote: Hmm. Is that really a good idea, compared to hard-wiring the checks into nodeSeqscan and friends? My eyebrows went up when I read this too. Presumably, if it's hardwired like you suggest then the planner can't take any account of the filter, though. Do we want it to? Well, the planner could have hardwired knowledge about the existence of the hardwired filters --- if anything, that'd probably be easier than hacking it to have a similar level of knowledge about generic-looking function calls. But in reality, since we don't know at plan time which userid will execute the constructed plan, I'm not sure we could come up with useful estimates 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] [COMMITTERS] pgsql: Convert wal_sync_method to guc enum.
Magnus Hagander wrote: Tom Lane wrote: [EMAIL PROTECTED] (Magnus Hagander) writes: Convert wal_sync_method to guc enum. Buildfarm says you broke things for Windows. Yeah, working on that with Dave. First part was to unbreak the error message so we can actually figure out what's broken :-( I need to leave for a couple of hours, will look again when I get back. But so far, I'm quite surprised. Here's my reasoning, please poke holes in it :-) 1) Win32 always defines O_DSYNC (win32.h) 2) That means we should always define OPEN_DATASYNC_FLAG (xlogdefs.h, line 107) 3) That means that the error should not happen at all, because of xlog.c line 6358. Anybody who can kill this argument before I get back ;-) It's obviously flawed somewhere... //Magnus -- 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] bloated heapam.h
Tom Lane wrote: Alvaro Herrera [EMAIL PROTECTED] writes: Oops :-( I just noticed that I removed bufmgr.h from bufpage.h, which is a change you had objected to previously :-( http://archives.postgresql.org/pgsql-patches/2008-04/msg00149.php Hmm. I did notice that the patch seemed to need to add bufmgr.h inclusions to an awful lot of files, but I'd forgotten the argument about the bufpage.h macros needing it. Do you want to undo that aspect of it? Done -- I put back bufmgr.h into bufpage.h. Surely there is a better structure for this, perhaps involving more header files, but I don't have time for that ATM. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- 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] bloated heapam.h
Zdenek Kotala wrote: Alvaro Herrera wrote: (Digging further, it seems like bufpage.h should also include transam.h in order to get TransactionIdIsNormal ... I start to wonder how many problems of this nature we have on our headers. Without having a way to detect whether the defined macros are valid, it seems hard to check programatically, however.) I attached script which should check it. In first step it runs C preprocessor on each header (postgres.h is included as well). The output from first step is processed again with preprocessor and define.h is included. Define.h contains all used macros in following format: #define SIGABRT NOT_EXPANDED_SIGABRT Main problem is how to generate define.h. I used following magic: grep ^#define `find . -name *.h` | cut -d -f 2 | cut -f 1 | cut -f 1 -d( but it generates some noise as well. Maybe some Perl or AWK magic should be better. So were you able to detect anything bogus with this technique? -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- 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] Setting a pre-existing index as a primary key
Joshua D. Drake wrote: Tom Lane wrote: Well it should be optional but it would be nice if we had the option to have it renamed per the default... meaning the same output if I were to do this: If you want that, you can rename the index (either before or afterwards). I don't see any reason to clutter the make-constraint-from-index command with questions of renaming. As a counter point, I don't see any reason to make the DBA's life harder. Sure it is just one step but it is a human step, prone to error and taking more time than it should. Why not just make it easy? Especially when the easy isn't sacrificing data integrity or quality of product? I realize most feel we don't need to add a rename to this, but there are two more reasons _not_ to do this. First, there is the possibility of name collision with the new name so you would then require the user to use the option not to rename. Plus, if you renamed, the old index name would go away, and some people might think the index was removed and not realize it was renamed, or find it confusing it was renamed. -- Bruce Momjian [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- 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] ecpg crash
On Sat, May 10, 2008 at 12:14:57AM -0300, Euler Taveira de Oliveira wrote: While i'm working on a ecpg patch I found a bug in ecpg code. The simple program above could reproduce it. But basically it crashes (segfault) It appeared that the whole error checking code was missing. Fixed now. Thanks for reporting this. Michael -- Michael Meskes Email: Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: [EMAIL PROTECTED] Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] another ecpg crash
On Sun, May 11, 2008 at 01:50:22AM -0300, Euler Taveira de Oliveira wrote: I found another bug when using 'exec sql include filename'. If you use a filename that doesn't exist, ecpg crashes while trying to close a null pointer. The above test case shows it. A possible fix is attached. Thanks again, I just committed the fix. Michael -- Michael Meskes Email: Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: [EMAIL PROTECTED] Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Syntax decisions for pl/pgsql RAISE extension
I've started to look over Pavel's revised RAISE patch http://archives.postgresql.org/pgsql-patches/2008-05/msg00187.php and I've got a few quibbles with the syntax choices. Pavel proposes extending RAISE like this: RAISE level 'format' [, expression [, ...] ] [ USING ( option = value [, ... ] ) ] the part before USING being what we had already. Each option keyword is one of SQLSTATE, CONDITION, DETAIL, or HINT, and each value is a string-valued expression. SQLSTATE takes a value like '22012' while the (mutually exclusive) CONDITION takes a value like 'DIVISION_BY_ZERO'. DETAIL and HINT allow those parts of an error report to be filled in. I'd like to propose the following changes: 1. The parentheses around the USING list seem useless; let's drop 'em. 2. I think the separation between SQLSTATE and CONDITION is just complication. A SQLSTATE is required to be exactly 5 digits and/or upper case ASCII letters; I see no realistic prospect that any condition name would ever look like a SQLSTATE (and we could certainly adjust condition names to prevent it, if anyone would make such an unhappy choice). So I think we could unify these options into one. I think CODE might be a better choice for the option name than SQLSTATE (since the latter already has a meaning in pl/pgsql, ie the function that gives you the code for the currently thrown error) --- thoughts? 3. I think we should allow the user to specify the error message the same way as the other options, that is RAISE level USING MESSAGE = string_expression [ , ... ] The %-format business has always struck me as a bit weird, and it's much more so if we aren't handling the other error report components in the same fashion. So we ought to provide an alternative that's more uniform. Now, the elephant in the room is the issue of Oracle compatibility. None of this looks anything even a little bit like Oracle's RAISE command. Oracle allows RAISE exception_name ; RAISE ; where the second case is allowed only in an EXCEPTION handler and means to re-throw the current error. I think the latter is a very good idea and we ought to support it. Right now there's no way to re-throw an error without information loss, and that'll get a lot worse with these additions to what RAISE can throw. I'm less excited about the condition-name-only syntax; that seems awfully impoverished given the lack of any way to supply a specific error message or data values. Still, we could imagine people wanting something like RAISE condition_name USING message = string_expression where the condition_name would substitute for the CODE option. I think we could support this as long as the condition name were given as an exception name rather than a string literal (otherwise it looks too much like our legacy syntax). Comments? Is anyone excited about that one way or the other? Lastly: to allow users to catch errors thrown with user-defined SQLSTATEs, Pavel proposes extending the syntax of EXCEPTION WHEN lists so that an error code can be specified in either of these styles: DIVISION_BY_ZERO SQLSTATE 22012 I find the second style rather weird, and I think it probably doesn't even work for cases like 2201F (which isn't going to get lexed as a single token). I would suggest a quoted literal and drop the noise word, so that the alternatives are DIVISION_BY_ZERO '22012' Comments? If we can get some consensus I'll undertake to adjust the patch accordingly. 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] Setting a pre-existing index as a primary key
Bruce Momjian [EMAIL PROTECTED] writes: I realize most feel we don't need to add a rename to this, but there are two more reasons _not_ to do this. One other thought I had about this is that the proposed syntax ALTER TABLE tab ADD PRIMARY KEY (col [, ...]) USING INDEX foo is not well chosen anyway. It forces the user to provide a column name list matching the index, which is just extra typing and extra cognitive burden, and it forces the system to have code checking that this list matches the specified index. So I'm thinking it should look like ALTER TABLE tab ADD PRIMARY KEY USING INDEX foo or maybe just ALTER TABLE tab ADD PRIMARY KEY USING foo This would be a separate grammar production having nothing to do with the ADD CONSTRAINT syntax. It's not ambiguous since the column name list is required in ADD CONSTRAINT. BTW, aside from selecting the index the command would have to verify that the indexed columns are all NOT NULL. We could either have it just throw an error if they aren't, or have it silently try to do an ALTER SET NOT NULL, which would require a table scan. I'm going to argue for the just throw an error choice. I don't like the idea of a utility command that takes exclusive lock and then is either near-instantaneous or slow depending on factors not immediately obvious. 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] Proposal: Integrity check
Zdenek Kotala escribió: Regarding to Robert Mach's work during Google SOC on data integrity check. I would like to improve storage module and implement some Robert's code into the core. Did this go anywhere? -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Setting a pre-existing index as a primary key
Tom Lane wrote: BTW, aside from selecting the index the command would have to verify that the indexed columns are all NOT NULL. We could either have it just throw an error if they aren't, or have it silently try to do an ALTER SET NOT NULL, which would require a table scan. I'm going to argue for the just throw an error choice. I don't like the idea of a utility command that takes exclusive lock and then is either near-instantaneous or slow depending on factors not immediately obvious. +1 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] Syntax decisions for pl/pgsql RAISE extension
Tom Lane [EMAIL PROTECTED] wrote: Now, the elephant in the room is the issue of Oracle compatibility. None of this looks anything even a little bit like Oracle's RAISE command. Oracle allows RAISE exception_name ; RAISE ; I'm probably in the minority, but I care more about SQL/PSM compatibility than Oracle compatibility. I would hope that the ISO standard is at least a gorilla sitting in the corner of the room. If it's not too impractical, a nod toward these would be good: DECLARE condition-name CONDITION FOR SQLSTATE VALUE character-literal SIGNAL condition-name -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] [COMMITTERS] pgsql: Convert wal_sync_method to guc enum.
Magnus Hagander [EMAIL PROTECTED] writes: I need to leave for a couple of hours, will look again when I get back. But so far, I'm quite surprised. Here's my reasoning, please poke holes in it :-) I think you forgot to handle SYNC_METHOD_OPEN_DSYNC in issue_xlog_fsync. If you are going to split SYNC_METHOD_OPEN into two codes, you need to handle both those codes everywhere SYNC_METHOD_OPEN was formerly referenced ... 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] Syntax decisions for pl/pgsql RAISE extension
On Tue, May 13, 2008 at 2:53 AM, Tom Lane [EMAIL PROTECTED] wrote: 1. The parentheses around the USING list seem useless; let's drop 'em. Yes. 2. I think the separation between SQLSTATE and CONDITION is just complication. A SQLSTATE is required to be exactly 5 digits and/or upper case ASCII letters; I see no realistic prospect that any condition name would ever look like a SQLSTATE (and we could certainly adjust condition names to prevent it, if anyone would make such an unhappy choice). So I think we could unify these options into one. I think CODE might be a better choice for the option name than SQLSTATE (since the latter already has a meaning in pl/pgsql, ie the function that gives you the code for the currently thrown error) --- thoughts? Yes. CODE has a nice symmetry with the use of errcode in ereport as well. 3. I think we should allow the user to specify the error message the same way as the other options, that is RAISE level USING MESSAGE = string_expression [ , ... ] The %-format business has always struck me as a bit weird, and it's much more so if we aren't handling the other error report components in the same fashion. So we ought to provide an alternative that's more uniform. I agree that the % formatting in the RAISE message is weird, but it is useful. When you're writing an exception message you almost always want to substitute in information about the values (causing|involved in) the exception. With MESSAGE = string you would have to concatenate the pieces together with ||, which is longer and less readable. I support adding the MESSAGE option (again, nice symmetry with ereport), but will probably continue to use the %-formatted message style in my applications. What would we do if the user specifies a %-formatted message as well as a MESSAGE option? I think it would be reasonable to bail out with a message explaining that they should use the formatted message XOR the MESSAGE option. Now, the elephant in the room is the issue of Oracle compatibility. None of this looks anything even a little bit like Oracle's RAISE command. Oracle allows RAISE exception_name ; RAISE ; where the second case is allowed only in an EXCEPTION handler and means to re-throw the current error. I think the latter is a very good idea and we ought to support it. Right now there's no way to re-throw an error without information loss, and that'll get a lot worse with these additions to what RAISE can throw. Yes! I've wished for a re-throw ability several times in the past. I'm less excited about the condition-name-only syntax; that seems awfully impoverished given the lack of any way to supply a specific error message or data values. Still, we could imagine people wanting something like RAISE condition_name USING message = string_expression where the condition_name would substitute for the CODE option. I think we could support this as long as the condition name were given as an exception name rather than a string literal (otherwise it looks too much like our legacy syntax). Comments? Is anyone excited about that one way or the other? I like RAISE condition_name, can we support it in conjunction with the current syntax? That is: RAISE level [condition] [string literal, [parameter, ...]] [USING [option = value, ...]] Cheers, BJ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] XIDs and big boxes again ...
Hans-Juergen Schoenig wrote: i suggest to introduce a --with-long-xids flag which would give me 62 / 64 bit XIDs per vacuum on the entire database. this should be fairly easy to implement. i am not too concerned about the size of the tuple header here - if we waste 500 gb of storage here i am totally fine. As you say later in the thread, you are on 8.1. Alot of work has gone into reducing the effect, impact and frequency of XID wrap around and vacuuming since then. In 8.3 transactions that don't actually update a table no long use a real XID and autovacuum you no longer need a database wide vacuum to solve the XID wraparound problem, so I think the answer is upgrade to 8.3 and see if you still have this problem. Matthew O'Connor -- 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] Syntax decisions for pl/pgsql RAISE extension
Brendan Jurd [EMAIL PROTECTED] writes: I agree that the % formatting in the RAISE message is weird, but it is useful. Sure, I'm not proposing removing it. What would we do if the user specifies a %-formatted message as well as a MESSAGE option? Throw an error (just like if they specified the same option type twice). I like RAISE condition_name, can we support it in conjunction with the current syntax? That is: RAISE level [condition] [string literal, [parameter, ...]] [USING [option = value, ...]] Well, it's sort of a mess because level has to become optional in order to be Oracle-compatible (or PSM-compliant, if Kevin is correct). We could get away with it only if the condition were not allowed to be a string literal, which I guess is tolerable but it's a bit annoying. It would get less annoying if we allowed user-declared exception names. I find the Oracle syntax for those to be spectacularly awful: DECLARE deadlock_detected EXCEPTION; PRAGMA EXCEPTION_INIT(deadlock_detected, -60); but it sounds like SQL/PSM's syntax isn't so bad. I could live with the reported DECLARE condition-name CONDITION FOR SQLSTATE VALUE character-literal However, that's a separate feature and I don't want to get into it as part of the current patch. 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] Syntax decisions for pl/pgsql RAISE extension
2008/5/12 Kevin Grittner [EMAIL PROTECTED]: Tom Lane [EMAIL PROTECTED] wrote: Now, the elephant in the room is the issue of Oracle compatibility. None of this looks anything even a little bit like Oracle's RAISE command. Oracle allows RAISE exception_name ; RAISE ; I'm probably in the minority, but I care more about SQL/PSM compatibility than Oracle compatibility. I would hope that the ISO standard is at least a gorilla sitting in the corner of the room. If it's not too impractical, a nod toward these would be good: DECLARE condition-name CONDITION FOR SQLSTATE VALUE character-literal SIGNAL condition-name -Kevin plpgsql can't be SQL/PSM compatible - it's goal other language plpgpsm, and there is condition declared via standard. Pavel -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Syntax decisions for pl/pgsql RAISE extension
2008/5/12 Tom Lane [EMAIL PROTECTED]: Brendan Jurd [EMAIL PROTECTED] writes: I agree that the % formatting in the RAISE message is weird, but it is useful. Sure, I'm not proposing removing it. What would we do if the user specifies a %-formatted message as well as a MESSAGE option? Throw an error (just like if they specified the same option type twice). I like RAISE condition_name, can we support it in conjunction with the current syntax? That is: RAISE level [condition] [string literal, [parameter, ...]] [USING [option = value, ...]] Well, it's sort of a mess because level has to become optional in order to be Oracle-compatible (or PSM-compliant, if Kevin is correct). We could get away with it only if the condition were not allowed to be a string literal, which I guess is tolerable but it's a bit annoying. It would get less annoying if we allowed user-declared exception names. I find the Oracle syntax for those to be spectacularly awful: DECLARE deadlock_detected EXCEPTION; PRAGMA EXCEPTION_INIT(deadlock_detected, -60); but it sounds like SQL/PSM's syntax isn't so bad. I could live with the reported DECLARE condition-name CONDITION FOR SQLSTATE VALUE character-literal However, that's a separate feature and I don't want to get into it as part of the current patch. regards, tom lane Tom, it's exactly like my patch that you rejected two years ago. http://archives.postgresql.org/pgsql-patches/2005-07/msg00176.php Pavel -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Syntax decisions for pl/pgsql RAISE extension
Pavel Stehule [EMAIL PROTECTED] writes: 2008/5/12 Tom Lane [EMAIL PROTECTED]: It would get less annoying if we allowed user-declared exception names. Tom, it's exactly like my patch that you rejected two years ago. Uh, no, not exactly like --- that patch doesn't have anything to do with the SQL/PSM syntax, and not much with the SQL/PSM semantics. As I read the spec, a condition name isn't a variable and so you can't do runtime assignment to it (and unlike Neil, I don't think you should be able to do so). 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] Syntax decisions for pl/pgsql RAISE extension
2008/5/12 Tom Lane [EMAIL PROTECTED]: I've started to look over Pavel's revised RAISE patch http://archives.postgresql.org/pgsql-patches/2008-05/msg00187.php and I've got a few quibbles with the syntax choices. Pavel proposes extending RAISE like this: RAISE level 'format' [, expression [, ...] ] [ USING ( option = value [, ... ] ) ] the part before USING being what we had already. Each option keyword is one of SQLSTATE, CONDITION, DETAIL, or HINT, and each value is a string-valued expression. SQLSTATE takes a value like '22012' while the (mutually exclusive) CONDITION takes a value like 'DIVISION_BY_ZERO'. DETAIL and HINT allow those parts of an error report to be filled in. I'd like to propose the following changes: 1. The parentheses around the USING list seem useless; let's drop 'em. it hasn't any precedent in PostgreSQL. But option list in parenthesesis 2. I think the separation between SQLSTATE and CONDITION is just complication. A SQLSTATE is required to be exactly 5 digits and/or upper case ASCII letters; I see no realistic prospect that any condition name would ever look like a SQLSTATE (and we could certainly adjust condition names to prevent it, if anyone would make such an unhappy choice). So I think we could unify these options into one. I think CODE might be a better choice for the option name than SQLSTATE (since the latter already has a meaning in pl/pgsql, ie the function that gives you the code for the currently thrown error) --- thoughts? CODE isn't well name. It's too much general. If you would to drop one identifier I prefer CONDITION or some similar (minim. ERRCODE). In plpgsql SQLSTATE is keyword, and in some implementations it's implicit variables too. Using it, it's more readable - more verbose - it's in spirit of PL/SQL. Maybe: CONDITION = expression returning name | SQLSTATE expression returning SQLSTATE. 3. I think we should allow the user to specify the error message the same way as the other options, that is RAISE level USING MESSAGE = string_expression [ , ... ] The %-format business has always struck me as a bit weird, and it's much more so if we aren't handling the other error report components in the same fashion. So we ought to provide an alternative that's more uniform. Now, the elephant in the room is the issue of Oracle compatibility. None of this looks anything even a little bit like Oracle's RAISE command. Oracle allows RAISE exception_name ; RAISE ; where the second case is allowed only in an EXCEPTION handler and means to re-throw the current error. I think the latter is a very good idea and we ought to support it. Right now there's no way to re-throw an error without information loss, and that'll get a lot worse with these additions to what RAISE can throw. I'm less excited about the condition-name-only syntax; that seems awfully impoverished given the lack of any way to supply a specific error message or data values. Still, we could imagine people wanting something like RAISE condition_name USING message = string_expression where the condition_name would substitute for the CODE option. I think we could support this as long as the condition name were given as an exception name rather than a string literal (otherwise it looks too much like our legacy syntax). Comments? Is anyone excited about that one way or the other? I agree with this syntax, but I propose using code only with SQLSTATE keyword RAISE 22345 is ugly RAISE SQLSTATE 22345 is better and on this position can be parametrized - now I thing, so SQLSTATE and CONDITION shouldn't be defined in USING. variants: RAISE unique_violation USING message = '', hint = ''; RAISE SQLSTATE USING message ... RAISE variable USING ... RAISE SQLSTATE USING ... Lastly: to allow users to catch errors thrown with user-defined SQLSTATEs, Pavel proposes extending the syntax of EXCEPTION WHEN lists so that an error code can be specified in either of these styles: DIVISION_BY_ZERO SQLSTATE 22012 I find the second style rather weird, and I think it probably doesn't even work for cases like 2201F (which isn't going to get lexed as a single token). I would suggest a quoted literal and drop the noise word, so that the alternatives are DIVISION_BY_ZERO '22012' Comments? it's true - it's have to quoted literal or other hand, solve it on lexer level. But it's not important on plpgsql - there we can choice the most simple solution. Regards Pavel Stehule If we can get some consensus I'll undertake to adjust the patch accordingly. 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] Syntax decisions for pl/pgsql RAISE extension
2008/5/12 Tom Lane [EMAIL PROTECTED]: Pavel Stehule [EMAIL PROTECTED] writes: 2008/5/12 Tom Lane [EMAIL PROTECTED]: It would get less annoying if we allowed user-declared exception names. Tom, it's exactly like my patch that you rejected two years ago. Uh, no, not exactly like --- that patch doesn't have anything to do with the SQL/PSM syntax, and not much with the SQL/PSM semantics. As I read the spec, a condition name isn't a variable and so you can't do runtime assignment to it (and unlike Neil, I don't think you should be able to do so). In plpgsql I prefer PL/SQL syntax. Mix SQL/PSM and PL/SQL will be mismas. But I like idea, so you can set dynamically SQLSTATE and other params - because you can write own wrapper for RAISE statement. It's can be usable for centralized exception management. I can do it in C, but there are lot of users, that could use only plpgsql. 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] Syntax decisions for pl/pgsql RAISE extension
Kevin Grittner [EMAIL PROTECTED] writes:
I'm probably in the minority, but I care more about SQL/PSM
compatibility than Oracle compatibility.
Well, a different line of attack would be to leave RAISE as-is and adopt
the SQL/PSM syntax for a modernized command. What I'm seeing in Part 4
is
signal statement ::=
SIGNAL signal value
[ set signal information ]
signal value ::=
condition name
| sqlstate value
condition name ::=
identifier
sqlstate value ::=
SQLSTATE [ VALUE ] character string literal
set signal information ::=
SET signal information item list
signal information item list ::=
signal information item [ { comma signal information item
}... ]
signal information item ::=
condition information item name equals operator simple value
specification
If we're willing to invent Postgres-specific condition information item
names for MESSAGE, DETAIL, etc, then this is just about isomorphic to
the proposed RAISE syntax, except that if you want an elog level other
than ERROR you'd have to specify it as an item in the SET-list.
BTW, the spec also uses condition name and sqlstate value as above
in handler declarations, so it looks like both Pavel and I got it wrong
about how to extend the EXCEPTION syntax: it should be
SQLSTATE [VALUE] 'x'
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] Syntax decisions for pl/pgsql RAISE extension
2008/5/12 Tom Lane [EMAIL PROTECTED]:
Kevin Grittner [EMAIL PROTECTED] writes:
I'm probably in the minority, but I care more about SQL/PSM
compatibility than Oracle compatibility.
Well, a different line of attack would be to leave RAISE as-is and adopt
the SQL/PSM syntax for a modernized command. What I'm seeing in Part 4
is
signal statement ::=
SIGNAL signal value
[ set signal information ]
signal value ::=
condition name
| sqlstate value
condition name ::=
identifier
sqlstate value ::=
SQLSTATE [ VALUE ] character string literal
set signal information ::=
SET signal information item list
signal information item list ::=
signal information item [ { comma signal information item
}... ]
signal information item ::=
condition information item name equals operator simple
value specification
If we're willing to invent Postgres-specific condition information item
names for MESSAGE, DETAIL, etc, then this is just about isomorphic to
the proposed RAISE syntax, except that if you want an elog level other
than ERROR you'd have to specify it as an item in the SET-list.
BTW, the spec also uses condition name and sqlstate value as above
in handler declarations, so it looks like both Pavel and I got it wrong
about how to extend the EXCEPTION syntax: it should be
SQLSTATE [VALUE] 'x'
regards, tom lane
I like this syntax, but I am not if it's good idea add new similar
statement. I don't know - but maybe it's can be better then extending
RAISE - and way to get consensus.
Regards
Pavel Stehule
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Syntax decisions for pl/pgsql RAISE extension
2008/5/12 Tom Lane [EMAIL PROTECTED]:
Kevin Grittner [EMAIL PROTECTED] writes:
I'm probably in the minority, but I care more about SQL/PSM
compatibility than Oracle compatibility.
Well, a different line of attack would be to leave RAISE as-is and adopt
the SQL/PSM syntax for a modernized command. What I'm seeing in Part 4
is
signal statement ::=
SIGNAL signal value
[ set signal information ]
signal value ::=
condition name
| sqlstate value
condition name ::=
identifier
sqlstate value ::=
SQLSTATE [ VALUE ] character string literal
set signal information ::=
SET signal information item list
signal information item list ::=
signal information item [ { comma signal information item
}... ]
signal information item ::=
condition information item name equals operator simple
value specification
If we're willing to invent Postgres-specific condition information item
names for MESSAGE, DETAIL, etc, then this is just about isomorphic to
the proposed RAISE syntax, except that if you want an elog level other
than ERROR you'd have to specify it as an item in the SET-list.
BTW, the spec also uses condition name and sqlstate value as above
in handler declarations, so it looks like both Pavel and I got it wrong
about how to extend the EXCEPTION syntax: it should be
SQLSTATE [VALUE] 'x'
next step can be extension of GET DIAGNOSTIC statement ...
Pavel
p.s. CASE statement going from SQL/PSM too. so why not?
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] bloated heapam.h
Alvaro Herrera napsal(a): Zdenek Kotala wrote: Alvaro Herrera wrote: (Digging further, it seems like bufpage.h should also include transam.h in order to get TransactionIdIsNormal ... I start to wonder how many problems of this nature we have on our headers. Without having a way to detect whether the defined macros are valid, it seems hard to check programatically, however.) I attached script which should check it. In first step it runs C preprocessor on each header (postgres.h is included as well). The output from first step is processed again with preprocessor and define.h is included. Define.h contains all used macros in following format: #define SIGABRT NOT_EXPANDED_SIGABRT Main problem is how to generate define.h. I used following magic: grep ^#define `find . -name *.h` | cut -d -f 2 | cut -f 1 | cut -f 1 -d( but it generates some noise as well. Maybe some Perl or AWK magic should be better. So were you able to detect anything bogus with this technique? No, everything looks OK. Zdenek -- 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: Integrity check
Alvaro Herrera napsal(a): Zdenek Kotala escribió: Regarding to Robert Mach's work during Google SOC on data integrity check. I would like to improve storage module and implement some Robert's code into the core. Did this go anywhere? I did not catch May commit fest :(. I plan to send core improvement to next commit fest. Zdenek -- 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] bloated heapam.h
Zdenek Kotala wrote: Alvaro Herrera napsal(a): Zdenek Kotala wrote: I attached script which should check it. In first step it runs C preprocessor on each header (postgres.h is included as well). The output from first step is processed again with preprocessor and define.h is included. So were you able to detect anything bogus with this technique? No, everything looks OK. Well, then it is not working, because transam.h is missing from bufpage.h ... -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Convert wal_sync_method to guc enum.
Tom Lane wrote: Magnus Hagander [EMAIL PROTECTED] writes: I need to leave for a couple of hours, will look again when I get back. But so far, I'm quite surprised. Here's my reasoning, please poke holes in it :-) I think you forgot to handle SYNC_METHOD_OPEN_DSYNC in issue_xlog_fsync. If you are going to split SYNC_METHOD_OPEN into two codes, you need to handle both those codes everywhere SYNC_METHOD_OPEN was formerly referenced ... That was it, fixed. I missed the fact that the same error message occured twice in the file. I blame lack of caffeine comined with having hacked Latex code. It just kills the brain. Thanks for the pointer! //Magnus -- 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] Syntax decisions for pl/pgsql RAISE extension
Pavel Stehule [EMAIL PROTECTED] writes: I like this syntax, but I am not if it's good idea add new similar statement. I don't know - but maybe it's can be better then extending RAISE - and way to get consensus. I looked a bit more at the SQL spec. It already defines a condition information item name MESSAGE_TEXT, which arguably is what we should use for the primary message item, but that seems unpleasantly long for something that's going to be used in pretty much every SIGNAL command. Also there's a question of whether it's supposed to mean the *complete* message delivered to a client, which would subsume DETAIL, HINT, etc in our scheme. So I'm a bit tempted to stick with MESSAGE, DETAIL, and HINT as the settable options if we go with SQL/PSM-derived syntax. We'd also want LEVEL or something to be able to specify non-ERROR elog levels. Also, as to the re-throw-an-error capability, SQL/PSM defines a RESIGNAL command that does this. I propose implementing only the parameterless variant of this, at least for the time being. (The spec appears to intend that RESIGNAL can override selected fields of the error being re-thrown, which doesn't strike me as a terribly good idea --- you could end up with a completely nonsensical error report.) 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] bloated heapam.h
Alvaro Herrera napsal(a): Zdenek Kotala wrote: Alvaro Herrera napsal(a): Zdenek Kotala wrote: I attached script which should check it. In first step it runs C preprocessor on each header (postgres.h is included as well). The output from first step is processed again with preprocessor and define.h is included. So were you able to detect anything bogus with this technique? No, everything looks OK. Well, then it is not working, because transam.h is missing from bufpage.h ... It tried catch problems with defines not with datatypes. If you mean that TransactionID is not defined. I try to play with lint now if it gets better results. Zdenek -- 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] bloated heapam.h
Zdenek Kotala wrote: Alvaro Herrera napsal(a): Well, then it is not working, because transam.h is missing from bufpage.h ... It tried catch problems with defines not with datatypes. If you mean that TransactionID is not defined. No, I mean that TransactionIdIsNormal() is used in PageSetPrunable(). I try to play with lint now if it gets better results. Ok, good. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Fairly serious bug induced by latest guc enum changes
I see that assign_xlog_sync_method() is still assigning to sync_method. This is 100% wrong: an assign hook is chartered to set derived values, but not to set the GUC variable itself. This will for example result in set_config_option() stacking the wrong value (the already-updated one) as the value to roll back to if the transaction aborts. You could just remove the assignment from assign_xlog_sync_method, although it might look a bit odd to be setting open_sync_bit but not sync_method. It also bothers me slightly that the derived and main values wouldn't be set at exactly the same point --- problems inside guc.c might lead to them getting out of sync. Another possibility is to stick with something equivalent to the former design: what GUC thinks is the variable is just a dummy static integer in guc.c, and the assign hook is still setting the real value that the rest of the code looks at. A minor advantage of this second way is that the real value could still be declared as enum rather than int. Please fix this, and take another look at the prior WAL enum changes to see if the same problem hasn't been created elsewhere. 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] Fairly serious bug induced by latest guc enum changes
Tom Lane wrote: I see that assign_xlog_sync_method() is still assigning to sync_method. This is 100% wrong: an assign hook is chartered to set derived values, but not to set the GUC variable itself. This will for example result in set_config_option() stacking the wrong value (the already-updated one) as the value to roll back to if the transaction aborts. Hm. I never quite did get my head around how the stacking work, that's probably why :-) You could just remove the assignment from assign_xlog_sync_method, although it might look a bit odd to be setting open_sync_bit but not sync_method. It also bothers me slightly that the derived and main values wouldn't be set at exactly the same point --- problems inside guc.c might lead to them getting out of sync. Another possibility is to stick with something equivalent to the former design: what GUC thinks is the variable is just a dummy static integer in guc.c, and the assign hook is still setting the real value that the rest of the code looks at. A minor advantage of this second way is that the real value could still be declared as enum rather than int. That value never was an enum, so that part is not an advantage. I still think going with the older method would be the safest, though, for the other reasons. You agree? Please fix this, and take another look at the prior WAL enum changes to see if the same problem hasn't been created elsewhere. (I assume you mean GUC enum here, that seems fairly obvious) The only other assign hooks are assign_syslog_facility and assign_session_replication_role. The changes there are: In these, the value was previously derived from a string and set in the hook. It's now set directly by the GUC code, and the hook only updates other things (setting the actual syslog facility, and resetting the cache, respectively). I think that means there are no bugs there. //Magnus -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] odd output in restore mode
I have just been working on setting up a continuous recovery failover system, and noticed some odd log lines, shown below. (Using 8.3). First note that our parsing of recovery.conf in xlog.c is pretty bad, and at least we need to document the quirks if it's not going to be fixed. log_restartpoints is said to be boolean, but when I set it to an unquoted true I got a fatal error, while a quoted 'on' sets it to false, as seen. Ick. What is more, I apparently managed to get the recovery server to lose a WAL file and hang totally by having a bad recovery.conf. Triple ick. Second, what is all this about .history files? My understanding is that they are not necessary, so surely we should try to stat them to see if they are present before trying to copy them. These lines are going to confuse a lot of people, I suspect (including me). Lastly, not quite related to this output, but in the same general area, should we have an option on pg_standby to allow removing the archive file after it has been restored? cheers andrew LOG: database system was interrupted; last known up at 2008-05-12 15:18:23 EDT LOG: starting archive recovery LOG: log_restartpoints = false LOG: restore_command = '../bin/pg_standby -t ../common_archive/failover ../common_archive %f %p %r ' cp: cannot stat `../common_archive/0001.history': No such file or directory cp: cannot stat `../common_archive/0001.history': No such file or directory cp: cannot stat `../common_archive/0001.history': No such file or directory LOG: restored log file 000100A5.0068.backup from archive LOG: restored log file 000100A5 from archive LOG: automatic recovery in progress LOG: redo starts at 0/A5B0 LOG: restored log file 000100A6 from archive LOG: restored log file 000100A7 from archive LOG: restored log file 000100A8 from archive LOG: restored log file 000100A9 from archive trigger file found LOG: could not open file pg_xlog/000100AA (log file 0, segment 170): No such file or directory LOG: redo done at 0/A968 LOG: restored log file 000100A9 from archive cp: cannot stat `../common_archive/0002.history': No such file or directory cp: cannot stat `../common_archive/0002.history': No such file or directory cp: cannot stat `../common_archive/0002.history': No such file or directory LOG: selected new timeline ID: 2 cp: cannot stat `../common_archive/0001.history': No such file or directory cp: cannot stat `../common_archive/0001.history': No such file or directory cp: cannot stat `../common_archive/0001.history': No such file or directory LOG: archive recovery complete LOG: database system is ready to accept connections LOG: autovacuum launcher started -- 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] bloated heapam.h
Alvaro Herrera napsal(a): I try to play with lint now if it gets better results. Ok, good. Hmm, It generates a lot of unnecessary includes in *.c files. I have checked only couple of them and it seems that they are really unnecessary. A attach output. Unfortunately, it does not detect missing heapam.h from bufpage.h. However, I have not tested all magic switches yet :-). There are also several reports about system headers file, but it could be platform specific warning. Zdenek include.out.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] odd output in restore mode
On Mon, 2008-05-12 at 16:57 -0400, Andrew Dunstan wrote: I have just been working on setting up a continuous recovery failover system, and noticed some odd log lines, shown below. (Using 8.3). Hmmm, well, the first time you use something complex, there are some surprising features, I guess. Most especially the log lines are there to allow production issues to be diagnosed, not to create a beautiful log. Many of the things that look somewhat strange are there for a reason, since a wide range of options and save-your-customers-ass scenarios are covered by the recovery code. Suggestions for improvement are always welcome and you are welcome to suggest doc changes, as many people do. First note that our parsing of recovery.conf in xlog.c is pretty bad, and at least we need to document the quirks if it's not going to be fixed. log_restartpoints is said to be boolean, but when I set it to an unquoted true I got a fatal error, while a quoted 'on' sets it to false, as seen. Ick. Yes, some improvements are definitely due there. What is more, I apparently managed to get the recovery server to lose a WAL file and hang totally by having a bad recovery.conf. Triple ick. Sounds like a bug you should report in the normal way. Correctness is paramount. Or are you confusing the message in the log for file AA with an error? Second, what is all this about .history files? My understanding is that they are not necessary, so surely we should try to stat them to see if they are present before trying to copy them. These lines are going to confuse a lot of people, I suspect (including me). I try to keep it as simple as possible, since much of this code only gets run when you really need it to work. The request for the .history file and the cp are examples of that. Lastly, not quite related to this output, but in the same general area, should we have an option on pg_standby to allow removing the archive file after it has been restored? There already is one, but its more complex than that. (%r) LOG: database system was interrupted; last known up at 2008-05-12 15:18:23 EDT LOG: starting archive recovery LOG: log_restartpoints = false LOG: restore_command = '../bin/pg_standby -t ../common_archive/failover ../common_archive %f %p %r ' cp: cannot stat `../common_archive/0001.history': No such file or directory cp: cannot stat `../common_archive/0001.history': No such file or directory cp: cannot stat `../common_archive/0001.history': No such file or directory LOG: restored log file 000100A5.0068.backup from archive LOG: restored log file 000100A5 from archive LOG: automatic recovery in progress LOG: redo starts at 0/A5B0 LOG: restored log file 000100A6 from archive LOG: restored log file 000100A7 from archive LOG: restored log file 000100A8 from archive LOG: restored log file 000100A9 from archive trigger file found LOG: could not open file pg_xlog/000100AA (log file 0, segment 170): No such file or directory LOG: redo done at 0/A968 LOG: restored log file 000100A9 from archive cp: cannot stat `../common_archive/0002.history': No such file or directory cp: cannot stat `../common_archive/0002.history': No such file or directory cp: cannot stat `../common_archive/0002.history': No such file or directory LOG: selected new timeline ID: 2 cp: cannot stat `../common_archive/0001.history': No such file or directory cp: cannot stat `../common_archive/0001.history': No such file or directory cp: cannot stat `../common_archive/0001.history': No such file or directory LOG: archive recovery complete LOG: database system is ready to accept connections LOG: autovacuum launcher started There is an outstanding Windows issue with pg_standby that your help would be appreciated with, shown on latest commitfest page. It's a Windows issue and I don't maintain a Windows dev environment. -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.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] Fairly serious bug induced by latest guc enum changes
Magnus Hagander [EMAIL PROTECTED] writes: I still think going with the older method would be the safest, though, for the other reasons. You agree? Seems reasonable to me. (I assume you mean GUC enum here, that seems fairly obvious) Sorry, was writing in too much of a hurry. In these, the value was previously derived from a string and set in the hook. It's now set directly by the GUC code, and the hook only updates other things (setting the actual syslog facility, and resetting the cache, respectively). I think that means there are no bugs there. Yeah, that's fine. I think though that I may have created a bug inside GUC itself: the new stacking code could conceivably fail (palloc error) between success return from the assign hook and setting up the stack entry that is needed to undo the assignment on abort. In this situation the assign hook would've made its other thing changes but there is no GUC state to cause the hook to be called again to undo 'em. I need to fix it so that any palloc'ing needed is done before calling the assign hook. 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] odd output in restore mode
Simon Riggs wrote: What is more, I apparently managed to get the recovery server to lose a WAL file and hang totally by having a bad recovery.conf. Triple ick. Sounds like a bug you should report in the normal way. Correctness is paramount. Or are you confusing the message in the log for file AA with an error? No, it had to do with pg_standby waiting for a WAL file that had already gone, somehow. I will try to reproduce it when I get a spare moment. Second, what is all this about .history files? My understanding is that they are not necessary, so surely we should try to stat them to see if they are present before trying to copy them. These lines are going to confuse a lot of people, I suspect (including me). I try to keep it as simple as possible, since much of this code only gets run when you really need it to work. The request for the .history file and the cp are examples of that. I don't follow. AFAICT no .history file was in fact archived. ISTM that in this case we should only call RestoreWALFileForRecovery if the file in fact exists. Lastly, not quite related to this output, but in the same general area, should we have an option on pg_standby to allow removing the archive file after it has been restored? There already is one, but its more complex than that. (%r) I was using %r. But the WAL files that have been restored (according to the log) are still in the archive dir. So it looks like %r isn't working properly. There is an outstanding Windows issue with pg_standby that your help would be appreciated with, shown on latest commitfest page. It's a Windows issue and I don't maintain a Windows dev environment. The patch has been rejected for now, according to the Commitfest page. Not sure what you want my help on. BTW, none of what I reported was on Windows - it's on Linux. 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] odd output in restore mode
On Mon, 2008-05-12 at 18:58 -0400, Andrew Dunstan wrote: No, it had to do with pg_standby waiting for a WAL file that had already gone, somehow. I will try to reproduce it when I get a spare moment. Sounds like the bug I just fixed. There is an outstanding Windows issue with pg_standby that your help would be appreciated with, shown on latest commitfest page. It's a Windows issue and I don't maintain a Windows dev environment. The patch has been rejected for now, according to the Commitfest page. Not sure what you want my help on. Well, the patch was rejected long ago, not sure why its in this commitfest. But its an open issue on the Windows port. -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.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] odd output in restore mode
On Monday 12 May 2008 18:58:37 Andrew Dunstan wrote:
Simon Riggs wrote:
Lastly, not quite related to this output, but in the same general area,
should we have an option on pg_standby to allow removing the archive
file after it has been restored?
There already is one, but its more complex than that. (%r)
I was using %r. But the WAL files that have been restored (according to
the log) are still in the archive dir. So it looks like %r isn't working
properly.
Are you sure you've moved passed the latest restart point? Just because a WAL
file has been processed doesn't mean it can be deleted.
--
Robert Treat
Build A Brighter LAMP :: Linux Apache {middleware} PostgreSQL
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] odd output in restore mode
Simon Riggs wrote: On Mon, 2008-05-12 at 18:58 -0400, Andrew Dunstan wrote: No, it had to do with pg_standby waiting for a WAL file that had already gone, somehow. I will try to reproduce it when I get a spare moment. Sounds like the bug I just fixed. Yes, so I see. I didn't have that fix, so I'll test again with the patch. There is an outstanding Windows issue with pg_standby that your help would be appreciated with, shown on latest commitfest page. It's a Windows issue and I don't maintain a Windows dev environment. The patch has been rejected for now, according to the Commitfest page. Not sure what you want my help on. Well, the patch was rejected long ago, not sure why its in this commitfest. But its an open issue on the Windows port. Surely the right fix is to use the recently implemented pgwin32_safestat() (if we aren't already - I suspect we probably are) and remove the kluge in pg_standby.c. 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] Syntax decisions for pl/pgsql RAISE extension
On Monday 12 May 2008 14:40:46 Pavel Stehule wrote:
2008/5/12 Tom Lane [EMAIL PROTECTED]:
Pavel Stehule [EMAIL PROTECTED] writes:
2008/5/12 Tom Lane [EMAIL PROTECTED]:
It would get less annoying if we allowed user-declared exception names.
Tom, it's exactly like my patch that you rejected two years ago.
Uh, no, not exactly like --- that patch doesn't have anything to do
with the SQL/PSM syntax, and not much with the SQL/PSM semantics.
As I read the spec, a condition name isn't a variable and so you can't
do runtime assignment to it (and unlike Neil, I don't think you should
be able to do so).
In plpgsql I prefer PL/SQL syntax. Mix SQL/PSM and PL/SQL will be
mismas. But I like idea, so you can set dynamically SQLSTATE and other
params - because you can write own wrapper for RAISE statement. It's
can be usable for centralized exception management. I can do it in C,
but there are lot of users, that could use only plpgsql.
I think nod's toward PL/SQL compatability should be given in general. If
people want a PSM style language, let's work on getting pl/psm better
maintained or integrated.
--
Robert Treat
Build A Brighter LAMP :: Linux Apache {middleware} PostgreSQL
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] odd output in restore mode
Andrew Dunstan [EMAIL PROTECTED] writes: Simon Riggs wrote: Well, the patch was rejected long ago, not sure why its in this commitfest. But its an open issue on the Windows port. Surely the right fix is to use the recently implemented pgwin32_safestat() (if we aren't already - I suspect we probably are) and remove the kluge in pg_standby.c. I think the open issue is how to know whether pgwin32_safestat fixes the problem that the kluge tried to work around. 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] odd output in restore mode
Robert Treat wrote: On Monday 12 May 2008 18:58:37 Andrew Dunstan wrote: Simon Riggs wrote: Lastly, not quite related to this output, but in the same general area, should we have an option on pg_standby to allow removing the archive file after it has been restored? There already is one, but its more complex than that. (%r) I was using %r. But the WAL files that have been restored (according to the log) are still in the archive dir. So it looks like %r isn't working properly. Are you sure you've moved passed the latest restart point? Just because a WAL file has been processed doesn't mean it can be deleted. Thanks. It wasn't that, but when I ran with the very latest patches this problem went away. 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] Syntax decisions for pl/pgsql RAISE extension
Robert Treat [EMAIL PROTECTED] writes: On Monday 12 May 2008 14:40:46 Pavel Stehule wrote: In plpgsql I prefer PL/SQL syntax. I think nod's toward PL/SQL compatability should be given in general. This position seems just about entirely unhelpful for resolving the problem at hand, because PL/SQL hasn't *got* syntax that does what we want. It might lead us to favor RAISE without parameter over RESIGNAL, but that's a pretty trivial point 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] odd output in restore mode
Tom Lane wrote: Andrew Dunstan [EMAIL PROTECTED] writes: Simon Riggs wrote: Well, the patch was rejected long ago, not sure why its in this commitfest. But its an open issue on the Windows port. Surely the right fix is to use the recently implemented pgwin32_safestat() (if we aren't already - I suspect we probably are) and remove the kluge in pg_standby.c. I think the open issue is how to know whether pgwin32_safestat fixes the problem that the kluge tried to work around. Well, I think we need to consider quite a number of scenarios. The archive directory could be local, on a remote Windows machine, or on a remote Samba server. The archive file could be copied by Windows copy, or Unix cp, or scp, or rsync, among others. I'd like to know the setup that was found to produce the error, to start with. 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] Syntax decisions for pl/pgsql RAISE extension
2008/5/12 Tom Lane [EMAIL PROTECTED]: Pavel Stehule [EMAIL PROTECTED] writes: I like this syntax, but I am not if it's good idea add new similar statement. I don't know - but maybe it's can be better then extending RAISE - and way to get consensus. I looked a bit more at the SQL spec. It already defines a condition information item name MESSAGE_TEXT, which arguably is what we should use for the primary message item, but that seems unpleasantly long for something that's going to be used in pretty much every SIGNAL command. Also there's a question of whether it's supposed to mean the *complete* message delivered to a client, which would subsume DETAIL, HINT, etc in our scheme. So I'm a bit tempted to stick with MESSAGE, DETAIL, and HINT as the settable options if we go with SQL/PSM-derived syntax. We'd also want LEVEL or something to be able to specify non-ERROR elog levels. I agree Also, as to the re-throw-an-error capability, SQL/PSM defines a RESIGNAL command that does this. I propose implementing only the parameterless variant of this, at least for the time being. (The spec appears to intend that RESIGNAL can override selected fields of the error being re-thrown, which doesn't strike me as a terribly good idea --- you could end up with a completely nonsensical error report.) ok regards, tom lane who write this patch? Pavel -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] odd output in restore mode
On Mon, 2008-05-12 at 23:03 -0400, Andrew Dunstan wrote: Tom Lane wrote: Andrew Dunstan [EMAIL PROTECTED] writes: Simon Riggs wrote: Well, the patch was rejected long ago, not sure why its in this commitfest. But its an open issue on the Windows port. Surely the right fix is to use the recently implemented pgwin32_safestat() (if we aren't already - I suspect we probably are) and remove the kluge in pg_standby.c. I think the open issue is how to know whether pgwin32_safestat fixes the problem that the kluge tried to work around. Well, I think we need to consider quite a number of scenarios. The archive directory could be local, on a remote Windows machine, or on a remote Samba server. The archive file could be copied by Windows copy, or Unix cp, or scp, or rsync, among others. I'd like to know the setup that was found to produce the error, to start with. It's a race condition, not a deterministic bug with recreatable conditions. My understanding is the current code was introduced to work around the implementation of stat on Windows which says the filesize is correct even while it is still copying it. The 1sec delay fixed that but is clearly not a foolproof fix and introduces a delay also, which was the original complaint. -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Problem returning strings with pgsql 8.3.x
Having posted this to -general [1] per -hackers list instructions [2]
to try elsewhere first, and waited (not very long, I admit) in vain
for a response, I'm posting this to -hackers now. My apologies if my
impatience in that regard annoys.
While developing PL/LOLCODE, I've found something wrong with returning
strings from LOLCODE functions using 8.3.0 or greater. Using 8.4beta
from a few days ago, for instance, a function that should return test
string returns
\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F in
pgsql (sometimes the number of \x7F characters varies). In 8.2.4 it
works fine.
Here's the code involved, from pl_lolcode_call_handler, the call
handler function for PL/LOLCODE. First, the bit that finds the
FmgrInfo structure and typioparam for the result type:
procTup = SearchSysCache(PROCOID,
ObjectIdGetDatum(fcinfo-flinfo-fn_oid), 0, 0, 0);
if (!HeapTupleIsValid(procTup)) elog(ERROR, Cache lookup
failed for procedure %u, fcinfo-flinfo-fn_oid);
procStruct = (Form_pg_proc) GETSTRUCT(procTup);
typeTup = SearchSysCache(TYPEOID,
ObjectIdGetDatum(procStruct-prorettype), 0, 0, 0);
if (!HeapTupleIsValid(typeTup)) elog(ERROR, Cache lookup
failed for type %u, procStruct-prorettype);
typeStruct = (Form_pg_type) GETSTRUCT(typeTup);
resultTypeIOParam = getTypeIOParam(typeTup);
fmgr_info_cxt(typeStruct-typinput, flinfo,
TopMemoryContext); /*CurTransactionContext); */
ReleaseSysCache(typeTup);
Here's the code that converts the return value into a Datum later on
in the function:
if (returnTypeOID != VOIDOID) {
if (returnVal != NULL) {
if (returnVal-type == ident_NOOB)
fcinfo-isnull = true;
else {
SPI_push();
if (returnTypeOID == BOOLOID)
retval =
InputFunctionCall(flinfo, lolVarGetTroof(returnVal) == lolWIN ?
TRUE : FALSE, resultTypeIOParam, -1);
else {
/* elog(NOTICE,
lolVarGetString(returnVal, true)); */
retval =
InputFunctionCall(flinfo, lolVarGetString(returnVal, true),
resultTypeIOParam, -1);
}
SPI_pop();
}
}
else {
fcinfo-isnull = true;
}
}
SPI_finish();
/* elog(NOTICE, PL/LOLCODE ending); */
return retval;
returnVal is an instance of the struct PL/LOLCODE uses to store its
variables. The key line in this case is the one after the
commented-out call to elog. retval is a Datum type. lolVarGetString()
returns the string value the returnVal struct represents -- I'm
certain of that thanks to gdb and other testing. All other data types
PL/LOLCODE knows about internally seem to return just fine. I'm fairly
certain I'm screwing up memory somewhere, but I can't see what I've
done wrong.
I'm glad to provide further details, but those included above are all
the ones I thought were relevant. Thanks in advance for any help you
can provide.
- Josh / eggyknap
[1] http://archives.postgresql.org/pgsql-general/2008-05/msg00311.php
[2] http://archives.postgresql.org/pgsql-hackers/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
