Re: [PATCHES] Patch to remove deadcode from dbcommands.c
Gavin Sherry wrote: This patch removes dead code from redo_dbase(). Barring any objections, I'll apply this tomorrow. I've removed XLOG_DBASE_CREATE_OLD and XLOG_DBASE_DROP_OLD from the header as well but have not changed the values of XLOG_DBASE_CREATE and XLOG_DBASE_DROP so as to avoid the need for an initdb. I'll do this as well and bump the catversion. -Neil ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] pgcrypto volatility and strictness changes
Michael Fuhr wrote: This patch updates the DDL for contrib/pgcrypto to create all functions as STRICT, and all functions except gen_salt() as IMMUTABLE. gen_salt() is VOLATILE. Barring any objections, I'll apply this tomorrow. -Neil ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
[PATCHES] A couple of patches for PostgreSQL 64bit support
Hi all, Here're a couple of patches for PostgreSQL 64bit support. There're just two extension to 64bit, size of shared memory and transaction ID. Please take a look at overview.txt for this proposal and pathces, based upon 8.0.3. Any discussions are welcome. -- --- Koichi Suzuki Open Source Engineeering Departmeent, NTT DATA Intellilink Corporation Phone: +81-3-5566-9628 WWW: http://www.intellilink.co.jp -- Proposal: 64bit Extension in PostgreSQL 8.0.x July 7th, 2005 Koichi Suzuki (NTT DATA Intellilink) 1. Background and Purpose 64-bit architecture CPU is getting more and more popular in Intel-based CPU, such as EM64T, AMD64 and IA64.Servers based upon such CPUs can provide much more memory available. Tens of gigabytes of memory is available in a node, typically 16gigabytes or so. Obviously, 32bit-based Linux and its applications can run on such a machine. However, from the point of each process' view, size of avilable memory is limited by process user space, typically 1GB for kernel and 3GB for each process. PostgreSQL's kernel uses shared memory to hold shared data and much more memory should be available as PostgreSQL is going to handle bigger database. For this purpose, we need to extend PostgreSQL to 64-bit program and make shared memory management to handle shared memory beyond 32bit limitation. On the other hand, PostgreSQL is going to be used in mission-critical systems in enterprise environment. In such application, we need to provide long period operation without stopping service. Currently, PostgreSQL's transaction ID is limited by 32bit integer and when it is about to run out, we have to stop the database operation and run vacuum freeze to reuse the older transaction ID value. Vacuum freeze operation scans all the database space and together with the bigger database size in oparation, it's going to take longer. Because PostgreSQL is going to be used in busier systems, transaction IDs tend to run out more earlier. To provide longer continuous operation, we need to make transaction ID 64bit-based. 2. How we can do? It is basically very simple to do these two extensions. We can locate their definitions and change them into 64-bit based ones. However, there are much much more to be done. We have to find all the lines which deals with their value and have to modify them so that there will not be any loss of calculation precision. Detailed result will be given later. 3. Environment Our code will assume the following: 1) PostgreSQL server (i.e. postmaster and postgres processes) should run only on 64-bit CPU based server machies, that is, EM64T, AMD64 and IA64. 2) Both 64bit (EM64T, AMD64 and IA64) and 32bit CPU(IA32) are allowed as clients. 3) Currently, we support only Linux 2.6.x kernel. 4. Specification changes Due to these two changes, PostgreSQL's spec will change as follows: 4.1 Shared memory size Shared memory size are specified in shared_buffers entry in postgresql.conf file. In 32-bit environment, it is limited to INTMAX/BLOCKSIZE. Now new limitation is INTMAX/2. This value specifies the number of blocks, so actual memory which can be specified by this parameter will be (INITMAX/2)*BLOCKSIZE. Typical BLOCKSIZE is 8KB. Therefore, typicak maximum shared memory will be 8TB. This is beyond the limitation of Linux user space for EM64T (512GB) and should be sufficient. The reason why shared memory size specification is limited to INITMAX/2 is as follows: In 8.0.x buffer management, (buffer_number)*2 is used to produce buffer IDs and whole these subsystem is still based on 32bit calculation. We'd like to keep the change as minimum as possible and having this limit will not have bad influence to the maxumum value of actual shared memory size. 4.2 Transaction ID (XID) Type of transaction ID is pre-defined in the catalog and there are no way for users to redefine its type. In this extension, transaction IDs are handled as follows: 1) On catalog, transaction ID's (such as XMIN and XMAX) are give the type XID, as in 32-bit environment. 2) If applications on 32-bit based environment trys to read the value of the type XID, it has to read this value as unsigned long long value. 3) In the case of 64-bit based environment, the value of gXIDh can be handled as unsigned long value, depending on compilers. 4) The length of the type XID is stored in the catalog pg_type. 4.3 Configure W have added two configuration options: --enable-64bit-shared-memory enables 64bit shared memory by defining USE_64BIT_SHARED. --enable-64bit-transaction-id enables 64-bit transaction id by defining USE_64BIT_XID. 5. Acknowledgement I'd like to thank Mr.Tom Lane, Mr.Bruce Momjan and Mr.Jan Wieck
[PATCHES] A couple of patches for PostgreSQL 64bit support
Hi, all, I have posted a couple of patches with regard to 64bit environment support to PATCHES ml. It expands size of shared memory to 64bit space and extends XID to 64bit. Please take a look at it. -- --- Koichi Suzuki Open Source Engineeering Departmeent, NTT DATA Intellilink Corporation Phone: +81-3-5566-9628 WWW: http://www.intellilink.co.jp -- ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
[PATCHES] pgcrypto: OpenSSL/DES cleanup
As Kris Jurka found out, pgcrypto does not work with
OpenSSL 0.9.6x. The DES functions use the older 'des_'
API, but the newer 3DES functions use the 0.9.7x-only
'DES_' API.
I think I just used /usr/include/openssl/des.h for reference
when implementing them, and had upgraded OpenSSL in the
meantime.
Following patch converts DES also to newer API and provides
compatibility functions for OpenSSL 0.9.7.
I chose this route because:
- openssl.c uses few DES functions.
- compatibility for old 'des_' API is going away at some point
of time from OpenSSL.
- as seen from macros, new API is saner
- Thus pgcrypto supports any OpenSSL version from 0.9.5 to 1.0
Tested with OpenSSL 0.9.6c and 0.9.7e.
--
marko
PS. It's nice to see that the 'autoconfiguration' already pays
back.
Index: contrib/pgcrypto/openssl.c
===
RCS file: /opt/arc/cvs2/pgsql/contrib/pgcrypto/openssl.c,v
retrieving revision 1.20
diff -u -c -r1.20 openssl.c
*** contrib/pgcrypto/openssl.c 5 Jul 2005 18:15:36 - 1.20
--- contrib/pgcrypto/openssl.c 7 Jul 2005 09:18:37 -
***
*** 48,53
--- 48,73
#endif
/*
+ * Compatibility with older OpenSSL API for DES.
+ */
+ #if OPENSSL_VERSION_NUMBER 0x00907000L
+ #define DES_key_schedule des_key_schedule
+ #define DES_cblock des_cblock
+ #define DES_set_key(k, ks) \
+ des_set_key((k), *(ks))
+ #define DES_ecb_encrypt(i, o, k, e) \
+ des_ecb_encrypt((i), (o), *(k), (e))
+ #define DES_ncbc_encrypt(i, o, l, k, iv, e) \
+ des_ncbc_encrypt((i), (o), (l), *(k), (iv), (e))
+ #define DES_ecb3_encrypt(i, o, k1, k2, k3, e) \
+ des_ecb3_encrypt((des_cblock *)(i), (des_cblock *)(o), \
+ *(k1), *(k2), *(k3), (e))
+ #define DES_ede3_cbc_encrypt(i, o, l, k1, k2, k3, iv, e) \
+ des_ede3_cbc_encrypt((i), (o), \
+ (l), *(k1), *(k2), *(k3), (iv), (e))
+ #endif
+
+ /*
* Hashes
*/
static unsigned
***
*** 175,185
} bf;
struct
{
! des_key_schedule key_schedule;
} des;
struct
{
! des_key_schedule k1, k2, k3;
} des3;
CAST_KEYcast_key;
#ifdef GOT_AES
--- 195,205
} bf;
struct
{
! DES_key_schedule key_schedule;
} des;
struct
{
! DES_key_schedule k1, k2, k3;
} des3;
CAST_KEYcast_key;
#ifdef GOT_AES
***
*** 315,325
ossl_des_init(PX_Cipher * c, const uint8 *key, unsigned klen, const uint8 *iv)
{
ossldata *od = c-ptr;
! des_cblock xkey;
memset(xkey, 0, sizeof(xkey));
memcpy(xkey, key, klen 8 ? 8 : klen);
! des_set_key(xkey, od-u.des.key_schedule);
memset(xkey, 0, sizeof(xkey));
if (iv)
--- 335,345
ossl_des_init(PX_Cipher * c, const uint8 *key, unsigned klen, const uint8 *iv)
{
ossldata *od = c-ptr;
! DES_cblock xkey;
memset(xkey, 0, sizeof(xkey));
memcpy(xkey, key, klen 8 ? 8 : klen);
! DES_set_key(xkey, od-u.des.key_schedule);
memset(xkey, 0, sizeof(xkey));
if (iv)
***
*** 338,346
ossldata *od = c-ptr;
for (i = 0; i dlen / bs; i++)
! des_ecb_encrypt((des_cblock *) (data + i * bs),
! (des_cblock *) (res + i * bs),
! od-u.des.key_schedule, 1);
return 0;
}
--- 358,366
ossldata *od = c-ptr;
for (i = 0; i dlen / bs; i++)
! DES_ecb_encrypt((DES_cblock *) (data + i * bs),
! (DES_cblock *) (res + i * bs),
! od-u.des.key_schedule, 1);
return 0;
}
***
*** 353,361
ossldata *od = c-ptr;
for (i = 0; i dlen / bs; i++)
! des_ecb_encrypt((des_cblock *) (data + i * bs),
! (des_cblock *) (res + i * bs),
! od-u.des.key_schedule, 0);
return 0;
}
--- 373,381
ossldata *od = c-ptr;
for (i = 0; i dlen / bs; i++)
! DES_ecb_encrypt((DES_cblock *) (data + i * bs),
! (DES_cblock *) (res + i * bs),
! od-u.des.key_schedule, 0);
return 0;
}
***
***
Re: [PATCHES] pgcrypto: OpenSSL/DES cleanup
On Thu, Jul 07, 2005 at 12:25:53PM +0300, Marko Kreen wrote: Tested with OpenSSL 0.9.6c and 0.9.7e. I just applied this patch to my system running HEAD and OpenSSL 0.9.8; all regression tests passed. BTW, OpenSSL 0.9.8 has been released: http://www.mail-archive.com/openssl-announce@openssl.org/msg00063.html -- Michael Fuhr http://www.fuhr.org/~mfuhr/ ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] A couple of patches for PostgreSQL 64bit support
Koichi Suzuki [EMAIL PROTECTED] writes: Here're a couple of patches for PostgreSQL 64bit support. There're just two extension to 64bit, size of shared memory and transaction ID. I asked originally for some experimental evidence showing any value in having more than 2Gb of shared buffers. In the absence of any convincing demonstration, I'm not very inclined to worry about whether we can handle wider-than-int shared memory size. As for the XID change, I don't think this patch accurately reflects the size of the impact. There are a lot of things that in practice need to be the same size as XID (CID, most obviously, but I suspect also OID). And again, some demonstration of the performance impact would be appropriate. Here, not only do you have to prove that widening XID isn't a big performance hit in itself, but you also have to convince us that it's a win compared to the existing approach of vacuuming at least every billion transactions. regards, tom lane ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] A couple of patches for PostgreSQL 64bit support
Tom Lane wrote: Koichi Suzuki [EMAIL PROTECTED] writes: Here're a couple of patches for PostgreSQL 64bit support. There're just two extension to 64bit, size of shared memory and transaction ID. I asked originally for some experimental evidence showing any value in having more than 2Gb of shared buffers. In the absence of any convincing demonstration, I'm not very inclined to worry about whether we can handle wider-than-int shared memory size. There is some practical evidence. Recently the number of large boxes in the field is almost growing exponentially. Today I have heard somebody say this box has 'just 4 gig of ram' . On large installations we have already seen problems with too small caches (= 2gb). Surprisingly this has turned out to be a quite important issue in the field. Tests have shown that the cache provided by the OS is a lot worse for the database. 64-bit XIDs seem to be an overkill - the only practical impact I can see is an even larger tuple header (this can be an issue on large boxes too - at least compared to Oracle). Best regards, Hans ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] A couple of patches for PostgreSQL 64bit support
=?ISO-8859-1?Q?Hans-J=FCrgen_Sch=F6nig?= [EMAIL PROTECTED] writes: There is some practical evidence. Recently the number of large boxes in the field is almost growing exponentially. Today I have heard somebody say this box has 'just 4 gig of ram' . On large installations we have already seen problems with too small caches (= 2gb). Surprisingly this has turned out to be a quite important issue in the field. Tests have shown that the cache provided by the OS is a lot worse for the database. *What* tests? This is all handwaving :-( What I would find credible is a set of, say, OSDL test runs, showing a continuing increase of performance with shared_buffers right up to the 2Gb limit. Everything done to date says that you hit the point of diminishing returns well before that. regards, tom lane ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] Dependencies on shared objects
On Tue, Jul 05, 2005 at 03:51:32PM -0400, Tom Lane wrote: Alvaro Herrera [EMAIL PROTECTED] writes: On Tue, Jul 05, 2005 at 02:47:15PM -0400, Tom Lane wrote: Although I don't have any particular objection to the OWNER/NORMAL distinction, your explanation doesn't seem to make sense. Don't you have to poke the Acl anyway, if it's non-null? Else the grantor values will be wrong. Hum, we don't register a dependency on the owner when registering dependencies from the Acl -- we actively skip that (because we know the owner has an entry of the other type already). Is this still an issue? Not sure. ISTM the distinction we want to capture is more along the lines of DEPENDENCY_NORMAL vs DEPENDENCY_AUTO --- that is, we should be willing to auto-drop grants to a user when dropping the user, but not be willing to auto-drop objects unless the drop is with CASCADE. Also, grants from the user to someone else (using grant options) probably shouldn't go away without CASCADE either, though this is maybe debatable. If you believe the latter then the OWNER/ACL division is clearly the wrong way to think about it. So I'd be inclined to use the NORMAL/AUTO terminology instead. I'm not sure about DROP USER CASCADE. There's an inherent asymmetry in that command, because as soon as the user has dependencies in some other database, you cannot do anything, and the cascade will fail. So I'd rather not offer a DROP USER CASCADE, but just reject the command when there are any dependencies, and provide a command to drop the owned objects (DROP OWNED), or to give them to someone else (REASSIGN OWNED -- better wording?). An important distinction to be made is which dependencies are auto and which ones are normal. It's clear that OWNER entries cannot be AUTO. OTOH grantee entries in the ACL can be (have to be) dropped automatically, but what to do about grantor ones? We may have to do a distinction. We could drop the grants for which the dropped user is the grantor; but not automatically, I think. -- Alvaro Herrera (alvherre[a]alvh.no-ip.org) Postgres is bloatware by design: it was built to house PhD theses. (Joey Hellerstein, SIGMOD annual conference 2002) ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] A couple of patches for PostgreSQL 64bit support
Tom Lane wrote: =?ISO-8859-1?Q?Hans-J=FCrgen_Sch=F6nig?= [EMAIL PROTECTED] writes: There is some practical evidence. Recently the number of large boxes in the field is almost growing exponentially. Today I have heard somebody say this box has 'just 4 gig of ram' . On large installations we have already seen problems with too small caches (= 2gb). Surprisingly this has turned out to be a quite important issue in the field. Tests have shown that the cache provided by the OS is a lot worse for the database. *What* tests? This is all handwaving :-( What I would find credible is a set of, say, OSDL test runs, showing a continuing increase of performance with shared_buffers right up to the 2Gb limit. Everything done to date says that you hit the point of diminishing returns well before that. regards, tom lane well, you can easily try it on a big machine with gigs of ram and nothing but the database running. using a very low number of shared buffers will lead to worse performance than many shared buffers - even if the operating system caches some disk i/O (which is done by linux if nobody else want to have some ram). i have no public hard figures i could post here but customers have told me that 2Q cache vs. kernel cache is around 5-10 times faster (it varies of course). the 2gb thing is especially important for data crunchers - not necessarily for 'normal' OLTP databases. just assume somebody getting 5 gig of data and doing some repeated computation with this data. you definitely don't want to go to disk in this case. people will assume that postgresql can work with large caches (it is a good database - why do i get errors on startup - this is the usual story). people rather tend to rely on PostgreSQL than on some operating system thing ;). i might have some time to provide some real hard facts to prove this but i am a bit busy at the moment. best regards, hans ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] A couple of p.tches for PostgreSQL 64bit support
On Thu, Jul 07, 2005 at 06:23:40PM +0200, Hans-Jürgen Schönig wrote: Tom Lane wrote: Koichi Suzuki [EMAIL PROTECTED] writes: Here're a couple of patches for PostgreSQL 64bit support. There're just two extension to 64bit, size of shared memory and transaction ID. I asked originally for some experimental evidence showing any value in having more than 2Gb of shared buffers. In the absence of any convincing demonstration, I'm not very inclined to worry about whether we can handle wider-than-int shared memory size. There is some practical evidence. Recently the number of large boxes in the field is almost growing exponentially. Today I have heard somebody say this box has 'just 4 gig of ram' . Yes, but the point is if it's a good idea to have that many shared buffers. Is there a measurable difference between that, and leaving the extra memory for the kernel to manage cache? Remember that there have been measurements that showed, for previous releases, that having shared buffers set too high was detrimental to performance. So the first thing to do is present results showing that this is no longer true. This could very well be the case, because of the rewriting of the buffer manager. -- Alvaro Herrera (alvherre[a]alvh.no-ip.org) El destino baraja y nosotros jugamos (A. Schopenhauer) ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] Dependencies on shared objects
Alvaro Herrera [EMAIL PROTECTED] writes: On Tue, Jul 05, 2005 at 03:51:32PM -0400, Tom Lane wrote: An important distinction to be made is which dependencies are auto and which ones are normal. It's clear that OWNER entries cannot be AUTO. OTOH grantee entries in the ACL can be (have to be) dropped automatically, but what to do about grantor ones? We may have to do a distinction. We could drop the grants for which the dropped user is the grantor; but not automatically, I think. For the moment I left this alone; you can revisit the question when you do DROP OWNED or whatever we end up calling it. I've committed the patch with some revisions --- principally that I took out tracking of dependencies on tablespaces, as I couldn't see any real argument that we need to track that separately from the filesystem structure. (I was also a bit concerned about whether copying a database to a different default tablespace would work correctly.) I ended up calling the dependency types OWNER and ACL; there is no NORMAL at the moment. Obviously we'd need to add more dependency types to track dependencies on anything but roles. The reason for this is that getting the ALTER OWNER case right requires removing ACL dependency entries for the new owner, and I didn't like the idea of issuing a blanket removal except against a very specifically defined dependency type. Also, I figured out why you needed the separate OWNER dependency type: it's so you can tell which entry to update during ALTER OWNER. This is hopefully a bit better documented now. regards, tom lane ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] A couple of patches for PostgreSQL 64bit support
I have some experimeltal data about this extension. I will gather it and post hopefully this weekend. Tom Lane wrote: Koichi Suzuki [EMAIL PROTECTED] writes: Here're a couple of patches for PostgreSQL 64bit support. There're just two extension to 64bit, size of shared memory and transaction ID. I asked originally for some experimental evidence showing any value in having more than 2Gb of shared buffers. In the absence of any convincing demonstration, I'm not very inclined to worry about whether we can handle wider-than-int shared memory size. As for the XID change, I don't think this patch accurately reflects the size of the impact. There are a lot of things that in practice need to be the same size as XID (CID, most obviously, but I suspect also OID). And again, some demonstration of the performance impact would be appropriate. Here, not only do you have to prove that widening XID isn't a big performance hit in itself, but you also have to convince us that it's a win compared to the existing approach of vacuuming at least every billion transactions. regards, tom lane -- --- Koichi Suzuki Open Source Engineeering Departmeent, NTT DATA Intellilink Corporation Phone: +81-3-5566-9628 WWW: http://www.intellilink.co.jp -- ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] A couple of patches for PostgreSQL 64bit support
Hans-J|rgen Schvnig [EMAIL PROTECTED] wrote: 64-bit XIDs seem to be an overkill - the only practical impact I can see is an even larger tuple header (this can be an issue on large boxes too - at least compared to Oracle). I agreed, too. The changes of XIDs cannot be ignored because the overhead will be 32bytes per tuple. Avoiding overheads, can XIDs/CIDs be different bit length? For example, can XIDs/CIDs be changed to 48/16-bit or 40/24-bit? --- ITAGAKI Takahiro NTT Cyber Space Laboratories ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] A couple of p.tches for PostgreSQL 64bit support
On Fri, Jul 08, 2005 at 09:38:07AM +0900, ITAGAKI Takahiro wrote: Hans-J|rgen Schvnig [EMAIL PROTECTED] wrote: 64-bit XIDs seem to be an overkill - the only practical impact I can see is an even larger tuple header (this can be an issue on large boxes too - at least compared to Oracle). I agreed, too. The changes of XIDs cannot be ignored because the overhead will be 32bytes per tuple. Avoiding overheads, can XIDs/CIDs be different bit length? For example, can XIDs/CIDs be changed to 48/16-bit or 40/24-bit? Not unless you change the definition of HeapTupleFields (src/include/access/htup.h). Alignment concerns would probably bite you if you changed it, anyway. -- Alvaro Herrera (alvherre[a]alvh.no-ip.org) Ninguna manada de bestias tiene una voz tan horrible como la humana (Orual) ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send unregister YourEmailAddressHere to [EMAIL PROTECTED])
Re: [PATCHES] A couple of p.tches for PostgreSQL 64bit support
Alvaro Herrera [EMAIL PROTECTED] writes: On Fri, Jul 08, 2005 at 09:38:07AM +0900, ITAGAKI Takahiro wrote: Avoiding overheads, can XIDs/CIDs be different bit length? For example, can XIDs/CIDs be changed to 48/16-bit or 40/24-bit? Not unless you change the definition of HeapTupleFields (src/include/access/htup.h). Alignment concerns would probably bite you if you changed it, anyway. I don't think we could feasibly reduce the width of CID anyway; we've already seen a few complaints of people overrunning 2^32 commands per transaction, and surely this is a bigger rather than a lesser concern if you are thinking of large databases. It probably would be possible to keep CID at 32 bits and lay out the HeapTupleHeader so that you only pay for three, not four, 64-bit fields ... but that's still twelve bytes added per tuple. regards, tom lane ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] patch: garbage error strings in libpq
Neil Conway wrote: Tom Lane wrote: I think this is all irrelevant language-lawyering; jtv spotted the true problem which is that we do not protect errno during the *first* call of libpq_gettext. I think you're missing the point. Obviously the current code is wrong, the debate is over the best way to fix it. Jeroen's interpretation of the spec suggests that merely having libpq_gettext() preserve errno is not sufficient. I'm not convinced this his interpretation is correct, but it is a question worth resolving. Agree totally. If my interpretation is wrong then I'll happily get on with my life and let everyone else do the same. I was at that point once already, but Neil took another close look at the relevant part of the C standard he dug up and found this potential problem. I really don't like playing the smart-alec language lawyer here, but I've been following compiler developments and they are moving in a direction that makes this relevant. I do want to be sure that we're shipping correct code, not just code that practically speaking suppresses the symptoms of its bugs for a while, on most compilers, for the most popular CPU architectures. Moreover, I don't want to go through all of this again when the regression occurs and we think we've solved it forever and the problem must be somewhere else. I've been losing enough sleep over what I thought must be bugs in libpqxx that I just couldn't put my finger on. Jeroen ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] Patch to remove deadcode from dbcommands.c
Gavin Sherry wrote: This patch removes dead code from redo_dbase(). The code processes CREATE/DROP DATABASE recovery records which are of types XLOG_DBASE_CREATE_OLD and XLOG_DBASE_DROP_OLD. We do not create such records. Applied with additional fixes (you forgot to remove the reference to the old #defines in dbase_desc()) -- thanks for the patch. Catversion bumped. -Neil ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] pgcrypto volatility and strictness changes
Michael Fuhr wrote: This patch updates the DDL for contrib/pgcrypto to create all functions as STRICT, and all functions except gen_salt() as IMMUTABLE. gen_salt() is VOLATILE. Applied, thanks. -Neil ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] User's exception plpgsql
Neil Conway wrote: Ah, I see. I would be content to allow opt_sqlstate to be either a string literal, a T_WORD (predefined error condition), or a TEXT variable. If users need to throw a sqlstate that is derived from a SQL expression, they can always assign to a TEXT variable and then specify that variable to RAISE. RAISE [ opt_sqlstate ] LEVEL 'fmt' [ , expr ... ] BTW, do have we reached a consensus on this? -Neil ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] User's exception plpgsql
On Fri, 8 Jul 2005, Neil Conway wrote: Neil Conway wrote: Ah, I see. I would be content to allow opt_sqlstate to be either a string literal, a T_WORD (predefined error condition), or a TEXT variable. If users need to throw a sqlstate that is derived from a SQL expression, they can always assign to a TEXT variable and then specify that variable to RAISE. RAISE [ opt_sqlstate ] LEVEL 'fmt' [ , expr ... ] BTW, do have we reached a consensus on this? -Neil ok, but don't forget, please, on exception part. Pavel ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] User's exception plpgsql
Neil Conway [EMAIL PROTECTED] writes: BTW, do have we reached a consensus on this? Doesn't look that way --- I tend to agree with you that we could avoid inventing declared exceptions at all, but Pavel is definitely not happy with it, and AFAIR no one else has weighed in. Maybe we need to take the discussion back to pghackers to draw a wider audience. regards, tom lane ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] User's exception plpgsql
Pavel Stehule wrote: ok, but don't forget, please, on exception part. What do you mean? -Neil ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] User's exception plpgsql
On Fri, 8 Jul 2005, Neil Conway wrote: Pavel Stehule wrote: ok, but don't forget, please, on exception part. What do you mean? -Neil BEGIN EXCEPTION WHEN * THEN equvalent rules for raise have to be in * is true? Pavel ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] User's exception plpgsql
On Fri, 8 Jul 2005, Tom Lane wrote: Neil Conway [EMAIL PROTECTED] writes: BTW, do have we reached a consensus on this? Doesn't look that way --- I tend to agree with you that we could avoid inventing declared exceptions at all, but Pavel is definitely not happy with it, and AFAIR no one else has weighed in. Maybe we need to take the discussion back to pghackers to draw a wider audience. I am not happy (this is only half of step), but I don't expect better discussion. My opinion is so exception variable has more possibilities, but this solution is usefull and funkcional too. And we can introduce exception variables later without problems if will be good time. Discussion on pghackers was, but not too much people contributed. And more, I don't see user's exception as big qustion this days. regards, tom lane ---(end of broadcast)--- TIP 6: explain analyze is your friend
[PATCHES] minor nodeIndexScan tweak
Per EDB's Coverity analysis, runtimeKeyInfo is only non-NULL if
econtext is also non-NULL, so we can eliminate a conditional on the
former by moving it inside an if block for the latter. Per discussion
of earlier similar changes, this is not for performance reasons but for
code clarity.
Barring any objections I'll apply this tonight or tomorrow.
-Neil
Index: src/backend/executor/nodeIndexscan.c
===
RCS file: /Users/neilc/local/cvs/pgsql/src/backend/executor/nodeIndexscan.c,v
retrieving revision 1.103
diff -c -r1.103 nodeIndexscan.c
*** src/backend/executor/nodeIndexscan.c 6 May 2005 17:24:54 - 1.103
--- src/backend/executor/nodeIndexscan.c 8 Jul 2005 05:06:41 -
***
*** 208,221
* recalculate *all* runtime keys on each call.
*/
ResetExprContext(econtext);
- }
! /*
! * If we are doing runtime key calculations (ie, the index keys depend
! * on data from an outer scan), compute the new key values
! */
! if (runtimeKeyInfo)
! {
ExecIndexEvalRuntimeKeys(econtext,
runtimeKeyInfo,
scanKeys,
--- 208,219
* recalculate *all* runtime keys on each call.
*/
ResetExprContext(econtext);
! /*
! * If we are doing runtime key calculations (ie, the index
! * keys depend on data from an outer scan), compute the new
! * key values
! */
ExecIndexEvalRuntimeKeys(econtext,
runtimeKeyInfo,
scanKeys,
***
*** 603,610
Assert(leftop != NULL);
! if (!(IsA(leftop, Var)
! var_is_rel((Var *) leftop)))
elog(ERROR, indexqual doesn't have key on left side);
varattno = ((Var *) leftop)-varattno;
--- 601,607
Assert(leftop != NULL);
! if (!(IsA(leftop, Var) var_is_rel((Var *) leftop)))
elog(ERROR, indexqual doesn't have key on left side);
varattno = ((Var *) leftop)-varattno;
---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] minor nodeIndexScan tweak
Neil Conway [EMAIL PROTECTED] writes: Per EDB's Coverity analysis, runtimeKeyInfo is only non-NULL if econtext is also non-NULL, so we can eliminate a conditional on the former by moving it inside an if block for the latter. This is premature optimization par excellance --- it saves one if-test, in a not very performance-critical routine. I disagree that it improves the code clarity: it's far from obvious that being-passed-an-outer-tuple is the same condition as has-runtime-keys, and I can think of numerous possible future changes that would render the conditions distinct again. So please, don't do it. ! if (!(IsA(leftop, Var) ! var_is_rel((Var *) leftop))) elog(ERROR, indexqual doesn't have key on left side); ! if (!(IsA(leftop, Var) var_is_rel((Var *) leftop))) elog(ERROR, indexqual doesn't have key on left side); Just FYI, the reason for the line break is that pgindent will make it look uglier if it's all on one line. The IsA construct seems to confuse pgindent somehow ... regards, tom lane ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] patch: garbage error strings in libpq
Tom Lane wrote: (1) The fact that gettext works at all seems to me to be sufficient empirical evidence that it's a working solution. Tom, you have GOT to be joking. I agree with some of the things you say (see below), but here you're just not making sense. Consider: 1. We're only talking about a very specific property of gettext(), namely restoration of errno in calls that are not ordered w.r.t. a use of errno. This is only relevant in a very limited number of all calls to gettext(), so the mere fact that gettext() works doesn't prove anything relevant. 2. In those limited few cases, and for this specific property, we already know that something in this general area is *not* working, so you'd have to find, fix and test that before you could even make this argument. 3. Given that a call to gettext() crosses not just object-file but actual library boundaries, what do you think the chances are--regardless of language garantees and compiler aggressiveness--that we'd see the compiler interleaving instructions from gettext() with a use of errno on the other side of that boundary? Does the assumption that gettext() works at all, therefore, make a reliable indication that there is no problem? 4. In the case of libpq_gettext(), we're not crossing library boundaries. We're not even crossing object-file boundaries in some cases. That makes the chances of instructions being scheduled across a call much greater--and the question about sequence points much more relevant. 5. From what I understand, the latest versions of gcc are actually beginning to schedule instructions across function call boundaries. That will undoubtedly increase in the future. There is even a new feature that can, as far as I can see, lead to instruction scheduling across source file boundaries. (2) Whether there are sequence points in the function call or not, there most certainly are sequence points inside each called function. The spec allows the functions involved to be called in an unspecified order, but it doesn't allow the compiler to interleave the execution of several functions. That would answer the big question here, but where does it say that? I saw Neil's point that the sequence points before function calls apply for the nested calls as well as the outer one, but there is no ordering between those nested-call sequence points. It's all easy when you have a total ordering, but we're in a partial ordering here. So the missing piece of the puzzle is still that same question. Obviously the compiler isn't allowed to interleave function executions (or at least not let you find out about it) if there is a sequence point _between_ them. There is in sequential calls, for example, because there is a sequence point before the function is entered. But what happens if there isn't a separating sequence point, like here? (3) Interpretation or not, the approach that Jeroen proposes is unmaintainable; people will not remember to use such a kluge everywhere they'd need to. I'd much rather fix it in one place and do whatever we have to do to keep the compiler from breaking that one place. That means you haven't seen the approach I suggested the day before yesterday, where I also explained that I felt it best to fix the incumbent bugs first before refactoring the result. You're still talking about my initial quick-fix patch, which IMHO could have gone in already while we were arguing over a long-term solution. That patch was ugly solely in the sense that the original broken code was ugly; if you want to use words like kluge then please direct them there. The copy-paste style of writing loops didn't help either. As I suggested on Wednesday, maybe we can wrap the combination of printfPQExpBuffer() and libpq_gettext() into a single function that takes a PGconn *, a format string, and varargs. This makes the calls a lot shorter and simpler, it moots the question of sequence points because errno would be the first thing to be evaluated, and en passant we'll fix a few cases where we forgot to call libpq_gettext(). We'd have to have a similar wrapper for snprintf(), but then I think all cases in libpq are covered. Jeroen ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
