Re: [PATCHES] Improve shutdown during online backup, take 4
Tom Lane wrote:
> I concur that the messages added to pg_ctl are bizarrely formatted.
> Why would you put a newline in the middle of a sentence, when you
> could equally well emit something like
>
> WARNING: online backup mode is active.
> Shutdown will not complete until pg_stop_backup() is called.
>
> While we're on the subject, the messages added to xlog.c do not
> follow the style guidelines: in particular, errdetail should be
> a complete sentence, and the WARNING is trying to stuff independent
> thoughts into one message. I'd probably do
>
> errmsg("online backup mode cancelled"),
> errdetail("\"%s\" was renamed to \"%s\".", ...
>
> errmsg("online backup mode was not cancelled"),
> errdetail("Failed to rename \"%s\" to \"%s\": %m", ...
Attached is a patch that changes the messages along these lines.
Thanks!
> Lastly, the changes to pmdie's SIGINT handling seem quite bogus.
> Don't you need to transition into WAIT_BACKUP rather than WAIT_BACKENDS
> state in that case too? Shouldn't you do CancelBackup *before*
> PostmasterStateMachine? The thing screams of race conditions.
I suspect there must be a misunderstanding.
You cannot really mean that the postmaster should enter WAIT_BACKUP
state on a fast shutdown request.
Sure, CancelBackup could be called earlier. It doesn't do much more than
rename a file.
My reason for calling it late was that backup mode should really only be
cancelled if we manage to shutdown cleanly, and this is not clear until
the last child is gone.
I cannot see a race condition, except maybe the quite unlikely case
that two instances of CancelBackup might run concurrently, and it
*might* happen that one of them finds that "backup_label" is present but
fails to remove it, because the other one already did.
That would lead to a bogus "backup mode not canceled" message.
Are you referring to that or is there something more fundamental
I fail to see?
Yours,
Laurenz Albe
backup-shut.msg.patch
Description: backup-shut.msg.patch
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Improve shutdown during online backup, take 4
Albe Laurenz wrote:
> Tom Lane wrote:
> > I concur that the messages added to pg_ctl are bizarrely formatted.
> > Why would you put a newline in the middle of a sentence, when you
> > could equally well emit something like
> >
> > WARNING: online backup mode is active.
> > Shutdown will not complete until pg_stop_backup() is called.
> >
> > While we're on the subject, the messages added to xlog.c do not
> > follow the style guidelines: in particular, errdetail should be
> > a complete sentence, and the WARNING is trying to stuff independent
> > thoughts into one message. I'd probably do
> >
> > errmsg("online backup mode cancelled"),
> > errdetail("\"%s\" was renamed to \"%s\".", ...
> >
> > errmsg("online backup mode was not cancelled"),
> > errdetail("Failed to rename \"%s\" to \"%s\": %m", ...
>
> Attached is a patch that changes the messages along these lines.
> Thanks!
Hmm. I've preivously been told not to use "Failed to" but instead use
"Could not"... Didn't notice that Tom used the other one in his
suggestion.
Tom (or someone else) - can you comment on if I misunderstood that
recommendation earlier, or if it still holds? I'll hold back a commit
until someone has commented on it :-)
Also, from this patch, you removed the %m part - I have re-added that
before commit.
(I will not for now comment on the rest of the mail, I'll leave that to
Tom)
//Magnus
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Improve shutdown during online backup, take 4
Tom Lane wrote: > Magnus Hagander <[EMAIL PROTECTED]> writes: > > Hmm. I've preivously been told not to use "Failed to" but instead > > use "Could not"... Didn't notice that Tom used the other one in his > > suggestion. > > > Tom (or someone else) - can you comment on if I misunderstood that > > recommendation earlier, or if it still holds? > > Oh, yes, "could not" is better. My mistake. Ok, thanks. Committed new error messages as such then. //Magnus -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Improve shutdown during online backup, take 4
"Albe Laurenz" <[EMAIL PROTECTED]> writes: > Tom Lane wrote: >> Lastly, the changes to pmdie's SIGINT handling seem quite bogus. >> Don't you need to transition into WAIT_BACKUP rather than WAIT_BACKENDS >> state in that case too? Shouldn't you do CancelBackup *before* >> PostmasterStateMachine? The thing screams of race conditions. > I suspect there must be a misunderstanding. > You cannot really mean that the postmaster should enter WAIT_BACKUP > state on a fast shutdown request. Why not? It'll fall out of the state again immediately in PostmasterStateMachine, no, if we do a CancelBackup here? I don't think these two paths of control should be any more different than really necessary. > Sure, CancelBackup could be called earlier. It doesn't do much more than > rename a file. > My reason for calling it late was that backup mode should really only be > cancelled if we manage to shutdown cleanly, and this is not clear until > the last child is gone. Well, if there were anything conditional about calling it, then maybe that argument would hold some water, but the way you've got it here it *will* get called anyway, just after the PostmasterStateMachine call ... except in the corner case where there were no child processes left and so PostmasterStateMachine manages to decide it's ready to exit(). So far from implementing the logic you suggest, this arrangement never gets it right, and has a race condition case in which it gets it exactly backward. The other reason for the remark about race conditions is that the PostmasterStateMachine call should absolutely be the last thing that pmdie() does --- putting anything after it is wrong, especially things that might alter the PM state, as indeed CancelBackup could. The reason for having that in the signal handler is to cover the possibility that no such call will occur immediately when we return to the wait loop. In general all of the condition handlers in postmaster.c should be of the form "respond to the immediate condition, and then let PostmasterStateMachine decide if there's anything else to do". If you actually want the behavior you propose, then the only correct way to implement it is to embed it into the state machine logic, ie, do the CancelBackup inside PostmasterStateMachine in some state transition taken after the last child is gone. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Improve shutdown during online backup, take 4
Magnus Hagander <[EMAIL PROTECTED]> writes: > Hmm. I've preivously been told not to use "Failed to" but instead use > "Could not"... Didn't notice that Tom used the other one in his > suggestion. > Tom (or someone else) - can you comment on if I misunderstood that > recommendation earlier, or if it still holds? Oh, yes, "could not" is better. My mistake. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] lc_time and localized dates
Euler Taveira de Oliveira wrote: > Here is an updated patch. It follows the Oracle behaviour and uses a > cache mechanism to avoid calling setlocale() all the time. I unified the > localized_* and str_* functions. I didn't test it on Windows. I would > appreciate some feedback. Added to May commitfest. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Improve shutdown during online backup, take 4
Tom Lane wrote: >>> Lastly, the changes to pmdie's SIGINT handling seem quite bogus. >>> Don't you need to transition into WAIT_BACKUP rather than WAIT_BACKENDS >>> state in that case too? Shouldn't you do CancelBackup *before* >>> PostmasterStateMachine? The thing screams of race conditions. > >> I suspect there must be a misunderstanding. >> You cannot really mean that the postmaster should enter WAIT_BACKUP >> state on a fast shutdown request. > > Why not? It'll fall out of the state again immediately in > PostmasterStateMachine, no, if we do a CancelBackup here? I don't think > these two paths of control should be any more different than really > necessary. We cannot call CancelBackup there because that's exactly the state in which a smart shutdown waits for a superuser to issue pg_stop_backup(). > Well, if there were anything conditional about calling it, then maybe > that argument would hold some water, but the way you've got it here it > *will* get called anyway, just after the PostmasterStateMachine call [...] I see. > The other reason for the remark about race conditions is that the > PostmasterStateMachine call should absolutely be the last thing that > pmdie() does --- putting anything after it is wrong, especially things > that might alter the PM state, as indeed CancelBackup could. I see that too. Thanks for explaining. > If you actually want the behavior you propose, then the only correct way > to implement it is to embed it into the state machine logic, ie, do the > CancelBackup inside PostmasterStateMachine in some state transition > taken after the last child is gone. I've attached a patch that works for me. I hope I got it right. Yours, Laurenz Albe cancel_backup.patch Description: cancel_backup.patch -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Improve shutdown during online backup, take 4
"Albe Laurenz" <[EMAIL PROTECTED]> writes: > Tom Lane wrote: >> Why not? It'll fall out of the state again immediately in >> PostmasterStateMachine, no, if we do a CancelBackup here? > We cannot call CancelBackup there because that's exactly the state > in which a smart shutdown waits for a superuser to issue pg_stop_backup(). Er, I was complaining about the fast-shutdown code path, not the smart-shutdown one. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] 64-bit CommandIds
So, is this an option we want for configure? --- Zoltan Boszormenyi wrote: > Alvaro Herrera ?rta: > > Zoltan Boszormenyi wrote: > > > > > >> attached is our patch against HEAD which enables extending CommandIds > >> to 64-bit. This is for enabling long transactions that really do that much > >> non-read-only work in one transaction. > >> > > > > I think you should add a pg_control field and corresponding check, to > > avoid a 64bit-Cid postmaster to start on a 32bit-Cid data area and vice > > versa. > > > > I added the check but I needed to add it BEFORE checking for > toast_max_chunk_size otherwise it complained about this more > cryptic problem. I think it's cleaner to report this failure to know > why toast_max_chunk_size != TOAST_MAX_CHUNK_SIZE. > > Best regards, > Zolt?n B?sz?rm?nyi > > -- > -- > Zolt?n B?sz?rm?nyi > Cybertec Sch?nig & Sch?nig GmbH > http://www.postgresql.at/ > > > -- > Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) > To make changes to your Subscription: > http://mail.postgresql.org/mj/mj_wwwusr?domain=postgresql.org&extra=pgsql-patches -- 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-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Sun Studio on Linux spinlock patch
So, is this a feature we want? --- Julius Stroffek wrote: > Tom Lane wrote: > > This patch seems broken in a number of ways. Why are you removing > > -DLINUX_PROFILE, for example? Are you sure you don't need -D_GNU_SOURCE? > > And why add -DSUNOS4_CC, which is a Solaris-specific define (not that > > we seem to be using it anywhere anymore)? Do we really have to have a > > configure-time probe to detect this particular compiler? > > > You are right, removing -DLINUX_PROFILE seems to break profiling > on linux when compiled with sun studio. > > I am not quite sure about the desired usage of _GNU_SOURCE and SUNOS4_CC > macros. > I would not expect _GNU_SOURCE to be defined when compiling sources with > Sun Studio. > I am not quite sure why SUNOS4_CC was supposed to be defined at all for > Solaris as well. > There are already enough macros defined -- "__sun" is defined on Solaris > by both Sun > Studio and gcc and "__SUNPRO_C" is defined by Sun Studio on both Linux > and Solaris. > > Should we then remove _GNU_SOURCE and SUNOS4_CC macro definitions from > the build scripts since they are not used at all in the source code? > > Configure-time probe for sun studio is required to create tas.s link to > the proper > file - sunstudio_x86.s (or sunstudio_sparc.s). This is done during a run > of a configure > script based on settings for the platform. Since these settings may vary > on the same platform > based on the compiler we need to have a configure-time probe. > > But I guess the *real* question is why anyone would care ... what > > benefit is there to using Sun's compiler on Linux? > > > Some tools bundled with sun studio might be used. I personally run into this > when I wanted to debug postgres with sun studio ide and wanted to compile > it first. It is based on netbeans, written in java so it needs a big > enough memory, > however it offers a great possibility to explore postgres internals during > a query execution, etc. It is especially useful, if you do not know what > you are > interested in during a compilation. I am using this to step over join > order search > plugins. I mostly use Solaris for this but I switched to linux for a while. > > I wrote a blog with more details about this. > http://blogs.sun.com/databases/entry/debugging_postgresql_backends_on_solaris > There is also a screenshot showing how it looks in action > http://mediacast.sun.com/users/%7Ejulo/media/pgss_debugging.png > > Also, there was some message a while back on pgsql-bugs from Len Zaifman > requesting > this as well. > > Cheers > > Julo > > -- > Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-patches -- 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-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] 64-bit CommandIds
Bruce Momjian <[EMAIL PROTECTED]> writes: > So, is this an option we want for configure? I think the case for it got a whole lot weaker in 8.3, with lazy consumption of CIDs. If someone had tables big enough to make the 32-bit-CID limit still be a problem despite that fix, I'd think they'd not be very happy about adding another 4 bytes of tuple header overhead (or more likely 8 bytes, on the kind of machine where this patch would make some sense). I don't foresee many people paying that cost rather than breaking up their transactions. My feeling is we should avoid the extra complexity, at least till such time as we see whether there are still any real field complaints about this with 8.3. In any case the patch is broken by --enable-float8-byval, and would need some adjustments to play nice with that. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] 64-bit CommandIds
Tom Lane wrote: > Bruce Momjian <[EMAIL PROTECTED]> writes: > > So, is this an option we want for configure? > > I think the case for it got a whole lot weaker in 8.3, with lazy > consumption of CIDs. If someone had tables big enough to make the > 32-bit-CID limit still be a problem despite that fix, I'd think they'd > not be very happy about adding another 4 bytes of tuple header overhead > (or more likely 8 bytes, on the kind of machine where this patch would > make some sense). I don't foresee many people paying that cost rather > than breaking up their transactions. > > My feeling is we should avoid the extra complexity, at least till such > time as we see whether there are still any real field complaints about > this with 8.3. > > In any case the patch is broken by --enable-float8-byval, and would > need some adjustments to play nice with that. Agreed. Let's see if we get requests for it in >= 8.3 releases. -- 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-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] 64-bit CommandIds
Bruce Momjian wrote: > > I think the case for it got a whole lot weaker in 8.3, with lazy > > consumption of CIDs. > > Agreed. Let's see if we get requests for it in >= 8.3 releases. In the original submission message you find this text: : attached is our patch against HEAD which enables extending CommandIds : to 64-bit. This is for enabling long transactions that really do that : much non-read-only work in one transaction. Question for Hans-Juergen and Zoltan: have you tested 8.3 and do you still see the need for this? -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
[PATCHES] ADD COLUMN with PRIMARY KEY bug (was: I think this is a BUG?)
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
Hi,
The attached patch resolves the bug reported by Kaloyan Iliev [1] on -general.
The problem occurred when executing ALTER TABLE ... ADD COLUMN ...
PRIMARY KEY with no default clause, on a table with rows already
present. The NOT NULL constraint was not enforced. The bug has been
observed on (at least) 8.2, 8.3 and CVS HEAD.
The fix was to force evaluation of the NOT NULL constraint in
ATExecAddColumn in all cases where the parser indicated that the
column ought to be NOT NULL.
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.7 (GNU/Linux)
Comment: http://getfiregpg.org
iD8DBQFIENpl5YBsbHkuyV0RApP6AKDdnZAA0LjSzh9XdiQt7K/cx4YOcQCgydHr
6BYSD2kVX2DP7LYgDHvzNdE=
=Yrmp
-END PGP SIGNATURE-
On Fri, Apr 25, 2008 at 3:46 AM, Tom Lane <[EMAIL PROTECTED]> wrote:
> alter table t1 add column f2 int not null;
>
> transformAlterTableStmt will produce an AT_AddColumn subcommand
> containing a ColumnDef with is_not_null = false, followed by an
> AT_SetNotNull subcommand. But for
>
I realised that there's no reason for preparing a separate SetNotNull
subcommand anymore, now that ATExecAddColumn takes care of enforcing
the constraint, so I removed this special case.
I've also added a regression test for ADD COLUMN PRIMARY KEY.
Cheers,
BJ
[1] http://archives.postgresql.org/message-id/[EMAIL PROTECTED]
*** src/backend/commands/tablecmds.c
--- src/backend/commands/tablecmds.c
***
*** ,3338 ATExecAddColumn(AlteredTableInfo *tab, Relation rel,
--- ,3345
}
/*
+* Tell Phase 3 to perform the check for NULL values in the new column
if
+* its attnotnull bit has been set to true.
+*/
+ if (attribute->attnotnull)
+ tab->new_notnull = true;
+
+ /*
* Add needed dependency entries for the new column.
*/
add_column_datatype_dependency(myrelid, i, attribute->atttypid);
*** src/backend/parser/parse_utilcmd.c
--- src/backend/parser/parse_utilcmd.c
***
*** 1726,1753 transformAlterTableStmt(AlterTableStmt *stmt, const char
*queryString)
* If the column has a non-null
default, we can't skip
* validation of foreign keys.
*/
! if (((ColumnDef *)
cmd->def)->raw_default != NULL)
skipValidation = false;
newcmds = lappend(newcmds, cmd);
/*
-* Convert an ADD COLUMN ... NOT NULL
constraint to a
-* separate command
-*/
- if (def->is_not_null)
- {
- /* Remove NOT NULL from
AddColumn */
- def->is_not_null = false;
-
- /* Add as a separate
AlterTableCmd */
- newcmd =
makeNode(AlterTableCmd);
- newcmd->subtype = AT_SetNotNull;
- newcmd->name =
pstrdup(def->colname);
- newcmds = lappend(newcmds,
newcmd);
- }
-
- /*
* All constraints are processed in
other ways. Remove the
* original list
*/
--- 1726,1737
* If the column has a non-null
default, we can't skip
* validation of foreign keys.
*/
! if (def->raw_default != NULL)
skipValidation = false;
newcmds = lappend(newcmds, cmd);
/*
* All constraints are processed in
other ways. Remove the
* original list
*/
*** src/test/regress/expected/alter_table.out
--- src/test/regress/expected/alter_table.out
***
*** 505,510 create table atacc1 ( test int );
--- 505,522
alter table atacc1 add constraint atacc_test1 primary key (test1);
ERROR: column "test1" named in key does not exist
drop table atacc1;
+ -- Adding a new column as primary key to a non-empty table. Should fail
unless
+ -- the column has a default value.
+ create table atacc1 ( test int );
+ i
Re: [PATCHES] ADD COLUMN with PRIMARY KEY bug (was: I think this is a BUG?)
"Brendan Jurd" <[EMAIL PROTECTED]> writes: > I realised that there's no reason for preparing a separate SetNotNull > subcommand anymore, now that ATExecAddColumn takes care of enforcing > the constraint, so I removed this special case. This part seems to me to be code beautification, not a bug-fix, and shouldn't be back-patched (particularly in view of the fact that we haven't tested the change all that much --- there might be other places depending on the old behavior). Will take care of this. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] ADD COLUMN with PRIMARY KEY bug (was: I think this is a BUG?)
"Brendan Jurd" <[EMAIL PROTECTED]> writes: > The attached patch resolves the bug reported by Kaloyan Iliev [1] on -general. Applied as two separate patches (bug fix and inessential cleanup). The bug turns out to date back to the original addition of support for ALTER ... ADD COLUMN ... DEFAULT/NOT NULL in 8.0. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Sun Studio on Linux spinlock patch
Bruce Momjian <[EMAIL PROTECTED]> writes: > So, is this a feature we want? I have no objection to being able to use Sun Studio, but the submitted patch seemed to need a lot of work yet ... regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
