Re: [PATCHES] [EMAIL PROTECTED]: Re: [BUGS] Problem identifying constraints which should not be inherited]
"Alex Hunsaker" <[EMAIL PROTECTED]> writes:
> Currently this loops through all the constraints for a relation (old
> behavior of MergeAttributesIntoExisting)... Do you think its worth
> adding a non-unique index to speed this up?
No. If we were to refactor pg_constraint as I mentioned earlier,
then it could have a natural primary key (reloid, constrname)
(replacing the existing nonunique index on reloid) and then a number
of things could be sped up. But just piling more indexes on a
fundamentally bad design doesn't appeal to me ...
Will review the revised patch today.
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] column level privileges
Stephen Frost <[EMAIL PROTECTED]> writes: > * Tom Lane ([EMAIL PROTECTED]) wrote: >> I'm not sure where we go from here. Your GSOC student has disappeared, >> right? Is anyone else willing to take up the patch and work on it? > I'm willing to take it up and work on it. Excellent! As you say, you've seen that code before, so it should go more quickly for you than most people. >> One possible solution is to add a flag field >> to TargetEntry to carry the information forward. > I'll look into this, I liked the bitmap idea, personally. Yeah, I do too. What I am thinking now is that we need two bitmaps per RTE: one showing the columns explicitly referenced (hence needing SELECT permission) and one showing the columns assigned to (hence needing INSERT or UPDATE as appropriate --- we will never have both cases in one Query, so we don't need two bitmaps). It would be fairly easy to build these in the parser, and to check them in the executor ... the fun part would be keeping them up-to-date while the rewriter and planner mash the query around ... >> One other mistake I noted was that the version checks added in pg_dump >> and psql are ">= 80300", which of course is obsolete now. > That one's pretty easy to handle. :) Yeah, I just wanted to make sure it wasn't forgotten. It's the kind of thing you'd not notice in testing unless you thought to try pg_dump against old server versions (which is a good idea of course). 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] column level privileges
Andrew Dunstan <[EMAIL PROTECTED]> writes: >> [ column privileges patch ] I looked over this patch with the hope of applying it, but soon despaired. It needs a great deal more work than I am willing to put into it during commitfest. There are two absolutely critical must-fix problems: 1. The syntax implemented by the patch for GRANT and REVOKE has nothing to do with that specified by the standard. The patch does, eg, GRANT INSERT ON TABLE foo (col1, col2) TO somebody but unless I have lost my ability to read BNF, what the spec demands is GRANT INSERT (col1, col2) ON TABLE foo TO somebody Admittedly the patch's syntax is more logical (especially if you consider the possibility of multiple tables) but I don't think we can go against the spec. This problem invalidates the gram.y changes and a fair amount of what was done in aclchk.c. 2. The checking of INSERT/UPDATE permissions has been moved to a completely unacceptable time/place, namely during parse analysis instead of at the beginning of execution. This is unusable for prepared queries, for example, and it also fails to apply permission checking properly for UPDATEs of inheritance trees (only the parent would get checked). This seems not very simple to fix :-(. By the time the plan gets to the executor it is not clear which columns were actually specified as targets by the user and which were filled in as defaults by the rewriter or planner. One possible solution is to add a flag field to TargetEntry to carry the information forward. Some other points that need to be considered: >> 1. The execution of GRANT/REVOKE for column privileges. Now only >> INSERT/UPDATE/REFERENCES privileges are supported, as SQL92 specified. >> SELECT privilege is now not supported. Well, SQL99 has per-column SELECT privilege, so we're already behind the curve here. The main problem again is to figure out a reasonable means for the executor to know which columns to check. TargetEntry markings won't help here. I thought about adding a bitmap to each RTE showing the attribute numbers explicitly referenced in the query, but I'm unsure if that's a good solution or not. I'd be willing to leave this as work to be done later, since 90% of the patch is just concerned with the mechanics of storing per-column privileges and doesn't care which ones they are exactly. But it needs to be on the to-do list. >> 1.1 Add a column named 'attrel' in pg_attribute catalog to store >> column privileges. Now all column privileges are stored, no matter >> whether they could be implied from table-level privilege. What this actually means, but doesn't say, is that there's no table-level representation of INSERT/UPDATE privilege any more at all. I think this is a pretty fundamental design error. In the first place it bloats pg_attribute with data that's entirely redundant for the "typical" case where per-column privileges aren't used. In the second place it slows privilege checking for the typical case, since instead of one check for the relation you have to do one for each attribute. There are some other problems too, like having to extend pg_shdepend to include an objsubid column, and some other places where the patch has to do awkward things because it's now lacking table-level information about privilege checks. What I think would be a more desirable solution is that the table ACL still sets the table-level INSERT or UPDATE privilege bit as long as you have any such privilege. In the normal case where no per-column privilege has been granted, the per-column attacl fields all remain NULL and that's all you need. As soon as any per-column GRANT or REVOKE is issued against a table, expand out the per-column ACLs to match the previous table-level state, and then apply the per-column changes. I think you'd need an additional pg_class flag column, say "relhascolacls", to denote whether this has been done --- then privilege checking would know it only needs to look at the column ACLs when this field is set. With this scheme we don't need per-column entries in pg_shdepend, we can just reference the table-level bits as before. REVOKE would have the responsibility of getting rid of per-column entries, if any, as a followup to revoking per-table entries during a DROP USER operation. Something else that needs to be thought about is whether system columns have privileges or not. The patch seems to be assuming "not" in some places, but at least for SELECT it seems like this might be sensible. Also, you can already do COPY TO the OID column if any, so even without any future extensions it seems like we've got the issue in front of us. One other mistake I noted was that the version checks added in pg_dump and psql are ">= 80300", which of course is obsolete now. I'm n
Re: [PATCHES] Snapshot management, final
I wrote: > Also, I don't think the subtransaction management is correct at all. BTW, maybe it would make more sense to keep the reference count management inside ResourceOwners, instead of having a largely duplicative set of logic in snapmgr.c. 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] Snapshot management, final
Alvaro Herrera <[EMAIL PROTECTED]> writes: > Simon Riggs wrote: >> On Tue, 2008-04-22 at 15:49 -0400, Alvaro Herrera wrote: >>> - Three CopySnapshot call sites remain outside snapmgr.c: DoCopy() on >>> copy.c, ExplainOnePlan() on explain.c and _SPI_execute_plan() on spi.c. >>> They are there because they grab the current ActiveSnapshot, modify it, >>> and then use the resulting snapshot. There is no corresponding >>> FreeSnapshot, because it's not needed. >> >> Not needed? How can we be certain that the modified snapshot does not >> outlive its original source? > It's not CopySnapshot that's not needed, but FreeSnapshot. The point > here is that the snapshot will be freed automatically as soon as it is > PopActiveSnapshot'd out of existance. CopySnapshot creates a new, > separate copy of the passed snapshot, and each of them will be freed > (separately) as soon as their refcounts reach zero. I looked over this patch and I think it still needs work. I concur with Simon's discomfort about the external CopySnapshot calls: they are reference leaks waiting to happen. As an example, you have the pattern snap = CopySnapshot(...); do some stuff; RegisterSnapshot(); but what if the "stuff" fails? In most of these code paths there's at least one palloc in between, so a failure is certainly possible. In the current form of the patch, a failure might not cost more than some leaked memory in TopTransactionContext, but it's still pretty ugly. I think it would be best if it were simply not possible to do CopySnapshot without simultaneously putting the snap into either the registered or active lists. In the COPY and EXPLAIN cases I think you misunderstood the point of the original code anyway: the modified snapshot is still the active snapshot for the duration of the command. So I think the right approach for both of these is the equivalent of what you put into spi.c: snap = CopySnapshot(snapshot); if (!read_only) snap->curcid = GetCurrentCommandId(false); PushActiveSnapshot(snap); and then a Pop at the end. It might be worth encapsulating the above series as a single copy-modify-and-push subroutine in snapmgr.c, so that you don't have to expose CopySnapshot publicly, nor expose adjustment of a snap's curcid outside snapmgr. Also, I don't think the subtransaction management is correct at all. How can it handle cases where a subtransaction and a sub-sub-transaction both take out reference counts on the same snap? If the sub-sub-xact aborts, you have no way to determine how many refcounts should go away. I think it would work better to keep the subxact level indicators in the ActiveSnapshotElt/RegdSnapshotElt list members. This would mean multiple RegdSnapshotElt members pointing at the same snapshot in any case where two different subxact levels actually had refs to the same snapshot. The RegdSnapshotElt members would each count registration counts for their own subtransaction, and the underlying snapshot would just count how many RegdSnapshotElts were pointing at it. This would allow the same amount of verification in subxact commit as you have in xact commit, ie there should be no counts left for the current subtransaction. Also, I think that the whole snapshot-sharing mechanism is not working as intended except for the serializable case; otherwise sequences like x = RegisterSnapshot(GetTransactionSnapshot()); y = RegisterSnapshot(GetTransactionSnapshot()); will result in x and y being separate copies. Or are you assuming that this just isn't worth optimizing? Some minor points: It seems odd that snapshot cleanup is late in main xact commit/abort and early in subxact commit/abort. Unless there's a really good reason for that, keep the ordering of module cleanup calls consistent across the various cases in xact.c. GetSnapshotData's serializable parameter is obsolete and should be removed. I'm not entirely comfortable with the reference counts being only uint16 wide. int32 seems safer and it isn't really saving anything much. Push/PopActiveSnap leaks the ActiveSnapshotElt. PopActiveSnapshot should probably assert active_count > 0 before decrementing, likewise in UnregisterSnapshot. 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] create or replace language
Gregory Stark <[EMAIL PROTECTED]> writes: > "Tom Lane" <[EMAIL PROTECTED]> writes: >> ... So maybe the right thing is that >> CREATE OR REPLACE LANGUAGE can change "inessential" properties of an >> existing language, but not the core properties --- which might only be >> the handler function, though you could make a case for the validator and >> the trusted flag as well. > I'm not so sure. What about if a PL language wants to include a version number > in the language handler? Or if a new version has to change the name for some > reason -- perhaps they discover that the old name doesn't work on some linkers > for some reason. Not sure that I find those cases convincing. Remember that what CREATE OR REPLACE LANGUAGE is going to be referring to is the handler function's SQL-level name; there's already a layer of indirection between it and link-level issues. 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] configure option for XLOG_BLCKSZ
Alvaro Herrera <[EMAIL PROTECTED]> writes: >> On Sat, 03 May 2008 13:14:35 -0400 Tom Lane wrote: >>> I think the use-case for varying the WAL segment size is unrelated to >>> performance of the master server, but would instead be concerned with >>> adjusting the granularity of WAL log shipping. > Seems the stuff to zero out the unused segment tail would be more useful > here. Well, that's also useful, but it hardly seems like a substitute for picking a more optimal segment size in the first place. 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] Sorting writes during checkpoint
Greg Smith <[EMAIL PROTECTED]> writes: > On Sun, 4 May 2008, Tom Lane wrote: >> Well, I tried a pgbench test similar to that one --- on smaller hardware >> than was reported, so it was a bit smaller test case, but it should have >> given similar results. > ... If > you're not offloading to another device like that, the OS-level elevator > sorting will handle sorting for you close enough to optimally that I doubt > this will help much (and in fact may just get in the way). Yeah. It bothers me a bit that the patch forces writes to be done "all of file A in order, then all of file B in order, etc". We don't know enough about the disk layout of the files to be sure that that's good. (This might also mean that whether there is a win is going to be platform and filesystem dependent ...) >> Unless someone can volunteer to test sooner, I think we should drop this >> item from the current commitfest queue. > From the perspective of keeping the committer's plates clean, a reasonable > system for this situation might be for you to bounce this into the > rejected pile as "Returned for testing" immediately, to clearly remove it > from the main queue. A reasonable expectation there is that you might > consider it again during May if someone gets back with said testing > results before the 'fest ends. Right, that's in the ground rules for commitfests: if the submitter can respond to complaints before the fest is over, we'll reconsider the patch. 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] options for RAISE statement
"Pavel Stehule" <[EMAIL PROTECTED]> writes: > this patch adds possibility to set additional options (SQLSTATE, > DETAIL, DETAIL_LOG and HINT) for RAISE statement, I looked this over briefly. A couple of comments: * Raising errors via hard-coded SQLSTATEs seems pretty unfriendly, at least for cases where we are reporting built-in errors. Wouldn't it be better to be able to raise errors using the same SQLSTATE names that are recognized in EXCEPTION clauses? * If we are going to let people throw random SQLSTATEs, there had better be a way to name those same SQLSTATEs in EXCEPTION. * I don't really like exposing DETAIL_LOG in this. That was a spur of the moment addition and we might take it out again; I think it's way premature to set it in stone by exposing it as a plpgsql feature. * Please avoid using errstart() directly. This is unwarranted intimacy with elog.h's implementation and I also think it will have unpleasant behavior if an error occurs while evaluating the RAISE arguments. (In fact, I think a user could easily force a backend PANIC that way.) The approved way to deal with ereport options that might not be there is like this: ereport(ERROR, ( ..., have_sqlstate ? errcode(...) : 0, ... That is, you should evaluate all the options into local variables and then do one normal ereport call. * // comments are against our coding conventions. 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] Fix \dT enum in psql
Andrew Dunstan <[EMAIL PROTECTED]> writes:
> Tom Lane wrote:
>> But it'll only be in \dT+ anyway, no?
> Not in this patch.
Hmmm ... given that a long list of enum members would bloat the output
quite a lot, I think I'd vote for putting it in \dT+.
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] Sorting writes during checkpoint
Greg Smith <[EMAIL PROTECTED]> writes: > On Sun, 4 May 2008, Tom Lane wrote: >> However, I am completely unable to measure any performance improvement >> from it. Given the possible risk of out-of-memory failures, I think the >> patch should not be applied without some direct proof of performance >> benefits, and I don't see any. > Fair enough. There were some pgbench results attached to the original > patch submission that gave me a good idea how to replicate the situation > where there's some improvement. Well, I tried a pgbench test similar to that one --- on smaller hardware than was reported, so it was a bit smaller test case, but it should have given similar results. I didn't see any improvement; if anything it was a bit worse. So that's what got me concerned. Of course it's notoriously hard to get consistent numbers out of pgbench anyway, so I'd rather see some other test case ... > I expect I can take a shot at quantifying > that independantly near the end of this month if nobody else gets to it > before then (I'm stuck sorting out a number of OS level issue right now > before my testing system is online again). Was planning to take a longer > look at Greg Stark's prefetching work at that point as well. Fair enough. Unless someone can volunteer to test sooner, I think we should drop this item from the current commitfest queue. 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] temporal version of generate_series()
"H.Harada" <[EMAIL PROTECTED]> writes: > Here's the sync and updated patch. > It contains "strict" in catalog as well. Applied with some revisions --- I added a timestamptz version; it didn't seem very appropriate to have only a timestamp version. You can't just pick a convenient-looking OID, you must use one that the unused_oids script reports as free. There was no check for a zero step size. There was no documentation. 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] Fix \dT enum in psql
Andrew Dunstan <[EMAIL PROTECTED]> writes:
> I notice that this patch adds an "Elements" column to the output of \dT,
> which will only be used by enum types. That seems rather ... cluttered.
But it'll only be in \dT+ anyway, no?
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] pg_postmaster_reload_time() patch
"George Gensure" <[EMAIL PROTECTED]> writes: > The new function name is pg_conf_load_time() Applied with revisions. > I'm now using LWLocks only on the backend in order to protect the > PgReloadTime from mid copy reads. This may prove to be unnecessary, > since the code to handle HUPs seems to be executed synchronously on > the backend, but I'll let someone else tell me its safe before > removing it. The locking was not only entirely useless, but it didn't even compile, since you'd not supplied a definition for "ReloadTimeLock". I took it out. I moved the setting of PgReloadTime into ProcessConfigFile. The advantages are (1) only one place to do it, and (2) inside ProcessConfigFile, we know whether or not the reload actually "took". As committed, the definition of PgReloadTime is really "the time of the last *successful* load of postgresql.conf", which I think is more useful than "the last attempted load". I also improved the documentation a bit and added the copy step needed in restore_backend_variables(). 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] [HACKERS] Text <-> C string
"Brendan Jurd" <[EMAIL PROTECTED]> writes: > Well ... if somebody does want to change the representation of xml > down the road, he's going to have to touch all the sites where the > code converts to and from cstring anyway, right? True. > With that in mind, please find attached my followup patch. It cleans > up another 21 sites manually copying between cstring and varlena, for > a net reduction of 115 lines of code. I applied most of this, but not the parts that were dependent on the assumption that bytea and text are the same. That is unlikely to remain true if we ever get around to putting locale/encoding information into text values. Furthermore it's a horrid type pun, because bytea can contain embedded nulls whereas cstrings can't; you were making unwarranted assumptions about whether, say, cstring_to_text_with_len would allow embedded nulls to go by. And lastly, quite a few of those changes were just plain broken, eg several places in selfuncs.c where you allowed strlen() to be applied to a "bytea converted to cstring". 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] Sorting writes during checkpoint
ITAGAKI Takahiro <[EMAIL PROTECTED]> writes:
> Greg Smith <[EMAIL PROTECTED]> wrote:
>> If shared_buffers(=NBuffers) is set to something big, this could give some
>> memory churn. And I think it's a bad idea to allocate something this
>> large at checkpoint time, because what happens if that fails? Really not
>> the time you want to discover there's no RAM left.
> Hmm, but I think we need to copy buffer tags into bgwriter's local memory
> in order to avoid locking taga many times in the sorting.
I updated this patch to permanently allocate the working array as Greg
suggests, and to fix a bunch of commenting issues (attached).
However, I am completely unable to measure any performance improvement
from it. Given the possible risk of out-of-memory failures, I think the
patch should not be applied without some direct proof of performance
benefits, and I don't see any.
regards, tom lane
Index: src/backend/storage/buffer/bufmgr.c
===
RCS file: /cvsroot/pgsql/src/backend/storage/buffer/bufmgr.c,v
retrieving revision 1.228
diff -c -r1.228 bufmgr.c
*** src/backend/storage/buffer/bufmgr.c 1 Jan 2008 19:45:51 - 1.228
--- src/backend/storage/buffer/bufmgr.c 4 May 2008 01:11:08 -
***
*** 56,61
--- 56,68
#define BUF_WRITTEN 0x01
#define BUF_REUSABLE 0x02
+ /* Struct for BufferSync's internal to-do list */
+ typedef struct BufAndTag
+ {
+ int buf_id;
+ BufferTag tag;
+ } BufAndTag;
+
/* GUC variables */
bool zero_damaged_pages = false;
***
*** 986,991
--- 993,1025
}
/*
+ * qsort comparator for BufferSync
+ */
+ static int
+ bufandtagcmp(const void *a, const void *b)
+ {
+ const BufAndTag *lhs = (const BufAndTag *) a;
+ const BufAndTag *rhs = (const BufAndTag *) b;
+ int r;
+
+ /*
+* We don't much care about the order in which different relations get
+* written, so memcmp is enough for comparing the relfilenodes,
+* even though its behavior will be platform-dependent.
+*/
+ r = memcmp(&lhs->tag.rnode, &rhs->tag.rnode, sizeof(lhs->tag.rnode));
+ if (r != 0)
+ return r;
+
+ /* We do want blocks within a relation to be ordered accurately */
+ if (lhs->tag.blockNum < rhs->tag.blockNum)
+ return -1;
+ if (lhs->tag.blockNum > rhs->tag.blockNum)
+ return 1;
+ return 0;
+ }
+
+ /*
* BufferSync -- Write out all dirty buffers in the pool.
*
* This is called at checkpoint time to write out all dirty shared buffers.
***
*** 995,1004
static void
BufferSync(int flags)
{
int buf_id;
- int num_to_scan;
int num_to_write;
int num_written;
/* Make sure we can handle the pin inside SyncOneBuffer */
ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
--- 1029,1056
static void
BufferSync(int flags)
{
+ static BufAndTag *bufs_to_write = NULL;
int buf_id;
int num_to_write;
int num_written;
+ int i;
+
+ /*
+* We allocate the bufs_to_write[] array on first call and keep it
+* around for the life of the process. This is okay because in normal
+* operation this function is only called within the bgwriter, so
+* we won't have lots of large arrays floating around. We prefer this
+* way because we don't want checkpoints to suddenly start failing
+* when the system gets under memory pressure.
+*/
+ if (bufs_to_write == NULL)
+ {
+ bufs_to_write = (BufAndTag *) malloc(NBuffers *
sizeof(BufAndTag));
+ if (bufs_to_write == NULL)
+ ereport(FATAL,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+errmsg("out of memory")));
+ }
/* Make sure we can handle the pin inside SyncOneBuffer */
ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
***
*** 1033,1038
--- 1085,1092
if (bufHdr->flags & BM_DIRTY)
{
bufHdr->flags |= BM_CHECKPOINT_NEEDED;
+ bufs_to_write[num_to_write].buf_id = buf_id;
+ bufs_to_write[num_to_write].tag = bufHdr->tag;
num_to_write++;
}
***
*** 1043,1061
return; /
Re: [PATCHES] [HACKERS] Multiline privileges in \z
Andrew Dunstan <[EMAIL PROTECTED]> writes: > Wouldn't this expression: > pg_catalog.array_to_string(c.relacl, chr(10)) > be better expressed as > pg_catalog.array_to_string(c.relacl, E'\n') +1 ... it's minor, but knowing that ASCII LF is 10 is probably not wired into too many people's brains anymore. (Besides, some of us remember it as octal 12, anyway...) 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] Exposing keywords to clients
"Dave Page" <[EMAIL PROTECTED]> writes:
> Attached is an updated patch, giving the following output.
Oh, one other thing: dropping externs into random modules unrelated to
their source module is completely awful programming style, because there
is nothing preventing incompatible declarations. Put those externs in
keywords.h instead. I suspect you have ignored a compiler warning
about not declaring pg_get_keywords itself, too --- it should be
extern'd in builtins.h.
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] Exposing keywords to clients
"Dave Page" <[EMAIL PROTECTED]> writes: > Attached is an updated patch, giving the following output. The catdesc > column can be translated. Documentation has got a couple of problems: > + contains the keyword, the catcode column contains a > + category code of 'U' for unknown, 'C' for column name, 'T' for type or > + function name or 'U' for unreserved. The catdesc > + column contains a localised stribg describing the category. I wouldn't document the "unknown" case at all, much less document it incorrectly, and you forgot the "reserved" case, and "stribg"? 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] create or replace language
"Andreas 'ads' Scherbaum" <[EMAIL PROTECTED]> writes: > Attached is a first version for the "CREATE OR REPLACE LANGUAGE" patch. > It's still missing some functionality (especially the update part is > far away from being complete) and it's also missing documentation. It strikes me that if there are any existing functions in the language, we might want to restrict what can be changed by CREATE OR REPLACE. For instance switching to a completely different language handler doesn't seem like a great idea. The equivalent problem for views and functions is handled by restricting CREATE OR REPLACE to not change the output column set of a view or the type signature of a function, independently of whether there are any actual references to the object. So maybe the right thing is that CREATE OR REPLACE LANGUAGE can change "inessential" properties of an existing language, but not the core properties --- which might only be the handler function, though you could make a case for the validator and the trusted flag as well. 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] [COMMITTERS] pgsql: Sigh ...
Andrew Dunstan <[EMAIL PROTECTED]> writes: > Peter Eisentraut wrote: >> Btw., it is quite easily possible to use the autom4te tracing facility to >> parse the configure.ac file, in case you are interested in generating some >> of >> the Windows build code automatically. > Yeah, maybe. Let's fix the issue pg_config.h.win32 that Tom raised first. +1 for both. We really need to eliminate as much redundancy as we can between the two build systems, because it'll keep biting us until we do. 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] configure option for XLOG_BLCKSZ
Simon Riggs <[EMAIL PROTECTED]> writes:
> We already hit that issue and fixed it early in the 8.3 cycle. It was
> more of a problem than the checkpoint issue because it caused hard
> lock-outs while the file switches occurred. It didn't show up unless you
> looked at the very detailed transaction result data because on fast
> systems we are file switching every few seconds.
> Not seen any gains from varying the WAL file size since then...
I think the use-case for varying the WAL segment size is unrelated to
performance of the master server, but would instead be concerned with
adjusting the granularity of WAL log shipping.
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] Exposing keywords to clients
Peter Eisentraut <[EMAIL PROTECTED]> writes: > Dave Page wrote: >>> Perhaps use a separate string for machine parse (say R, T, C, U), and >>> let the string be translatable. >> >> I considered that, but wasn't sure if folks would like the redundancy. >> It's trivial to do of course - any objections? > Is there anything useful you would do with this information? Or would you > just quote all listed words anyway? I think the practical application would be to avoid quoting unreserved keywords, as pg_dump for instance already does. I doubt anyone would bother distinguishing the different types of partially/wholly reserved words. So maybe a boolean would be sufficient --- but I have nothing against the R/T/C/U suggestion. A more radical alternative is just to omit unreserved words from the view altogether. 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] fix for pl/pythons named OUT parameter handling
Hannu Krosing <[EMAIL PROTECTED]> writes:
> Before this patch, pl/python will not do the right thing if OUT
> parameters are present
Applied with minor stylistic corrections.
I notice that plpython still doesn't like multiple OUT parameters,
but at least it throws a sane error ...
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] Exposing keywords to clients
Alvaro Herrera <[EMAIL PROTECTED]> writes: > FWIW pg_dump has fmtId() which does something related. > I think it's a bit bogus to be using the list as compiled client-side, > precisely due to the theoretical chance that it could change from one > server version to the next, but it's probably not very likely that we > ever remove a keyword from the server grammar. Actually, it's 100% intentional that pg_dump does it that way --- I would not support modifying it to use this function (even if it existed in the back branches). The reason is exactly that pg_dump wants to generate output that is correct for its own PG version, not that of the server it's dumping from. The tradeoffs are probably different for pgAdmin, but it is important to realize that either way might be the best thing for a particular case. 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] plpgsql RETURN QUERY EXECUTE
"Pavel Stehule" <[EMAIL PROTECTED]> writes:
> This patch allows dynamic queries in RETURN QUERY statement.
> http://archives.postgresql.org/pgsql-hackers/2008-02/msg01180.php
Applied, thanks.
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] [COMMITTERS] pgsql: Sigh ...
Andrew Dunstan <[EMAIL PROTECTED]> writes:
> Tom Lane wrote:
>> This doesn't look quite right; unless the arithmetic is being done in
>> floating point? I had it like this in configure.in:
>>
>> RELSEG_SIZE=`expr '(' 1024 '*' ${segsize} / ${blocksize} ')' '*' 1024`
> blocksize is one of (1,2,4,8,16,32) so it should always be a factor of
> 1024 unless my arithmetic is awry. I did it that way because I dislike
> expressions with unbracketed mixed operations - they make me think too
> much.
Well, if you dislike the original on style grounds, you should change it
to match. Doing the same thing in two different ways in two places
isn't good.
>> Also it looks like you missed adding segsize to the config.pl comments.
> That's deliberate - we are currently only allowing a value of 1 here, so
> I don't see any point in putting it in the sample config file, even as a
> comment. When we enable other seg sizes we can add it to the sample file.
Ah, OK.
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] plpgsql CASE statement - last version
"Pavel Stehule" <[EMAIL PROTECTED]> writes: > 2008/5/2 Heikki Linnakangas <[EMAIL PROTECTED]>: >> How about taking a completely different strategy, and implement the >> CASE-WHEN construct fully natively in plpgsql, instead of trying to convert >> it to a single SQL CASE-WHEN expression? It's not a very good match anyway; > It was first variant. It's simpler for parsing and slower for > execution :(. It means more than once expression evaluation and for > simple case value casting and comparation. I agree with Heikki: this patch is seriously ugly, and "slower for execution" isn't a good enough reason for saddling us with having to maintain such a kluge in the parser. I don't really see why you should need to have multiple expression evaluations, anyhow. Can't you evaluate the test expression once and inject its value into the comparisons using CaseTestExpr, the same way the core CASE-expression code works? 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] [COMMITTERS] pgsql: Sigh ...
Andrew Dunstan <[EMAIL PROTECTED]> writes:
> ! print O "#define RELSEG_SIZE ",
> ! (1024 / $self->{options}->{blocksize}) *
> ! $self->{options}->{segsize} * 1024, "\n";
This doesn't look quite right; unless the arithmetic is being done in
floating point? I had it like this in configure.in:
RELSEG_SIZE=`expr '(' 1024 '*' ${segsize} / ${blocksize} ')' '*' 1024`
Also it looks like you missed adding segsize to the config.pl comments.
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] GUC parameter cursors_tuple_fraction
"Hell, Robert" <[EMAIL PROTECTED]> writes:
> This patch adds a GUC parameter for tuple_fraction of cursors (discussed
> earlier here:
> http://archives.postgresql.org/pgsql-performance/2008-04/msg00018.php).
> By setting this parameter the planner's favor to use fast-start plans
> for cursors can be affected.
Applied with some documentation cleanup, and also care for the behavior
at the range endpoints 0 and 1. A user expecting it to act like a
simple fraction would have been quite surprised at the endpoints
(see the header comments for grouping_planner).
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: [HACKERS] [PATCHES] GUC parameter cursors_tuple_fraction
"Hell, Robert" <[EMAIL PROTECTED]> writes: > You're right - that's just a typo in the subject of the post. > It's called cursor_tuple_fraction in the submitted patch. Ah, I hadn't actually read the patch yet ;-). As penance for the noise, I will do so now. 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: [HACKERS] [PATCHES] GUC parameter cursors_tuple_fraction
Simon Riggs <[EMAIL PROTECTED]> writes:
> OK, if that's the view then the patch is ready for commit, AFAICS.
Use of the plural in the name seems a bit odd to me. Anyone have a
problem with calling it "cursor_tuple_fraction" instead?
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] configure option for XLOG_BLCKSZ
"Mark Wong" <[EMAIL PROTECTED]> writes: > I saw a that a patch was committed that exposed a configure switch for > BLCKSZ. I was hoping that I could do that same for XLOG_BLCKSZ. I > think I got the configure.in, sgml, pg_config_manual.h, and > pg_config.h.in changes correct. Applied with minor changes: * I thought it better to call the switch --with-wal-blocksize than --with-xlog-blocksize. Although we've not been terribly consistent about it, there is more user-facing documentation that calls it WAL than XLOG. * I added a --with-wal-segsize switch as well. It's not totally clear what the allowed ranges of the settings should be. The method of using a shell "case" to verify the setting validity is kinda klugy, but I couldn't offhand think of a direct test for "is this a power of 2" at the shell level, so it seems we need to be restrictive. 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] configure option for XLOG_BLCKSZ
"Mark Wong" <[EMAIL PROTECTED]> writes:
> I still believe it makes sense to have them separated. I did have
> some data, which has since been destroyed, that suggested there were
> some system characterization differences for OLTP workloads with
> PostgreSQL. Let's hope those disks get delivered to Portland soon. :)
Fair enough. It's not that much more code to have another configure
switch --- will go do that.
If we are allowing blocksize and relation seg size to have configure
switches, seems that symmetry would demand that XLOG_SEG_SIZE be
configurable as well. Thoughts?
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: [HACKERS] [PATCHES] GUC parameter cursors_tuple_fraction
"Heikki Linnakangas" <[EMAIL PROTECTED]> writes: > Simon Riggs wrote: >> * We've said here http://www.postgresql.org/docs/faqs.TODO.html that we >> "Don't want hints". If that's what we really think, then this patch must >> surely be rejected because its a hint... That isn't my view. I *now* >> think we do need hints of various kinds. > cursors_tuple_fraction or OPTIMIZE FOR xxx ROWS isn't the kind of hints > we've said "no" to in the past. More to the point, I think what we've generally meant by "hints" is nonstandard decoration on individual SQL commands (either explicit syntax or one of those interpret-some-comments kluges). Simon is reading the policy in such a way that it would forbid all the planner cost parameters, which is surely not what is intended. I see this as being basically another cost parameter, and as such I don't think it needs more documentation than any of those have. (Now admittedly you could argue that they could all use a ton more documentation than they now have, but it's not reasonable to insist on just this one meeting a different standard.) 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] configure option for XLOG_BLCKSZ
"Mark Wong" <[EMAIL PROTECTED]> writes: > As someone who has tested varying both those parameters it feels > awkward to have a configure option for one and not the other, or vice > versa. I have slightly stronger feelings for having them both as > configure options because it's easier to script, but feel a little > more strongly about having BLCKSZ and XLOG_BLCKSZ both as either > configure options or in pg_config_manual.h. To have them such that > one needs to change them in different manners makes a tad more work in > automating testing. So my case is just for ease of testing. Well, that's a fair point. Another issue though is whether it makes sense for XLOG_BLCKSZ to be different from BLCKSZ at all, at least in the default case. They are both the unit of I/O and it's not clear why you'd want different units. Mark, has your testing shown any indication that they really ought to be separately configurable? I could see having the same configure switch set both of 'em. 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] configure option for XLOG_BLCKSZ
"Mark Wong" <[EMAIL PROTECTED]> writes: > I saw a that a patch was committed that exposed a configure switch for > BLCKSZ. I was hoping that I could do that same for XLOG_BLCKSZ. Well, we certainly *could*, but what's the use-case really? The case for varying BLCKSZ is marginal already, and I've seen none at all for varying XLOG_BLCKSZ. Why do we need to make it easier than "edit pg_config_manual.h"? 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] Removing NONSEG mode
Zdenek Kotala <[EMAIL PROTECTED]> writes:
> I attach patch which remove nonsegment mode support. It was discussed during
> last commit fest. Nonsegment mode is possible uses only on couple of FS (ZFS,
> XFS) and it is not safe on any OS because each OS support more filesystems.
Applied with revisions --- mostly, you missed updating the
documentation, but also I didn't like the "LL" constant you used in
RELSEG_SIZE. It's unportable and there might be unexpected side effects
from having the macro represent an int64 constant instead of an integer.
So I had configure do the arithmetic and emit an integer.
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: [HACKERS] [PATCHES] Removing typename from A_Const (was: Empty arrays with ARRAY[])
Alvaro Herrera <[EMAIL PROTECTED]> writes: > Hmm, I'm now wondering if the location should be added to Value as well, > so that it can be passed down to Const? Just for the record, we don't want it in Const. Parse locations are only useful in the "raw grammar" output, mainly because they aren't helpful unless you still have the original query string laying about. 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: [HACKERS] [PATCHES] Removing typename from A_Const (was: Empty arrays with ARRAY[])
Alvaro Herrera <[EMAIL PROTECTED]> writes:
> I came up with the attached patch.
I wasn't envisioning anything anywhere near this invasive. We only
need locations on constants in a few contexts, I think.
BTW, you broke _equalAConst() ... it was a bad idea anyway to recast
it on the assumption that there would never again be more than one
field.
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: [HACKERS] [PATCHES] Removing typename from A_Const (was: Empty arrays with ARRAY[])
Alvaro Herrera <[EMAIL PROTECTED]> writes:
> Tom Lane escribió:
>> They're logically different things, and after I get done putting a parse
>> location field into A_Const, they'll still be physically different too.
> Aha. Are you working from Brendan's patch? I was going to commit it.
Sure, go ahead. I was going to add the parse location at the same time,
but it can perfectly well be done as a separate patch.
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] Removing typename from A_Const (was: Empty arrays with ARRAY[])
Alvaro Herrera <[EMAIL PROTECTED]> writes: > Brendan Jurd escribió: >> Here's my attempt to remove the typename field from A_Const. There >> were a few places (notably flatten_set_variable_args() in guc.c, and >> typenameTypeMod() in parse_type.c) where the code expected to see an >> A_Const with a typename, and I had to adjust for an A_Const within a >> TypeCast. Nonetheless, there was an overall net reduction of 34 lines >> of code, so I think this was a win. > Do say ... why don't we do away with A_Const altogether and just replace > it with Value? After this patch, I don't see what's the difference. They're logically different things, and after I get done putting a parse location field into A_Const, they'll still be physically different too. 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] win32mak_patch
"Hiroshi Saito" <[EMAIL PROTECTED]> writes: > *** src/interfaces/libpq/exports.txt.orig Sun Apr 27 23:41:51 2008 > --- src/interfaces/libpq/exports.txt Sun Apr 27 23:42:48 2008 > *** > *** 141,143 > --- 141,144 > pg_valid_server_encoding_id 139 > PQconnectionNeedsPassword 140 > lo_import_with_oid141 > + pgwin32_safestat 142 We are most certainly NOT doing that (for one reason, it will instantly break every platform except Windows). If there is something that seems to require it then you need to find another way. 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
"Albe Laurenz" <[EMAIL PROTECTED]> writes: > Tom Lane wrote: >> 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. Applied with additional cleanup. You hadn't thought very carefully about additional state transitions that would have to be introduced into the postmaster state machine to support a new state --- for example, as coded a SIGINT delivered to the postmaster after SIGTERM would fail to do anything at all, when of course it really ought to force us into fast shutdown. Also, it's not really that hard to disallow non-superusers from connecting in PM_WAIT_BACKUP state. 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
Hans-Juergen Schoenig <[EMAIL PROTECTED]> writes: > Alvaro Herrera wrote: >> Question for Hans-Juergen and Zoltan: have you tested 8.3 and do you >> still see the need for this? > i have seen this problem two or three times within the past 2-3 years or > so. so, it can basically happen in the field for some special purpose > applications but i don't see this as an every day problem. it would be > nice to have it in. So these experiences were pre-8.3, right? The reason that I'm harping on that is that plpgsql does a CommandCounterIncrement for each expression it evaluates, whether or not there's any visible database access. As of 8.3 that won't cause consumption of CIDs, but before it did. I suspect that in a lot of real-world scenarios, CID consumption from triggers will be down by an order of magnitude. 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
"Albe Laurenz" <[EMAIL PROTECTED]> writes:
> That should work, but isn't it better if backup_label is removed
> only if we know we're going to shutdown cleanly?
Why? That seems like an entirely arbitrary specification.
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
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] 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] 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] 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] 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] Improve shutdown during online backup, take 4
Magnus Hagander <[EMAIL PROTECTED]> writes:
> Alvaro Herrera wrote:
>> I think the messages should not have a newline in the middle.
> Are you talking about the one in pg_ctl? We have other messages in
> pg_ctl that already do this, so I figured it was ok there...
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", ...
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.
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] Snapshot management, final
Alvaro Herrera <[EMAIL PROTECTED]> writes: > Simon Riggs wrote: >> OK, so it can;t be copied to a longer lived memory context? > CopySnapshot always copies snapshots to SnapshotContext, which is a > context that lives until transaction end. There's no mechanism for > copying a snapshot into another context, because I don't see the need. The only reason we have memory contexts at all is to avoid the need to track individual palloc'd objects. Since we're instituting exactly such tracking for snapshots, there's no value in placing them in general-purpose memory contexts. 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] float4/float8/int64 passed by value with tsearch fixup
"Guillaume Smet" <[EMAIL PROTECTED]> writes: > On Mon, Apr 21, 2008 at 2:28 AM, Tom Lane <[EMAIL PROTECTED]> wrote: >> Applied with revisions --- mostly, supporting configure-time control >> over whether pass-by-value is used. > Do we need buildfarm coverage of these options? Wouldn't hurt, I guess, though it's not very urgent since the non-default cases were default up till these patches. Note that we are already exercising both ends of the float8-byval option, so probably only --disable-float4-byval is very interesting to test explicitly. 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] int8/float8/time/timestamp[tz]/float4 passed by value, was Re: Fix HAVE_LONG[_LONG]_INT_64 to really define to 1
Gregory Stark <[EMAIL PROTECTED]> writes: > 4) Your problems with tsearch and timestamp etc raise an interesting problem. >We don't need to mark this in pg_control because it's a purely a run-time >issue and doesn't affect on-disk storage. Just for the record, that's wrong. It's true that on-disk data isn't affected, but the system catalog contents are, and they'd better match what the postgres binary is going to do. >However it does affect ABI >compatibility with modules. Perhaps it should be added to >PG_MODULE_MAGIC_DATA. Very good point, especially now that it's configurable. Included in committed patch. 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] float4/float8/int64 passed by value with tsearch fixup
Zoltan Boszormenyi <[EMAIL PROTECTED]> writes:
> I tried to split the previous patch up to see where the tsearch regression
> comes from. So, it turned out that:
> - float4 conversion is risk free (patch #1)
> - float8 conversion is okay, too, if coupled with time[stamp[tz]] conversion
> (patch #2) but with int64 timestamps enabled, the next one is also
> needed:
> - int64 conversion (patch #3) is mostly okay but it is the one that's
> causing
> the tsearch regression
Applied with revisions --- mostly, supporting configure-time control
over whether pass-by-value is used.
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] float4/float8/int64 passed by value with tsearchfixup
Gregory Stark <[EMAIL PROTECTED]> writes:
> So are you killing V0 for non-integral types? Because if not we should keep
> some sacrificial module to the regression tests to use to test for this
> problem.
Well, we could potentially continue to have, say, the oldstyle
geo_distance function used when not FLOAT8PASSBYVAL. Not clear how
useful that really is though.
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] float4/float8/int64 passed by value with tsearchfixup
Tom Lane <[EMAIL PROTECTED]> writes:
> BTW, I trolled the contrib files for other v0 functions taking or
> returning float4 or float8. I found seg_size (fixed it) and a whole
> bunch of functions in earthdistance.
Hmm, actually most of the "bunch" are SQL functions, there's only one
that needs fixing. Done.
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] float4/float8/int64 passed by value with tsearch fixup
Zoltan Boszormenyi <[EMAIL PROTECTED]> writes:
>> That looks suspiciously locale-ish; what locale are you running PG in?
> hu_HU.UTF-8
Ah, and I'll bet zs sorts after zy in hu_HU.
The query is already doing an ORDER BY, so it's not at fault. I think
the only thing we could do about this is add a variant expected file
with the hu_HU sort ordering. I'd be happy to do that if it were
affecting the main regression tests, but not sure it's worth it for
contrib/tsearch2 ... thoughts?
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] float4/float8/int64 passed by value with tsearch fixup
Alvaro Herrera <[EMAIL PROTECTED]> writes: > With contrib/seg also adjusted to use float4 instead of float32, and > thus the last usage of float32 gone, I am now wondering if it would be a > good idea to remove the float32 and float32data definitions in c.h. Well, there might still be some references to them in add-on code, but considering that c.h has said this since 7.1 * XXX: these typedefs are now deprecated in favor of float4 and float8. * They will eventually go away. it seems hard to argue that we've not given adequate notice. I'm +1 for removing them, and float64/float64data too. It's not like they're hard to avoid using, even if you don't want to update to v1 call convention. If there are no objections, I'll see about pushing in the remaining parts of the patch (the original and the just-submitted fix to allow float4byval to be selectable) over this weekend. 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] float4/float8/int64 passed by value with tsearchfixup
BTW, I trolled the contrib files for other v0 functions taking or returning float4 or float8. I found seg_size (fixed it) and a whole bunch of functions in earthdistance. Those use float8 not float4, so they are not broken by this patch, but that module will have to be v1-ified before we can consider applying the other part of the patch. 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] float4/float8/int64 passed by value with tsearchfixup
Gregory Stark <[EMAIL PROTECTED]> writes: >> Tom Lane wrote: >>> Specifically, I think what you missed is that on some platforms C >>> functions pass or return float values differently from similar-sized >>> integer or pointer values (typically, the float values get passed in >>> floating-point registers). > But I'm skeptical that it would hit such a wide swathe of the build farm. In > particular AFAIK the standard ABI for i386 does no such thing. I did some digging, and it seems you're mistaken. The standard gcc ABI for both i386 and x86_64 returns floats in float registers (387 registers in the first case, and SSE registers in the second case). This appears to have been the case for a very long time. I quote from the manual for gcc 2.95: `-mno-fp-ret-in-387' Do not use the FPU registers for return values of functions. The usual calling convention has functions return values of types `float' and `double' in an FPU register, even if there is no FPU. The idea is that the operating system should emulate an FPU. The option `-mno-fp-ret-in-387' causes such values to be returned in ordinary CPU registers instead. It seems very odd that Alvaro's testing on an AMD64 platform didn't show the problem. 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] float4/float8/int64 passed by value with tsearch fixup
Zoltan Boszormenyi <[EMAIL PROTECTED]> writes:
> Both --enable and --disable-float4-byval produced only this regression,
> but it seems to be a tuple order difference.
That looks suspiciously locale-ish; what locale are you running PG in?
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] float4/float8/int64 passed by value with tsearchfixup
Alvaro Herrera <[EMAIL PROTECTED]> writes: > Tom Lane wrote: >> I was wondering about that too, once it became obvious that (almost?) >> everything was failing not just some platforms. However, this >> afternoon's CVS HEAD *does* pass the seg regression test for me on HPPA, >> and I presume it passed on whatever Alvaro is using (btw, what was >> that?). So there's definitely a platform dependency involved and not >> just you-missed-a-pointer-someplace. > Huh, this was with the code with v0 conventions, right? I committed the > change to v1 conventions a while ago. Right, I had intentionally cvs updated to the broken version to test. 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] float4/float8/int64 passed by value with tsearchfixup
Gregory Stark <[EMAIL PROTECTED]> writes: >> Tom Lane wrote: >>> Specifically, I think what you missed is that on some platforms C >>> functions pass or return float values differently from similar-sized >>> integer or pointer values (typically, the float values get passed in >>> floating-point registers). > Hum. That's a valid concern for some platforms, Sparc I think? > But I'm skeptical that it would hit such a wide swathe of the build > farm. I was wondering about that too, once it became obvious that (almost?) everything was failing not just some platforms. However, this afternoon's CVS HEAD *does* pass the seg regression test for me on HPPA, and I presume it passed on whatever Alvaro is using (btw, what was that?). So there's definitely a platform dependency involved and not just you-missed-a-pointer-someplace. 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] float4/float8/int64 passed by value with tsearch fixup
Tom Lane <[EMAIL PROTECTED]> writes: > Alvaro Herrera <[EMAIL PROTECTED]> writes: >> Tom Lane wrote: >>> Since they're v0, they'd have to explicitly know about the pass-by-ref >>> status of float4. >> Well, the previous code was doing some pallocs, and the new code is not: >> http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/contrib/seg/seg.c.diff?r1=1.20;r2=1.21 > [ shrug... ] So, you missed something. Specifically, I think what you missed is that on some platforms C functions pass or return float values differently from similar-sized integer or pointer values (typically, the float values get passed in floating-point registers). On such platforms it is essentially impossible for a v0 function to work with pass-by-val float4 --- since fmgr_oldstyle thinks it's passing integers and getting back pointers, the values are being transferred in the wrong registers. The only way to make it work would be to mangle the function declarations and then play union tricks like those in Float4GetDatum/DatumGetFloat4, which is certainly not very practical. It'd be less ugly to convert to v1 calling convention. (This very likely explains why the Berkeley folk made float4 pass-by-ref in the first place, a decision that doesn't make a lot of sense if you don't know about this problem.) So I think we really do need to offer that compile-time option. Failing to do so will be tantamount to desupporting v0 functions altogether, which I don't think we're prepared to do. 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] float4/float8/int64 passed by value with tsearch fixup
Alvaro Herrera <[EMAIL PROTECTED]> writes: > Tom Lane wrote: >> Since they're v0, they'd have to explicitly know about the pass-by-ref >> status of float4. > Well, the previous code was doing some pallocs, and the new code is not: > http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/contrib/seg/seg.c.diff?r1=1.20;r2=1.21 [ shrug... ] So, you missed something. >> Did this patch include a compile-time choice of whether things could >> remain pass-by-ref? I rather imagine that some people out there will >> prefer to stay that way instead of fix their old v0 code. > Hmm, nope. Do we really need that? Given that we *have to* handle a compile-time choice for whether float8 is pass-by-ref, I should think that allowing a similar choice for float4 is perfectly sensible and not really more work (it'll just be a second instance of the same code pattern). I'm not at all sure it made sense to apply this portion of the patch separately. 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] float4/float8/int64 passed by value with tsearch fixup
Alvaro Herrera <[EMAIL PROTECTED]> writes:
> I assume this is just some dumb portability mistake on my part ... or
> perhaps the fact that the functions are still using v0 fmgr convention?
Since they're v0, they'd have to explicitly know about the pass-by-ref
status of float4.
Did this patch include a compile-time choice of whether things could
remain pass-by-ref? I rather imagine that some people out there will
prefer to stay that way instead of fix their old v0 code.
In the meantime, converting contrib/seg to v1 might be the best
solution.
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] Testing pg_terminate_backend()
Bruce Momjian <[EMAIL PROTECTED]> writes:
> Attached is my test script. I ran it for 14 hours (asserts on),
> running 450 regression tests, with up to seven backends killed per
> regression test.
Hmm, there are something on the order of 1 SQL commands in our
regression tests, so even assuming perfect randomness you've exercised
SIGTERM on maybe 10% of them --- and of course there's multiple places
in a complex DDL command where SIGTERM might conceivably be a problem.
Who was volunteering to run this 24x7 for awhile?
> SLEEP=`expr $RANDOM \* $REGRESSION_DURATION / 32767`
Uh, where's the randomness coming from?
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] Proposed patch - psql wraps at window width
Bryce Nesbitt <[EMAIL PROTECTED]> writes:
> I checked the use of COLUMNS and it seems bash updates the
> environment
> variable when a window is resized.
[ Please get rid of the HTML formatting ... ]
Bash can update the environment all it wants, but that will not affect
what is seen by a program that's already running. Personally I often
resize the window while psql is running, and I expect that to work.
I'm with Peter on this one: we have used ioctl, and nothing else, to
determine the vertical window dimension for many years now, to the tune
of approximately zero complaints. It's going to take one hell of a
strong argument to persuade me that determination of the horizontal
dimension should not work exactly the same way.
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] Proposed patch - psql wraps at window width
Gregory Stark <[EMAIL PROTECTED]> writes:
> Also, how would you suggest figuring the width to use for output going to a
> file? ioctl is irrelevant in that case, imho it should just default to 80
> columns if COLUMNS is unset.
It would be a spectacularly awful idea for this patch to affect the
output to a file at all.
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] [HACKERS] Multiline privileges in \z
"Brendan Jurd" <[EMAIL PROTECTED]> writes:
> It occurred to me that psql's \z command could benefit from the
> addition of some newlines. With any more than one grantee per object,
> the output of \z rapidly becomes extremely wide, and very hard to
> read.
Seems like a good idea now that psql deals nicely with multiline output.
The function names in the patch need schema-qualification in case
pg_catalog is not first in the search path.
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] [HACKERS] Text <-> C string
"Brendan Jurd" <[EMAIL PROTECTED]> writes: >> If we don't want to meddle with xmltype/bytea/VarChar at all, we'll >> have to revert those changes, and I'll have to seriously scale back >> the cleanup patch I was about to post. > Still not sure where we stand on the above. To cast, or not to cast? I dunno. I know there was previously some handwaving about representing XML values in some more intelligent fashion than a plain text string, but I have no idea if anyone is likely to do anything about it in the foreseeable future. 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] [HACKERS] Remove lossy-operator RECHECK flag?
Teodor Sigaev <[EMAIL PROTECTED]> writes: > Why don't use suggested way for GIN index over tsvector? > http://archives.postgresql.org/pgsql-hackers/2008-04/msg00812.php I had forgotten the details :-(. Will adopt that code. >> This is still WIP because I haven't touched any contrib code, but >> as far as the main backend goes I think it's ready to apply. > Patch to all contrib modules: > http://www.sigaev.ru/misc/contrib.patch.gz Ooops, we duplicated work :-(. Will compare with your version before applying. 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] Reference by output in : \d
"Brendan Jurd" <[EMAIL PROTECTED]> writes: > Sorry Kenneth, I didn't quite follow your explanation. How does the > position of "FOREIGN KEY " affect the indentation at the beginning of > the footer? I changed that patch around a bit while applying it, and very possibly fat-fingered the indentation --- I don't recall having explicitly compared it to the other cases. It surely should be consistent with everything else. 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] [HACKERS] Remove lossy-operator RECHECK flag?
I wrote: > I've committed changes that move the determination of whether recheck is > required into the index AMs. Right now, GIST and GIN just always set > the recheck flag to TRUE. Obviously that control should be pushed down > to the opclass consistent() functions, but I don't know that code well > enough to be clear on exactly what should happen. Are you willing to > do that part? Actually, I think I figured it out. Please look over the GIST/GIN parts of this patch and see if they're OK with you. This is still WIP because I haven't touched any contrib code, but as far as the main backend goes I think it's ready to apply. regards, tom lane binI2STY1wRy8.bin Description: run-time-recheck-1.patch.gz -- 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] [PATCH] sh: Add support Renesas SuperH
I wrote:
> Nobuhiro Iwamatsu <[EMAIL PROTECTED]> writes:
>> +#if defined(__sh__) /* Renesas SuperH */
> Do they have any longer form of that macro?
I looked into the gcc sources, and the answer seems to be "no" :-(.
So we're stuck with __sh__.
I'm still pretty skeptical about the adequacy of the asm parameters,
though.
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] datum passed to macro which expects a pointer
Gavin Sherry <[EMAIL PROTECTED]> writes:
>> might as well just use PG_RETURN_DATUM instead of casting twice.
> Oh of course. Updated patch attached.
Applied, thanks.
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] datum passed to macro which expects a pointer
Gavin Sherry <[EMAIL PROTECTED]> writes:
> I wish. It was actually thrown up when we (Greenplum) changed the macros
> to be inline functions as part of changing Datum to be 8 bytes.
Hmmm ... Datum has been 8 bytes for many years, on 64-bit machines.
What is it you're trying to accomplish by making it wider on 32-bitters?
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] datum passed to macro which expects a pointer
Gavin Sherry <[EMAIL PROTECTED]> writes:
> This may seem a little pedantic but I noticed a few places where we pass
> a datum to a macro which treats the datum as a pointer. This works now
> but might not in the future (if, say, Datum were to be 8 bytes).
Yeah, definitely something to fix. I think though that the cases
like this:
> ! PG_RETURN_TEXT_P(DatumGetPointer(result));
might as well just use PG_RETURN_DATUM instead of casting twice.
Was this just eyeball inspection or did you find a compiler that would
complain about 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] [HACKERS] Terminating a backend
Bruce Momjian <[EMAIL PROTECTED]> writes:
>> When we get the termination signal, why can't we just set a global
>> boolean, do a query cancel, and in the setjmp() code block check the
>> global and exit --- at that stage we know we have released all locks and
>> can exit cleanly.
> I have implemented this idea with the attached patch.
It was already explained to you why this is a bad idea.
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] libpq patch for pqtypes hook api and PGresult creation
Andrew Chernow <[EMAIL PROTECTED]> writes: > The requirements for the connCreate hook are that the PGconn is ready > for use. I am currently hooking in connectDBStart, which is dead wrong. I looked at the "object hooks" patch and it looked like a complete mess. AFAICS the only way you could use it would be to insert hooks at library _init() time, meaning that the mere linking of libpgtypes into an executable would cause all your hook overhead to occur on every connection and every query ever made by that program. The thread locking you put in is completely horrid as well --- you've got it holding a process-wide lock over operations that are likely to include nontrivial database interactions. I think you need to consider something a bit less invasive. What I'd imagine is something more like this: a program that wishes to use libpgtypes calls "PQinitTypes(PGconn *conn)" immediately after establishing a connection, and that installs hooks into connection-local storage and does whatever per-connection setup it needs. No need for any global state nor any thread mutexes. Lastly, as far as the hook designs themselves: the "hook name" concept seems utterly useless, and what *is* needed is missing: every callback function needs a pass-through void * pointer so that it can get at private state. 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] libpq Win32 Mutex performance patch
Andrew Chernow <[EMAIL PROTECTED]> writes: > A more graceful solution would be to print something to stderr and then > exit. stderr doesn't exist, or point to a useful place, in many environments. And a forced exit() is no better than a crash for most purposes. > I don't think libpq should core dump an app by choice. The case that we are talking about is a bug, or would be a bug if it could happen (the fact that we've gotten along happily with no similar test in the existing code shows that it can't). Many forms of bug can result in core dumps; it's foolish to try to prevent them all. And bloating one line of code into five or more lines to defend against can't-happen cases is a good way to end up with unreadable, unmaintainable software. 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] libpq Win32 Mutex performance patch
Andrew Chernow <[EMAIL PROTECTED]> writes:
> The attached patch replaces the win32 mutex calls with critical section
> calls. The change will not affect the behavior of the windows
> pthread_xxx functions.
Why have you defined the lock/unlock functions as willing to fall
through silently if handed a null pointer? I think a crash in
such a case is what we *want*. Silently not locking is surely
not very safe.
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] Fix for win32 stat() problems
Alvaro Herrera <[EMAIL PROTECTED]> writes: > Andrew Dunstan wrote: >> How about #defining safe_stat to be pg_win32_safe_stat on Windows and >> simply stat elsewhere? Then use safe_stat at the places you consider >> critical. > I would couple this with a pgwin32_unsafe_stat on Windows, which changes > the size value to 0, so that if anyone gets it wrong it's immediately > obvious. It's only worth having two versions if someone can show that there's actually going to be a performance problem from the extra syscall. I don't believe we use stat() in any place where it's really gonna matter much ... 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] Fix for win32 stat() problems
Magnus Hagander <[EMAIL PROTECTED]> writes:
> A whole lot simpler patch :-)
Seems like you no longer need the !defined(_DIRMOD_C) bit here?
Also please include a comment about why has to be
forcibly included.
A more general question: can't we get rid of most of the #ifdef WIN32
cruft in include/port.h, and put it in include/port/win32.h instead?
Seems cleaner that way, at least for things where there's just an
#ifdef WIN32 hunk and not two cases for Win and not-Win.
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] Fix for win32 stat() problems
Magnus Hagander <[EMAIL PROTECTED]> writes:
> Trying to prepare a patch that does it the normal way, but so far I'm
> failing rather miserably. The *struct* stat is already redefined on
> win32, so whenever I try #undef or so it conflicts with that :-( Since
> there is no way to #undef only the parametrized version.
I don't follow ... there's no such redefinition in our code AFAICS.
Do you mean that the system header files declare it as a macro?
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] Fix for win32 stat() problems
Magnus Hagander <[EMAIL PROTECTED]> writes:
> Tom Lane wrote:
>> Ick.
> The reason not to do so was to avoid having to do the two filesystem
> calls for *every* stat, instead just calling them both when we actually
> need to use the st_size member.
I don't think that's worth (a) the code uglification, or (b) the chance
of a bug later due to someone not knowing they need the special version
of stat(). Are there any stat() calls that are in sufficiently
performance-critical paths that it really matters? How much slower is
the second call, anyway?
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] Fix for win32 stat() problems
Magnus Hagander <[EMAIL PROTECTED]> writes:
> + #ifndef WIN32
> if (stat(xlogpath, &stat_buf) == 0)
> + #else
> + if (pgwin32_safestat(xlogpath, &stat_buf) == 0)
> + #endif
Ick. Please do this the way we normally do things when we have to
override broken Windows syscalls, that is put something like
#define stat(...) pgwin32_stat(...)
into the win32 port header file, so the calls don't have to be
nonstandard.
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] EXPLAIN progress info
Gregory Stark <[EMAIL PROTECTED]> writes:
> I think a better way to get a real "percentage done" would be to add a method
> to each node which estimates its percentage done based on the percentage done
> its children report and its actual and expected rows and its costs.
You can spend a week inventing some complicated method, and the patch
will be rejected because it adds too much overhead. Anything we do here
has to be cheap enough that no one will object to having it turned on
all the time --- else it'll be useless exactly when they need it.
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] Concurrent psql patch
Andrew Dunstan <[EMAIL PROTECTED]> writes: > My understanding was that all items in a commit-fest have one of these > three dispositions: > . committed > . rejected > . referred back to author for more work Right. But Bruce's personal queue has got a different lifecycle: items get removed when they are resolved by a committed patch, or by being rejected as not wanted, or by being summarized on the public TODO list. For what he's doing that's a very good definition --- things don't get forgotten just because nothing has happened lately. But it's becoming clearer to me that the commit-fest queue has to be a separate animal. We used Bruce's queue as the base this time around, because we had no other timely-available source of the raw data. Seems like it's time to split them, though. If we do split them then there is going to be some added effort to maintain the commit fest queue. Bruce has made it pretty clear that he doesn't want to put in any extra cycles here. So someone else has to step up to the plate if this is going to work. Any volunteers out there? 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] Concurrent psql patch
Bruce Momjian <[EMAIL PROTECTED]> writes:
> Are you suggesting we just delete the threads and let them die if they
> don't submit a new version?
I am suggesting that they are not material for another commit fest until
some new work has been done. (And the appearance of that new work would
trigger an entry being made in the pending-commit-fest list.)
I've surely got no objection to you hanging on to such threads in your
personal almost-TODO list, and prodding people when appropriate. But
the commit fest queue is not for that purpose.
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] Concurrent psql patch
Bruce Momjian <[EMAIL PROTECTED]> writes: > This has been saved for the next commit-fest: > http://momjian.postgresql.org/cgi-bin/pgpatches_hold Er, why "saved"? Until there's a new patch submission there's not going to be more work to do on this in the next fest. I think maybe you need to think a bit harder about the distinction between your TODO-items-in-waiting list and the commit fest queue. I was willing to wade through a pile of TODO-items-in-waiting this time, because I pressed you to make the list available before having sorted through it; but I'm not going to be pleased to see those same threads in the fest queue next time, unless someone's done some actual work in between. 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] EXPLAIN progress info
Gregory Stark <[EMAIL PROTECTED]> writes: > There are downsides: Insurmountable ones at that. This one already makes it a non-starter: > a) the overhead of counting rows and loops is there for every query execution, > even if you don't do explain analyze. and you are also engaging in a flight of fantasy about what the client-side code might be able to handle. Particularly if it's buried inside, say, httpd or some huge Java app. Yeah, you could possibly make it work for the case that the problem query was manually executed in psql, but that doesn't cover very much real-world territory. You'd be far more likely to get somewhere with a design that involves looking from another session to see if anything's happening. In the case of queries that are making database changes, pgstattuple is certainly a usable option. For SELECT-only queries, I agree it's harder, but it's still possible. I seem to recall some discussion of including a low-overhead progress counter of some kind in the pg_stat_activity state exposed by a backend. The number of rows so far processed by execMain.c in the current query might do for the definition. 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] EXPLAIN progress info
Gregory Stark <[EMAIL PROTECTED]> writes:
> I know I should still be looking at code from the March Commitfest but I was
> annoyed by this *very* FAQ:
> http://archives.postgresql.org/pgsql-general/2008-04/msg00402.php
Seems like pg_relation_size and/or pgstattuple would solve his problem
better, especially since he'd not have to abort and restart the long
query to find out if it's making progress. It'd help if pgstattuple
were smarter about "dead" vs uncommitted tuples, though.
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] Headers dependencies cleanup
Zdenek Kotala <[EMAIL PROTECTED]> writes: > Alvaro Herrera napsal(a): >> Not all compilers like (== support) inline macros apparently. > Is it your assumption or do you mean some specific compiler? IIRC, inline is > defined in C99 and my assumption :-) is that it should be supported by all > compilers today. The problem is (1) not all compilers support inline, (2) the ones that do have divergent ideas on its semantics, and (3) the semantics specified by C99 utterly suck (cf tuplesort.c). gcc's traditional semantics for inline are far more usable. We have done "#ifdef gcc" inlines in one or two places where the performance argument for adding such clutter was compelling. We can do that some more, but you'll need equally compelling arguments. Do not bother submitting patches to create a very large number of inlines. 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] [HACKERS] Database owner installable modules patch
"Tom Dunstan" <[EMAIL PROTECTED]> writes:
> OK, I found an example that does NOT fit the "just drop all
> dependencies" scenario, but that I would still like to support. I just
> had a look at the postgis pl/java support, and its install does stuff
> like "SELECT sqlj.install_jar('file://${PWD}/postgis_pljava.jar',
> 'postgis_pljava_jar', false);" and "SELECT
> sqlj.add_type_mapping('geometry', 'org.postgis.pljava.PLJGeometry');".
> There's no way we can deal with that sort of thing automatically, so
> we'll have to support uninstall scripts regardless.
Well, that just begs the question of what those commands actually *do*.
It seems not unlikely that they'd be inserting data into tables that
would belong to the module, in which case an uninstall that dropped
the table would be fine.
I still like the idea of uninstall being just a "DROP MODULE" with
subsequent cascading. If you want to argue that that isn't sufficient
you really need a pretty convincing example why not.
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] WIP: plpgsql source code obfuscation
"Joshua D. Drake" <[EMAIL PROTECTED]> writes: > Bruce Momjian <[EMAIL PROTECTED]> wrote: >> Added to TODO: >> o Add ability to obfuscate function bodies > For the record. I think this todo is bogus. For the record, I think so too ;-). The agreed-on TODO wording makes no mention of what an acceptable implementation would look like, and that's because there very possibly *is* no generally-acceptable implementation. (Though if someone has a bright new idea, I'm sure we'd all listen.) The point of the TODO entry is just to acknowledge that this is an issue for some folk, and that a real solution would be welcomed. 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
