Re: [HACKERS] Report search_path value back to the client.
Hi,
On Tue, Dec 2, 2014 at 5:59 PM, Alexander Kukushkin cyberd...@gmail.com wrote:
It's really crazy to keep so many (hundreds) connections to the database and
it would be much better to have something like pgbouncer in front of
postgres.
Right now it's not possible, because pgbouncer is not aware of search_path
and it's not really possible to fix pgbouncer, because there is no easy way
to
get current value of search_path.
I would like to mark 'search_path' as GUC_REPORT:
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2904,7 +2904,7 @@ static struct config_string ConfigureNamesString[] =
{search_path, PGC_USERSET, CLIENT_CONN_STATEMENT,
gettext_noop(Sets the schema search order for names
that are not schema-qualified.),
NULL,
- GUC_LIST_INPUT | GUC_LIST_QUOTE
+ GUC_LIST_INPUT | GUC_LIST_QUOTE | GUC_REPORT
},
namespace_search_path,
\$user\,public,
I know quite a lot of companies using search_path to switch to the
version of sprocs in the database
without changes to the application. Not being able to use pgbouncer in
such scenario is rather annoying.
It would be really great to have this functionality in the upcoming
9.5 and not wait for another year for it.
Given this is a one-liner, which doesn't introduce any new code, but
one flag to the function call, would it be
possible to review it as a part of the current commitfest?
Kind regards,
--
Alexey Klyukin
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] implement subject alternative names support for SSL connections
On Fri, Sep 12, 2014 at 4:20 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: Hmm. If that's what the browsers do, I think we should also err on the side of caution here. Ignoring the CN is highly unlikely to cause anyone a problem; a CA worth its salt should not issue a certificate with a CN that's not also listed in the SAN section. But if you have such a certificate anyway for some reason, it shouldn't be too difficult to get a new certificate. Certificates expire every 1-3 years anyway, so there must be a procedure to renew them anyway. Committed, with that change, ie. the CN is not checked if SANs are present. Thanks for bearing through all these iterations! Great news! Thank you very much for devoting your time and energy to the review and providing such a useful feedback! On the CN thing, I don't have particularly strong arguments for either of the possible behaviors, so sticking to RFC makes sense here Sincerely, -- Alexey Klyukin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] implement subject alternative names support for SSL connections
On Mon, Sep 15, 2014 at 10:23 AM, Alexey Klyukin al...@hintbits.com wrote:
On Fri, Sep 12, 2014 at 4:20 PM, Heikki Linnakangas
hlinnakan...@vmware.com wrote:
Hmm. If that's what the browsers do, I think we should also err on the
side of caution here. Ignoring the CN is highly unlikely to cause anyone
a problem; a CA worth its salt should not issue a certificate with a CN
that's not also listed in the SAN section. But if you have such a
certificate anyway for some reason, it shouldn't be too difficult to get
a new certificate. Certificates expire every 1-3 years anyway, so there
must be a procedure to renew them anyway.
Committed, with that change, ie. the CN is not checked if SANs are present.
Actually, I disagree with the way the patch ignores the CN. Currently,
it skips the
CN unconditionally if the SubjectAltName section is present. But what
RFC 6125 says
is:
If a subjectAltName extension of type dNSName is present, that MUST
be used as the identity. Otherwise, the (most specific) Common Name
field in the Subject field of the certificate MUST be used.
This means that we have to check that at least one dNSName resource is
present before
rejecting to examine the CN. Attached is a one-liner (excluding
comments) that fixes this.
Regards,
Alexey
diff --git a/src/interfaces/libpq/fe-secure-openssl.c
b/src/interfaces/libpq/fe-secure-openssl.c
new file mode 100644
index 98d02b6..dd4fab8
*** a/src/interfaces/libpq/fe-secure-openssl.c
--- b/src/interfaces/libpq/fe-secure-openssl.c
*** verify_peer_name_matches_certificate(PGc
*** 626,637
sk_GENERAL_NAME_free(peer_san);
}
/*
!* If there is no subjectAltName extension, check the Common Name.
*
!* (Per RFC 2818 and RFC 6125, if the subjectAltName extension is
present,
* the CN must be ignored.)
*/
! else
{
X509_NAME *subject_name;
--- 626,637
sk_GENERAL_NAME_free(peer_san);
}
/*
!* If there is no subjectAltName extension of type dNSName, check the
Common Name.
*
!* (Per RFC 2818 and RFC 6125, if the subjectAltName extension of type
dNSName is present,
* the CN must be ignored.)
*/
! if (names_examined == 0)
{
X509_NAME *subject_name;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] implement subject alternative names support for SSL connections
On Mon, Sep 8, 2014 at 8:04 PM, Heikki Linnakangas
hlinnakan...@vmware.com wrote:
On 09/05/2014 07:30 PM, Alexey Klyukin wrote:
The error does not state whether the names comes from the CN or from
the SAN section.
I'd reword that slightly, to:
psql: server certificate for example.com (and 2 other names) does not
match host name example-foo.com
I never liked the current wording, server name foo very much. I think
it's important to use the word server certificate in the error message, to
make it clear where the problem is.
+1
For translations, that message should be pluralized, but there is no
libpq_ngettext macro equivalent to ngettext(), like libpq_gettext. I wonder
if that was left out on purpose, or if we just haven't needed that in libpq
before. Anyway, I think we need to add that for this.
Well, the translation guidelines in the documentation suggest avoiding
plurals if possible, but I don't like rephrasing the sentence with
'names total: %d', so attached is my attempt to add the function.
I have also moved the one-time library binding code to the function of its own.
This version also checks for the availability of the subject name, it
looks like RFC 5280 does not require it at all.
4.1.2.6. Subject
The subject field identifies the entity associated with the public
key stored in the subject public key field. The subject name MAY be
carried in the subject field and/or the subjectAltName extension.
Ok.
The pattern of allocating the name in the subroutine and then
reporting it (and releasing when necessary) in the main function is a
little bit ugly, but it looks to me the least ugly of anything else I
could come up (i.e. extracting those names at the time the error
message is shown).
I reworked that a bit, see attached. What do you think?
Thank you, I like your approach of unconditionally allocating and
freeing memory, it makes the code easier to read.
2 minor changes I've made in v7 (in addition to adding the libpq_ngettext):
- the verify_peer_name_matches_certificate does not try to return -1
(since it returns bool, it would be misinterpreted as true)
- removed the !got_error !found_match check from the for loop
header to the loop body per style comment from Tom in the beginning of
this thread.
I think this is ready for commit, except for two things:
1. The pluralization of the message for translation
2. I still wonder if we should follow the RFC 6125 and not check the Common
Name at all, if SANs are present. I don't really understand the point of
that rule, and it seems unlikely to pose a security issue in practice,
because surely a CA won't sign a certificate with a bogus/wrong CN, because
an older client that doesn't look at the SANs at all would use the CN
anyway. But still... what does a Typical Web Browser do?
At least Firefox and Safari seem to follow RFC strictly, according to
some anecdotical evidences (I haven't got enough time to dig into the
source code yet):
http://superuser.com/questions/230136/primary-common-name-in-subjectaltname
https://bugzilla.mozilla.org/show_bug.cgi?id=238142
Chrome seem to follow them, so the major open-source browsers are
ignoring the Common Name if the SubjectAltName is present.
Still, I see no win in ignoring CN (except for the shorter code), it
just annoys some users that forgot to put the CN name also in the SAN
section, so my 5 cents is that we should check both.
Regards,
--
Alexey Klyukin
diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
new file mode 100644
index fc930bd..a082268
*** a/src/interfaces/libpq/fe-misc.c
--- b/src/interfaces/libpq/fe-misc.c
*** static int pqSendSome(PGconn *conn, int
*** 64,69
--- 64,71
static int pqSocketCheck(PGconn *conn, int forRead, int forWrite,
time_t end_time);
static intpqSocketPoll(int sock, int forRead, int forWrite, time_t
end_time);
+ static void libpq_bindomain();
+
/*
* PQlibVersion: return the libpq version number
*** PQenv2encoding(void)
*** 1210,1223
#ifdef ENABLE_NLS
! char *
! libpq_gettext(const char *msgid)
{
static bool already_bound = false;
if (!already_bound)
{
! /* dgettext() preserves errno, but bindtextdomain() doesn't */
#ifdef WIN32
int save_errno = GetLastError();
#else
--- 1212,1225
#ifdef ENABLE_NLS
! static void
! libpq_bindomain()
{
static bool already_bound = false;
if (!already_bound)
{
! /* bindtextdomain() does not preserve errno */
#ifdef WIN32
int save_errno = GetLastError();
#else
*** libpq_gettext(const char *msgid)
*** 1237,1244
--- 1239,1258
errno = save_errno;
#endif
}
+ }
+ char *
+ libpq_gettext(const char *msgid)
+ {
+ libpq_bindomain
Re: [HACKERS] implement subject alternative names support for SSL connections
On Thu, Sep 4, 2014 at 10:23 AM, Heikki Linnakangas
hlinnakan...@vmware.com wrote:
Hmm. Perhaps we should use X509_NAME_get_index_by_NID + X509_NAME_get_entry
instead of X509_NAME_get_text_by_NID. You could then pass the ASN1_STRING
object to the certificate_name_entry_validate_match() function, and have it
do the ASN1_STRING_length() and ASN1_STRING_data() calls too.
...
I think we should:
1. Check if there's a common name, and if so, print that
2. Check if there is exactly one SAN, and if so, print that
3. Just print an error without mentioning names.
There's a lot of value in printing the name if possible, so I'd really like
to keep that. But I agree that printing all the names if there are several
would get complicated and the error message could become very long. Yeah,
the error message might need to be different for cases 1 and 2. Or maybe
phrase it server certificate's name \%s\ does not match host name
\%s\, which would be reasonable for both 1. and 2.
Thank you, I've implemented both suggestions in the attached new
version of the patch.
On the error message, I've decided to show either a single name, or
the first examined name and the number of other names present in the
certificate, i.e:
psql: server name example.com and 2 other names from server SSL certificate
do not match host name example-foo.com
The error does not state whether the names comes from the CN or from
the SAN section.
This version also checks for the availability of the subject name, it
looks like RFC 5280 does not require it at all.
4.1.2.6. Subject
The subject field identifies the entity associated with the public
key stored in the subject public key field. The subject name MAY be
carried in the subject field and/or the subjectAltName extension.
The pattern of allocating the name in the subroutine and then
reporting it (and releasing when necessary) in the main function is a
little bit ugly, but it looks to me the least ugly of anything else I
could come up (i.e. extracting those names at the time the error
message is shown).
Regards,
--
Alexey Klyukin
diff --git a/src/interfaces/libpq/fe-secure-openssl.c
b/src/interfaces/libpq/fe-secure-openssl.c
new file mode 100644
index f950fc3..a4ad305
*** a/src/interfaces/libpq/fe-secure-openssl.c
--- b/src/interfaces/libpq/fe-secure-openssl.c
***
*** 60,68
--- 60,73
#ifdef USE_SSL_ENGINE
#include openssl/engine.h
#endif
+ #include openssl/x509v3.h
static bool verify_peer_name_matches_certificate(PGconn *);
static intverify_cb(int ok, X509_STORE_CTX *ctx);
+ static intcertificate_name_entry_validate_match(PGconn *conn,
+
ASN1_STRING *name,
+
bool *match,
+
char **store_name);
static void destroy_ssl_system(void);
static intinitialize_SSL(PGconn *conn);
static PostgresPollingStatusType open_client_SSL(PGconn *);
*** wildcard_certificate_match(const char *p
*** 473,540
/*
! *Verify that common name resolves to peer.
*/
! static bool
! verify_peer_name_matches_certificate(PGconn *conn)
{
! char *peer_cn;
! int r;
! int len;
! boolresult;
!
! /*
!* If told not to verify the peer name, don't do it. Return true
!* indicating that the verification was successful.
!*/
! if (strcmp(conn-sslmode, verify-full) != 0)
! return true;
! /*
!* Extract the common name from the certificate.
!*
!* XXX: Should support alternate names here
!*/
! /* First find out the name's length and allocate a buffer for it. */
! len = X509_NAME_get_text_by_NID(X509_get_subject_name(conn-peer),
!
NID_commonName, NULL, 0);
! if (len == -1)
! {
! printfPQExpBuffer(conn-errorMessage,
! libpq_gettext(could not get
server common name from server certificate\n));
! return false;
! }
! peer_cn = malloc(len + 1);
! if (peer_cn == NULL)
{
printfPQExpBuffer(conn-errorMessage,
! libpq_gettext(out of
memory\n));
! return false;
}
! r = X509_NAME_get_text_by_NID(X509_get_subject_name(conn-peer),
!
NID_commonName, peer_cn, len + 1);
! if (r != len)
{
- /* Got different length than on the first call. Shouldn't
happen. */
printfPQExpBuffer(conn-errorMessage
Re: [HACKERS] implement subject alternative names support for SSL connections
On Wed, Sep 3, 2014 at 11:50 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: * It's ugly that the caller does the malloc and memcpy, and the certificate_name_entry_validate_match function then modifies its name argument. Move the malloc+memcpy inside the function. For the case of CN the caller has to do the malloc anyway, since X509_NAME_get_text_by_NID expects the already allocated buffer. This means that 'movable' malloc + memcpy occurs only once, and while it would still make sense to move it into the function, it would also mean we would do an unnecessary malloc for the case of CN. * The error message in certificate_name_entry_validate_match says SSL certificate's common name contains embedded null even though it's also used for SANs. Will fix, thank you. The tricky part is the error message if no match was found: initially, it only listed a single common name, but now tracking all DNS names just for the sake of the error message makes the code more bloated, so I'm wondering if simply stating that there was no match, as implemented in the attached patch, would be good enough? Hmm. It would still be nice to say something about the certificate that was received. How about: server certificate with common name %s does not match host name %s We cannot guarantee at this point that the common name is present in the certificate. And if it is not, which name should we pick instead? One way to solve this is to take the first non-NULL name, but it if there is no common name, but then the wording of the error message would depend on the availability of CN. Another is to show all available names, but I do not like collecting them just for the sake of displaying in the error message. And last one is to just show the error without mentioning names, that's what I've chosen to be the most consistent. Regards, -- Alexey Klyukin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Enable WAL archiving even in standby
On Wed, Aug 13, 2014 at 12:42 PM, Fujii Masao masao.fu...@gmail.com wrote:
Hi,
I'd propose the attached WIP patch which allows us to enable WAL archiving
even in standby.
...
I think that this feature is useful for the case, e.g., where large database
needs to be replicated between remote servers. Imagine the situation where
the replicated database gets corrupted completely in the remote standby.
How should we address this problematic situation and restart the standby?
One approach is to take a fresh backup from the master and restore it onto
the standby. But since the database is large and there is long distance
between two servers, this approach might take a surprisingly long time.
Another approach is to restore the backup which was taken from the standby
before. But most of many WAL files which the backup needs might exist only
in the master (because WAL archiving cannot be enabled in the standby) and
they need to be transfered from the master to the standby via long-distance
network. So I think that this approach also would take a fairly long time.
To shorten that time, you may think that archive_command in the master can
be set so that it transfers WAL files from the master to the standby's
archival storage. I agree that this setting can accelerate the database
restore
process. But this causes every WAL files to be transfered between remote
servers twice (one is by streaming replication, another is by
archive_command),
and which is a waste of network bandwidth.
Well, in theory one can also use pg_receivexlog to get the WAL files
from master,
and then run them through the PITR on the slave without the
archive_cleanup command.
I'm not sure you can do the same if the source of the WAL files is a
cascading slave,
but I see no reasons why not to.
However, I find the patch useful, since it allows accomplishing
things in a much more
straightforward way.
Back to the patch. If archive_mode is set to always, archive_command is
always used to archive WAL files even during recovery. Do we need to separate
the command into two for master and standby, respectively? We can add
something like standby_archive_command parameter which is used to archive
only WAL files walreceiver writes. The other WAL files are archived by
archive_command. I'm not sure if it's really worth separating the command
that way. Is there any use case?
I don't see a good use case for doing things only on standby, but I can imagine
that some different archiving methods might be used depending on the role of
the archiver: on master, one may do, for instance, additional copy to the NFS
partition. Does it make sense to expose the server role ('is_master') via an
additional variable available to the recovery_command (i.e. %m)?
The patch doesn't allow us to enable WAL archiving *only* during recovery.
Should we support yet another archive_mode like standby which allows
the archiver to be running only during recovery, but makes it end just after
the server is promoted to master? I'm not sure if there is really use case for
that.
I do not see much use for this as well.
I've not included the update of document in the patch yet. If we agree to
support this feature, I will do the remaining work.
I think it is useful, and I gave this patch a spin by, essentially, creating a
cascaded archive-only slave. I made a base backup from master, then
ran the standby from this base backup with archive_mode = 'always' and
archive_command copying files to the archive_location, then created another
base backup out of it (without including WAL files into the backup) and pointed
the recovery command of the final slave into the archive created by
the intermediate one.
Recovery worked, as well as the promotion of the intermediate slave to
the master,
the final slave just caught up with the timeline changes (with
recovery_timeline set to
'latest') and went on with the recovery.
One thing I've noticed is that pg_basebackup copies the postgresql.conf from the
standby verbatim, including the archive_mode, which means that if one runs
the cascaded replica without changing the archive_mode, that replica
will try to archive
the WAL, and if both the source and the replica are running on the same machine
(or attached to NFS using the same mount points) even the destination
for archiving
will be the same. Should not be a big problem if one follows the
recommendation of not
overwriting the files that already exist at the destination, but it
would be nice to reset the
archive_mode flag to off.
I do not know much about the WAL-related code, but one thing that I
found strange
in the patch is a separate file xlogarchive.h instead of putting
stuff into xlog.h?
Does not make much sense for a single enum, are you planning to put
more things there?
There was a single hunk when applying this against the latest master:
Hunk #4 succeeded at 4789 (offset -1 lines).
--
Regards,
Alexey Klyukin
--
Sent via pgsql-hackers mailing list
Re: [HACKERS] implement subject alternative names support for SSL connections
On Fri, Aug 29, 2014 at 11:22 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 08/28/2014 07:28 PM, Alexey Klyukin wrote: On Mon, Aug 25, 2014 at 12:02 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 08/24/2014 03:11 PM, Alexey Klyukin wrote: On Wed, Aug 20, 2014 at 11:53 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: The patch doesn't seem to support wildcards in alternative names. Is that on purpose? Not really, we just did not have any use case for them, but it seems that RFC 5280 does not disallow them: Finally, the semantics of subject alternative names that include wildcard characters (e.g., as a placeholder for a set of names) are not addressed by this specification. Applications with specific requirements MAY use such names, but they must define the semantics. I've added support for them in the next iteration of the patch attached to this email. Hmm. So wildcards MAY be supported, but should we? I think we should follow the example of common browsers here, or OpenSSL or other SSL libraries; what do they do? Yes, they seem to be supported. The function you've mentioned above (X509_check_host) specifically mentions wildcards in SANs at https://www.openssl.org/docs/crypto/X509_check_host.html: 'X509_check_host() checks if the certificate Subject Alternative Name (SAN) or Subject CommonName (CN) matches the specified host name, which must be encoded in the preferred name syntax described in section 3.5 of RFC 1034. By default, wildcards are supported and they match only in the left-most label; but they may match part of that label with an explicit prefix or suffix. For example, by default, the host name ``www.example.com'' would match a certificate with a SAN or CN value of ``*.example.com'', ``w*.example.com'' or ``*w.example.com''.' RFC 6125 section 6.4.4 Checking of Common Names says: As noted, a client MUST NOT seek a match for a reference identifier of CN-ID if the presented identifiers include a DNS-ID, SRV-ID, URI-ID, or any application-specific identifier types supported by the client. So, to conform to that we shouldn't check the Common Name at all, if an alternative subject field is present. While the RFC indeed says so, the OpenSSL implementation includes X509_CHECK_FLAG_ALWAYS_CHECK_SUBJECT flag (which, as far as I can see, is set to 1 by default), which makes it check for the CN even if DNS names in SAN are present. I'm not sure what is the reason behind section 6.4.4, and I think it makes sense to check CN as well, since it does not lead to the case of false matches. Yeah, I think a certificate without CN should be supported. See also RFC 6125, section 4.1. Rules [for issuers of certificates]: 5. Even though many deployed clients still check for the CN-ID within the certificate subject field, certification authorities are encouraged to migrate away from issuing certificates that represent the server's fully qualified DNS domain name in a CN-ID. Therefore, the certificate SHOULD NOT include a CN-ID unless the certification authority issues the certificate in accordance with a specification that reuses this one and that explicitly encourages continued support for the CN-ID identifier type in the context of a given application technology. Certificates without a CN-ID are probably rare today, but they might start to appear in the future. Ok, I will change a patch to add support for this clause. BTW, should we also support alternative subject names in the server, in client certificates? And if there are multiple names, which one takes effect? Perhaps better to leave that for a separate patch. Good question. The RFC 5280 section 4.2.1.6 does not restrict the certificates to be used only server-side, so the same rules should be applicable to the client certs. I'm wondering if there is an equivalent of RFC 6125 for the clients? PostgreSQL, however, checks different things on the backends and the clients, the formers are checked against the DNS name, while on the clients only the user name is checked. For this, I think, a SAN section with some custom identity type should be issued (the 4.2.1.6 does not list user names as a pre-defined identify type). Note that PostgreSQL can already support clients with multiple names via the user maps, so support for SAN is not that urgent there. On the other hand, should we, in addition to verification of client user names, verify the client names supplied during connections against the DNS entries in their certificates? Are there use cases for this? I do agree this should be the subject of a separate patch. Regards, Alexey -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] implement subject alternative names support for SSL connections
On Mon, Sep 1, 2014 at 10:39 AM, Alexey Klyukin al...@hintbits.com wrote:
On Fri, Aug 29, 2014 at 11:22 AM, Heikki Linnakangas
hlinnakan...@vmware.com wrote:
Yeah, I think a certificate without CN should be supported. See also RFC
6125, section 4.1. Rules [for issuers of certificates]:
5. Even though many deployed clients still check for the CN-ID
within the certificate subject field, certification authorities
are encouraged to migrate away from issuing certificates that
represent the server's fully qualified DNS domain name in a
CN-ID. Therefore, the certificate SHOULD NOT include a CN-ID
unless the certification authority issues the certificate in
accordance with a specification that reuses this one and that
explicitly encourages continued support for the CN-ID identifier
type in the context of a given application technology.
Certificates without a CN-ID are probably rare today, but they might start
to appear in the future.
Ok, I will change a patch to add support for this clause.
Attached is a new version. I've changed the logic to check for the SAN
names first, and only check the common name if there is no match. The
error when the common name is missing is only shown if SAN section
does not contain any DNS names as well. The tricky part is the error
message if no match was found: initially, it only listed a single
common name, but now tracking all DNS names just for the sake of the
error message makes the code more bloated, so I'm wondering if simply
stating that there was no match, as implemented in the attached patch,
would be good enough?
--
Regards,
Alexey Klyukin
diff --git a/src/interfaces/libpq/fe-secure-openssl.c
b/src/interfaces/libpq/fe-secure-openssl.c
new file mode 100644
index f950fc3..a4e3fc6
*** a/src/interfaces/libpq/fe-secure-openssl.c
--- b/src/interfaces/libpq/fe-secure-openssl.c
***
*** 60,68
--- 60,73
#ifdef USE_SSL_ENGINE
#include openssl/engine.h
#endif
+ #include openssl/x509v3.h
static bool verify_peer_name_matches_certificate(PGconn *);
static intverify_cb(int ok, X509_STORE_CTX *ctx);
+ static intcertificate_name_entry_validate_match(PGconn *conn,
+
char *name,
+
unsigned int len,
+
bool *match);
static void destroy_ssl_system(void);
static intinitialize_SSL(PGconn *conn);
static PostgresPollingStatusType open_client_SSL(PGconn *);
*** wildcard_certificate_match(const char *p
*** 471,487
return 1;
}
/*
! *Verify that common name resolves to peer.
*/
static bool
verify_peer_name_matches_certificate(PGconn *conn)
{
! char *peer_cn;
! int r;
! int len;
! boolresult;
/*
* If told not to verify the peer name, don't do it. Return true
--- 476,525
return 1;
}
+ /*
+ * Validate a single certificate name entry and match it against the pghost.
+ * Returns 0 if the certificate name is invalid (contains embedded NULLs), 1
otherwise.
+ */
+ static int
+ certificate_name_entry_validate_match(PGconn *conn, char *name, unsigned int
len, bool *match)
+ {
+ /* There is no guarantee the string returned from the certificate is
NULL-terminated */
+ name[len] = '\0';
+ *match = false;
+ /*
+* Reject embedded NULLs in certificate common or alternative name to
prevent attacks
+* like CVE-2009-4034.
+*/
+ if (len != strlen(name))
+ {
+ printfPQExpBuffer(conn-errorMessage,
+ libpq_gettext(SSL certificate's
common name contains embedded null\n));
+ return 0;
+ }
+ if (pg_strcasecmp(name, conn-pghost) == 0)
+ /* Exact name match */
+ *match = true;
+ else if (wildcard_certificate_match(name, conn-pghost))
+ /* Matched wildcard certificate */
+ *match = true;
+ else
+ *match = false;
+ return 1;
+ }
+
/*
! *Verify that common name or any of the alternative DNS names resolves to
peer.
! *Names in Subject Alternative Names and Common Name if present are
considered.
*/
static bool
verify_peer_name_matches_certificate(PGconn *conn)
{
! inti;
! intsan_len;
! bool result;
! bool san_has_dns_names;
! STACK_OF(GENERAL_NAME) *peer_san;
/*
* If told not to verify the peer name, don't do it. Return true
Re: [HACKERS] implement subject alternative names support for SSL connections
On Mon, Aug 25, 2014 at 12:02 PM, Heikki Linnakangas
hlinnakan...@vmware.com wrote:
On 08/24/2014 03:11 PM, Alexey Klyukin wrote:
On Wed, Aug 20, 2014 at 11:53 AM, Heikki Linnakangas
hlinnakan...@vmware.com wrote:
The patch doesn't seem to support wildcards in alternative names. Is
that on purpose?
Not really, we just did not have any use case for them, but it seems that
RFC 5280 does not disallow them:
Finally, the semantics of subject alternative names that include
wildcard characters (e.g., as a placeholder for a set of names) are
not addressed by this specification. Applications with specific
requirements MAY use such names, but they must define the semantics.
I've added support for them in the next iteration of the patch attached to
this email.
It would be good to add a little helper function that does the NULL-check,
straight comparison, and wildcard check, for a single name. And then use
that for the Common Name and all the Alternatives. That'll ensure that all
the same rules apply whether the name is the Common Name or an Alternative
(assuming that the rules are supposed to be the same; I don't know if
that's true).
Thanks, common code has been moved into a separate new function.
Another question is how should we treat the certificates with no CN and
non-empty SAN?
Current code just bails out right after finding no CN section present , but
the RFC (5280) says:
Further, if the only subject identity included in the certificate is
an alternative name form (e.g., an electronic mail address), then the
subject distinguished name MUST be empty (an empty sequence), and the
subjectAltName extension MUST be present.
which to me sounds like the possibility of coming across such certificates
in the wild, although I personally see little use in them.
Regards,
--
Alexey Klyukin
diff --git a/src/interfaces/libpq/fe-secure-openssl.c
b/src/interfaces/libpq/fe-secure-openssl.c
new file mode 100644
index f950fc3..394a66f
*** a/src/interfaces/libpq/fe-secure-openssl.c
--- b/src/interfaces/libpq/fe-secure-openssl.c
***
*** 60,68
--- 60,73
#ifdef USE_SSL_ENGINE
#include openssl/engine.h
#endif
+ #include openssl/x509v3.h
static bool verify_peer_name_matches_certificate(PGconn *);
static intverify_cb(int ok, X509_STORE_CTX *ctx);
+ static intcertificate_name_entry_validate_match(PGconn *conn,
+
char *name,
+
unsigned int len,
+
bool *match);
static void destroy_ssl_system(void);
static intinitialize_SSL(PGconn *conn);
static PostgresPollingStatusType open_client_SSL(PGconn *);
*** wildcard_certificate_match(const char *p
*** 471,479
return 1;
}
/*
! *Verify that common name resolves to peer.
*/
static bool
verify_peer_name_matches_certificate(PGconn *conn)
--- 476,515
return 1;
}
+ /*
+ * Validate a single certificate name entry and match it against the pghost.
+ * Returns 0 if the certificate name is invalid (contains embedded NULLs), 1
otherwise.
+ */
+ static int
+ certificate_name_entry_validate_match(PGconn *conn, char *name, unsigned int
len, bool *match)
+ {
+ /* There is no guarantee the string returned from the certificate is
NULL-terminated */
+ name[len] = '\0';
+ *match = false;
+ /*
+* Reject embedded NULLs in certificate common or alternative name to
prevent attacks
+* like CVE-2009-4034.
+*/
+ if (len != strlen(name))
+ {
+ printfPQExpBuffer(conn-errorMessage,
+ libpq_gettext(SSL certificate's
common name contains embedded null\n));
+ return 0;
+ }
+ if (pg_strcasecmp(name, conn-pghost) == 0)
+ /* Exact name match */
+ *match = true;
+ else if (wildcard_certificate_match(name, conn-pghost))
+ /* Matched wildcard certificate */
+ *match = true;
+ else
+ *match = false;
+ return 1;
+ }
+
/*
! *Verify that common name or any of the alternative dNSNames resolves to
peer.
*/
static bool
verify_peer_name_matches_certificate(PGconn *conn)
*** verify_peer_name_matches_certificate(PGc
*** 492,499
/*
* Extract the common name from the certificate.
-*
-* XXX: Should support alternate names here
*/
/* First find out the name's length and allocate a buffer for it. */
len = X509_NAME_get_text_by_NID(X509_get_subject_name(conn-peer),
--- 528,533
*** verify_peer_name_matches_certificate(PGc
*** 522,540
Re: [HACKERS] implement subject alternative names support for SSL connections
On Mon, Aug 25, 2014 at 12:33 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 08/25/2014 01:07 PM, Andres Freund wrote: On 2014-08-25 13:02:50 +0300, Heikki Linnakangas wrote: But actually, I wonder if we should delegate the whole hostname matching to OpenSSL? There's a function called X509_check_host for that, although it's new in OpenSSL 1.1.0 so we'd need to add a configure test for that and keep the current code to handle older versions. Given that we're about to add support for other SSL implementations I'm not sure that that's a good idea. IIRC there exist quite a bit of different interpretations about what denotes a valid cert between the libraries. As long as just this patch is concerned, I agree it's easier to just implement it ourselves, but if we want to start implementing more complicated rules, then I'd rather not get into that business at all, and let the SSL library vendor deal with the bugs and CVEs. Sounds reasonable. I guess we'll go ahead with this patch for now, but keep this in mind if someone wants to complicate the rules further in the future. +1 -- Regards, Alexey Klyukin
[HACKERS] re-reading SSL certificates during server reload
Greetings, Is there a strong reason to disallow reloading server key and cert files during the PostgreSQL reload? Basically, once you run multiple databases in a cluster and use different DNS names to connect to different databases (in order for those databases to be moved somewhere without changing the client code), and enable SSL certificate checking, the problem becomes evident: in order to add a new database to the existing cluster you have to add its name to the SSL certificate for the server, and in order for this changes to come into effect you have to restart the server. In the documentation for server cert and key file there is a notice that this parameter can only be reloaded during the server start. It seems that the only place the backend certificates are loaded is inside the secure_initialize, which, in order, calls initialize_SSL(). From my point of view, I see nothing preventing separation of the certificate reload code and SSL library initialization and calling the former during the server reload. It might happen that with the new certificate file that some of the existing connections will be unable to reconnect, or, if the certificate is invalid, the server will be unable to restart, but this are the sort of problems that also happen with reload of pg_hba.conf as well, so these alone does not sound like a significant showstopper. -- Regards, Alexey Klyukin
Re: [HACKERS] implement subject alternative names support for SSL connections
On Wed, Aug 20, 2014 at 11:53 AM, Heikki Linnakangas
hlinnakan...@vmware.com wrote:
On 07/25/2014 07:10 PM, Alexey Klyukin wrote:
Greetings,
I'd like to propose a patch for checking subject alternative names entry
in
the SSL certificate for DNS names during SSL authentication.
Thanks! I just ran into this missing feature last week, while working on
my SSL test suite. So +1 for having the feature.
This patch needs to be rebased over current master branch, thanks to my
refactoring that moved all OpenSSL-specific stuff to be-secure-openssl.c.
The patch is rebased against fe-secure-openssl.c (that's where
verify_peer_name_matches_certificate appeared in the master branch), I've
changed the condition in the for loop to be less confusing (thanks to
comments from Magnus and Tom), making an explicit break once a match is
detected.
Note that It generates a lot of OpenSSL related warnings on my system (66
total) with clang, complaining about
$X is deprecated: first deprecated in OS X 10.7
[-Wdeprecated-declarations], but it does so for most other SSL functions,
so I don't think it's a problem introduced by this patch.
Sincerely,
Alexey.
diff --git a/src/interfaces/libpq/fe-secure-openssl.c
b/src/interfaces/libpq/fe-secure-openssl.c
new file mode 100644
index f950fc3..b4f6bc9
*** a/src/interfaces/libpq/fe-secure-openssl.c
--- b/src/interfaces/libpq/fe-secure-openssl.c
***
*** 60,65
--- 60,66
#ifdef USE_SSL_ENGINE
#include openssl/engine.h
#endif
+ #include openssl/x509v3.h
static bool verify_peer_name_matches_certificate(PGconn *);
static intverify_cb(int ok, X509_STORE_CTX *ctx);
*** wildcard_certificate_match(const char *p
*** 473,479
/*
! *Verify that common name resolves to peer.
*/
static bool
verify_peer_name_matches_certificate(PGconn *conn)
--- 474,480
/*
! *Verify that common name or any of the alternative dNSNames resolves to
peer.
*/
static bool
verify_peer_name_matches_certificate(PGconn *conn)
*** verify_peer_name_matches_certificate(PGc
*** 492,499
/*
* Extract the common name from the certificate.
-*
-* XXX: Should support alternate names here
*/
/* First find out the name's length and allocate a buffer for it. */
len = X509_NAME_get_text_by_NID(X509_get_subject_name(conn-peer),
--- 493,498
*** verify_peer_name_matches_certificate(PGc
*** 556,565
result = true;
else
{
printfPQExpBuffer(conn-errorMessage,
! libpq_gettext(server
common name \%s\ does not match host name \%s\\n),
peer_cn,
conn-pghost);
- result = false;
}
}
--- 555,627
result = true;
else
{
+ inti;
+ intalt_names_total;
+ STACK_OF(GENERAL_NAME) *alt_names;
+
+ /* Get the list and the total number of alternative
names. */
+ alt_names = (STACK_OF(GENERAL_NAME) *)
X509_get_ext_d2i(conn-peer, NID_subject_alt_name, NULL, NULL);
+ if (alt_names != NULL)
+ alt_names_total =
sk_GENERAL_NAME_num(alt_names);
+ else
+ alt_names_total = 0;
+
+ result = false;
+
+ /*
+* Compare the alternative names dnsNames identifies
against
+* the originally given hostname.
+*/
+ for (i = 0; i alt_names_total; i++)
+ {
+ const GENERAL_NAME *name =
sk_GENERAL_NAME_value(alt_names, i);
+ if (name-type == GEN_DNS)
+ {
+ intreported_len;
+ intactual_len;
+ char *dns_namedata,
+ *dns_name;
+
+ reported_len =
ASN1_STRING_length(name-d.dNSName);
+ /* GEN_DNS can be only IA5String,
equivalent to US ASCII */
+ dns_namedata = (char *)
ASN1_STRING_data(name-d.dNSName);
+
+ dns_name = malloc(reported_len + 1);
+ memcpy(dns_name, dns_namedata,
reported_len);
+ dns_name[reported_len] = '\0
[HACKERS] implement subject alternative names support for SSL connections
Greetings,
I'd like to propose a patch for checking subject alternative names entry in
the SSL certificate for DNS names during SSL authentication.
When the client PGSSLMODE is set to verify-full, the common name (CN) of
the PostgreSQL server in the certificate is matched against the actual
host name supplied by the client. A successful connection is only
established when those names match.
If a failover schema with a floating IP is used, a single DNS name may
point to different hosts after failover. In order to maintain the correct
pair of (client connection FQDN, FQDN in the certificate) the certificate
from the master should also be transferred to the slave. Unfortunately, it
makes failover more complicated, since the server restart is required when
the new certificate is installed.
The other option is to issue a single certificate covering all the hosts
that may participate in the failover.
So far the only way to cover more than one server name with a single
certificate is to use wildcards in the server common name, i.e.
*.db.example com. Unfortunately, this schema only works for names like
master.db.example.com, slave.db.example.com, but not for the example.com
and db-master.example.com and db-slave.example.com or other more complex
naming schemas.
The other way to issue a single certificate for multiple names is to set
the subject alternative names, described in the RFC 3280 4.2.1.7. SAN
allows binding multiple identities to the certificate, including DNS names.
For the names above the SAN with look like this:
X509v3 Subject Alternative Name:
DNS:db-master.example.com, DNS:db-slave.example.com.
At the moment PostgreSQL doesn't support SANs, but should, according to the
following comment in the code in verify_peer_name_matches_certificate:
/*
* Extract the common name from the certificate.
*
* XXX: Should support alternate names here
*/
The attached patch adds support for checking the client supplied names
against the dNSName-s in SAN, making it easier to set secure HA
environments using PostgreSQL. If SAN section is present, the DNS names
from that section are checked against the peer name in addition to checking
the common name (CN) from the certificate. The match is successful if at
least one of those names match the name supplied by the peer.
I gave it a spin and it works in our environment (Linux database servers,
Linux and Mac clients). I don't have either Windows or old versions of
OpenSSL, it's not tested against those systems.
I'd appreciate your feedback.
Sincerely,
--
Alexey Klyukin
diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
new file mode 100644
index 9ba3567..64eab27
*** a/src/interfaces/libpq/fe-secure.c
--- b/src/interfaces/libpq/fe-secure.c
***
*** 64,69
--- 64,70
#ifdef USE_SSL_ENGINE
#include openssl/engine.h
#endif
+ #include openssl/x509v3.h
#ifndef WIN32
*** wildcard_certificate_match(const char *p
*** 754,760
/*
! *Verify that common name resolves to peer.
*/
static bool
verify_peer_name_matches_certificate(PGconn *conn)
--- 755,761
/*
! *Verify that common name or any of the alternative dNSNames resolves to
peer.
*/
static bool
verify_peer_name_matches_certificate(PGconn *conn)
*** verify_peer_name_matches_certificate(PGc
*** 773,780
/*
* Extract the common name from the certificate.
-*
-* XXX: Should support alternate names here
*/
/* First find out the name's length and allocate a buffer for it. */
len = X509_NAME_get_text_by_NID(X509_get_subject_name(conn-peer),
--- 774,779
*** verify_peer_name_matches_certificate(PGc
*** 837,846
result = true;
else
{
printfPQExpBuffer(conn-errorMessage,
! libpq_gettext(server
common name \%s\ does not match host name \%s\\n),
peer_cn,
conn-pghost);
- result = false;
}
}
--- 836,904
result = true;
else
{
+ inti;
+ intalt_names_total;
+ STACK_OF(GENERAL_NAME) *alt_names;
+
+ /* Get the list and the total number of alternative
names. */
+ alt_names = (STACK_OF(GENERAL_NAME) *)
X509_get_ext_d2i(conn-peer, NID_subject_alt_name, NULL, NULL);
+ if (alt_names != NULL)
+ alt_names_total =
sk_GENERAL_NAME_num(alt_names);
+ else
+ alt_names_total = 0;
+
+ result = false
Re: [HACKERS] implement subject alternative names support for SSL connections
On Fri, Jul 25, 2014 at 6:34 PM, Magnus Hagander mag...@hagander.net wrote: I just took a very quick look at the code, and just noticed one thing: Why keep looping once you've found a match? When you set result=true you should break; from the loop I think. Not necessarily for performance, but there might be something about a different extension we can't parse for example, no need to fail in that case. The for loop header is for (i = 0; i alt_names_total !result; i++), so the loop should terminate right when the result becomes true, which happens if the pg_strcasecmp finds a match between the given dNSName and the name supplied by the client. Please add it to the next CF - this was just a very quick review, and it needs a proper one along with openssl version testing :) Done. Sincerely, -- Alexey Klyukin
Re: [HACKERS] Could not open file pg_multixact/offsets/ ERROR on 9.3.4
On Wed, Jun 4, 2014 at 5:10 PM, Andres Freund and...@2ndquadrant.com wrote: Looks like you're hitting the issue described in http://archives.postgresql.org/message-id/20140530121631.GE25431%40alap3.anarazel.de Aha, so the problem looks like this: - all multixact contents was purged (9.2 to 9.3 incompatibility) - the only offset was the file created by initidb during bootstrap - truncation code in vacuum assumed the page related to multixact it tried to truncate actually exists - the assumption was based on the fact that there exists a page earlier than the one it tried to truncate - due to the wrong assumption, there was an attempt to truncate a non-existing page, resulting in the error. Now that makes sense, thank you very much. Vacuum runs w/o issues once I removed that file. I think it should be safe to recommend removing the file for everyone with the same problem. The 2 cases where file is actually used are: - when the are no more then 2 pages of offsets ( and 0001) since the server creation - when the multixacts wrapped around and is not the first page. I think in both cases there will be no gaps in segments in offsets, so the problem won't be there in the first place. Cheers, -- Alexey Klyukin
[HACKERS] Could not open file pg_multixact/offsets/ ERROR on 9.3.4
Greetings, I've recently discovered a peculiar problem on one of our big databases (more than 1TB). The database has been upgraded from 9.2 to 9.3.4 (using hardlinks to speedup the process) on April 7th around 22:00 local time. When doing vacuum on any table, the system fails with the following error: ERROR: could not access status of transaction 2035987419 DETAIL: Could not open file pg_multixact/offsets/795A: No such file or directory. The erorr doesn't depend on the table being vacuumed, or even database, i.e: postgres=# create database x; CREATE DATABASE postgres=# \c x You are now connected to database x as user postgres. x=# create table test(); CREATE TABLE x=# vacuum test; ERROR: could not access status of transaction 2035987419 DETAIL: Could not open file pg_multixact/offsets/795A: No such file or directory. The content of pg_multixact/offsets is: pg_multixact$ ls -lR ./members: -rw--- 1 postgres postgres 8192 Apr 16 18:20 ./offsets: -rw--- 1 postgres postgres 8192 Apr 7 22:51 -rw--- 1 postgres postgres 262144 Apr 16 18:20 79A6 the select mutlixact from pg_database gives me: postgres=# select oid, datminmxid from pg_database; oid | datminmxid ---+ 12078 | 2040987419 12073 | 2040987419 16414 | 2035987419 16415 | 2040987418 1 | 2040987419 (5 rows) and the 2035987419 = 0x795AB3DB belongs to 795A segment. The file just contains a single page of all zeroes. Neither the 9.3.4 replica of this database, nor the original 9.2 cluster data directory contain this file. I guess what happens is that at the end of vacuum PostgreSQL checks the status of the earliest available multixact (perhaps in order to truncate it), and fails because the file has been already removed. I also have a replica of this database, (which, as I said, doesn't have the segment). When I shut down this replica, copied the data directory, promoted the copied directory (effectively making a copy of the master) and issued vacuum I observed no errors. However, when I manually created this multixact/offset/ with dd (populating 8192 bytes with 0), exactly the same problem re-appeared as on the master. I'm tempted to just remove the file from master and restart the database, since it's effectively impossible to run vacuum now, but I'd like to understand what's happening first. Below is the result of pg_filedump for the master: data$ pg_controldata . pg_control version number:937 Catalog version number: 201306121 Database system identifier: 5999656352803688379 Database cluster state: in production pg_control last modified: Wed 04 Jun 2014 04:49:45 PM CEST Latest checkpoint location: 25D0/D38A8DF0 Prior checkpoint location:25D0/BFA7B068 Latest checkpoint's REDO location:25D0/C27A54A0 Latest checkpoint's REDO WAL file:000125D000C2 Latest checkpoint's TimeLineID: 1 Latest checkpoint's PrevTimeLineID: 1 Latest checkpoint's full_page_writes: on Latest checkpoint's NextXID: 0/1655213806 Latest checkpoint's NextOID: 16795470 Latest checkpoint's NextMultiXactId: 2040987419 Latest checkpoint's NextMultiOffset: 3 Latest checkpoint's oldestXID:1038291920 Latest checkpoint's oldestXID's DB: 16415 Latest checkpoint's oldestActiveXID: 1655189767 Latest checkpoint's oldestMultiXid: 2040987417 Latest checkpoint's oldestMulti's DB: 0 Time of latest checkpoint:Wed 04 Jun 2014 04:45:45 PM CEST Fake LSN counter for unlogged rels: 0/1 Minimum recovery ending location: 0/0 Min recovery ending loc's timeline: 0 Backup start location:0/0 Backup end location: 0/0 End-of-backup record required:no Current wal_level setting:hot_standby Current max_connections setting: 500 Current max_prepared_xacts setting: 0 Current max_locks_per_xact setting: 64 Maximum data alignment: 8 Database block size: 8192 Blocks per segment of large relation: 131072 WAL block size: 8192 Bytes per WAL segment:16777216 Maximum length of identifiers:64 Maximum columns in an index: 32 Maximum size of a TOAST chunk:1996 Date/time type storage: 64-bit integers Float4 argument passing: by value Float8 argument passing: by value Data page checksum version: 0 There were no wraparound-related messages in the logs of either old or new cluster, nor we observed any other errors except this one (we originally discovered it in the server logs, likely because the autovacuum was also failing). Any hints on what's going on (and whether the data corruption is a possibility)? Cheers, -- Alexey Klyukin
Re: [HACKERS] Wildcard usage enhancements in .pgpass
Hi Martijn, On Sun, Nov 17, 2013 at 7:56 PM, Martijn van Oosterhout klep...@svana.orgwrote: On Sat, Nov 16, 2013 at 09:26:33PM +0100, Alexey Klyukin wrote: Hi, Attached is the patch that improves usage of '*' wildcard in .pgpass, particularly in the host part. The use case is below. Looks interesting, though I wonder if you could use fnmatch(3) here. Or woud that match more than you expect? For example, it would allow 'foo*bar' to match 'foo.bar' which your code doesn't. fnmatch(3) looks like a good deal and I'd certainly consider it if we go the road of matching regular expressions, although for simpler use cases it's an overkill, since it forces us to do an extra pass over the string to be matched and introduces some performance penalties of using a regexp matching engine. -- Regards, Alexey Klyukin
[HACKERS] Wildcard usage enhancements in .pgpass
Hi,
Attached is the patch that improves usage of '*' wildcard in .pgpass,
particularly in the host part. The use case is below.
I work with multiple environments (like staging, production, performance
and so on), each one containing tens of different database clusters, each
cluster has its own subdomain in DNS, i.e. foo.test.db, bar.test.db and so
on. Each user has the same credentials on every database of a single
domain, i.e. .test.db databases, but different ones in other domains. For
those databases, each line in pgpass currently corresponds to a single
database, i.e.
foo1.test.db:5432:postgres:postgres:keep!tester
foo2.test.db:5432:postgres:postgres:keep!tsecret
...
foo99.test.db:5432:postgres:postgres:keep!tsecret
bar1.another.db.:5432:postgres:postgres:trustno1
bar2.another.db.:5432:postgres:postgres:trustno1
...
The problem I'm having is that there are just too many lines like those
(tens or even hundreds) and the new databases are added very frequently,
making it hard to keep .pgpass up-to-date.
What I'd like to have is an ability to specify a pattern in the hostname of
.pgpass, to replace the plenty of lines with one:
*.test.db:5432:postgres:postgres:keep!secret
bar*.*.db:5432:postgres:postgres:trustno1
The patch in attachment implements exactly this, by allowing '*' in the
hostname to substitute arbitrary number of characters until the dot. The
pattern is only matched when there is a '.' or ':' after the * and only in
the hostname, otherwise, '*' is treated like a normal character. It appears
that it can only be used for IPv4 addresses, i.e. instead of 255 hosts for
192.168.1.0/24 one can just specify 192.168.1.*.
I do understand that it might be a very limited use case in terms of
community plans for improving .pgpass, but I doubt that I'm the only one to
stumble upon the current limitation; the patch is quite small and might be
the first step into extending the functionality of .pgpass
It's currently missing the documentation, which I will add in case there
will be an interest in making this patch a part of the core.
Your feedback is greatly appreciated.
--
Regards,
Alexey Klyukin
diff --git a/src/interfaces/libpq/fe-connect.c
b/src/interfaces/libpq/fe-connect.c
new file mode 100644
index 18fcb0c..003739f
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*** static int parseServiceFile(const char *
*** 373,379
PQconninfoOption *options,
PQExpBuffer errorMessage,
bool *group_found);
! static char *pwdfMatchesString(char *buf, char *token);
static char *PasswordFromFile(char *hostname, char *port, char *dbname,
char *username);
static bool getPgPassFilename(char *pgpassfile);
--- 373,379
PQconninfoOption *options,
PQExpBuffer errorMessage,
bool *group_found);
! static char *pwdfMatchesString(char *buf, char *token, bool is_hostname);
static char *PasswordFromFile(char *hostname, char *port, char *dbname,
char *username);
static bool getPgPassFilename(char *pgpassfile);
*** defaultNoticeProcessor(void *arg, const
*** 5466,5472
* token doesn't match
*/
static char *
! pwdfMatchesString(char *buf, char *token)
{
char *tbuf,
*ttok;
--- 5466,5472
* token doesn't match
*/
static char *
! pwdfMatchesString(char *buf, char *token, bool is_hostname)
{
char *tbuf,
*ttok;
*** pwdfMatchesString(char *buf, char *token
*** 5480,5485
--- 5480,5497
return tbuf + 2;
while (*tbuf != 0)
{
+ /* '*' matches everything until '.' or end, but only for the
hostname */
+ if (*tbuf == '*' (*(tbuf + 1) == '.' || *(tbuf + 1) == ':')
+ !bslash is_hostname)
+ {
+ tbuf++;
+ while (*ttok != *tbuf)
+ {
+ if (*ttok == 0)
+ return (*tbuf == ':') ? tbuf + 1 : NULL;
+ ttok++;
+ }
+ }
if (*tbuf == '\\' !bslash)
{
tbuf++;
*** PasswordFromFile(char *hostname, char *p
*** 5588,5597
if (buf[len - 1] == '\n')
buf[len - 1] = 0;
! if ((t = pwdfMatchesString(t, hostname)) == NULL ||
! (t = pwdfMatchesString(t, port)) == NULL ||
! (t = pwdfMatchesString(t, dbname)) == NULL ||
! (t = pwdfMatchesString(t, username)) == NULL)
continue
Re: [HACKERS] Notes on implementing URI syntax for libpq
On Nov 24, 2011, at 9:40 AM, Martijn van Oosterhout wrote: On Thu, Nov 24, 2011 at 08:59:56AM +0200, Alexander Shulgin wrote: How would you specifiy a local port/UNIX domain socket? Missed that in my previous reply. If host part of the URI points to localhost, the UNIX domain socket would be considered by libpq just as if you would pass -h localhost -p 5433. Uh, no it doesn't. -h localhost uses TCP/IP (try it). This is one piece of mysql magic we don't copy. If you want to use the socket you need to specify -h /tmp or wherever you keep it. Leaving out the -h parameter also uses UNIX domain sockets. Which does raise the valid question of how to represent that in URI syntax. SQLAlchemy (for example) doesn't try with it's URL syntax, to connect to a non-default UNIX socket, you need to create the URL object directly. How about the service option, that's a nice way of handling non-default socket options. Another idea is to use local:/dir/name for UNIX domain socket instead of hostname:port, like it's displayed in the psql prompt. -- Alexey Klyukinhttp://www.commandprompt.com The PostgreSQL Company – Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pl/perl example in the doc no longer works in 9.1
On Oct 13, 2011, at 7:09 AM, Alex Hunsaker wrote:
On Wed, Oct 12, 2011 at 15:33, Alex Hunsaker bada...@gmail.com wrote:
On Wed, Oct 12, 2011 at 15:00, Tom Lane t...@sss.pgh.pa.us wrote:
The core of the problem seems to be that if SvROK(sv) then
the code assumes that it must be intended to convert that to an array or
composite, no matter whether the declared result type of the function is
compatible with such a thing.
Hrm, well 9.0 and below did not get this right either:
create or replace function test_hash() returns text as $$ return
{'a'=1}; $$ language plperl;
select test_array();
test_array
---
ARRAY(0x7fd92384dcb8)
(1 row)
create or replace function test_hash() returns text as $$ return
{'a'=1}; $$ language plperl;
select test_hash();
test_hash
--
HASH(0x7fd92387f848)
(1 row)
Given the output above (both pre 9.1 and post) it seems unless the
type is a set or composite we should throw an error. Maybe PL/Perl
function returning type %s must not return a reference ?
It would be more appropriate to drive the
cases off the nature of the function result type, perhaps.
Ill see if I can cook up something that's not too invasive.
PFA my attempt at a fix.
This gets rid of of most of the if/else chain and the has_retval crap
in plperl_handl_func(). Instead we let plperl_sv_to_datum() do most of
the lifting. It also now handles VOIDOID and checks that the request
result oid can be converted from the perl structure. For example if
you passed in a hashref with a result oid that was not an rowtype it
will error out with PL/Perl cannot convert hash to non rowtype %s.
Arrays behave similarly.
One side effect is you can now return a composite literal where you
could not before. ( We already let you return array literals )
The comments might still be a bit sparse-- Im hoping the added errors
make things a bit more self explanatory.
A large portion of the diff is added regression tests, testing what
happens when you return various references.
Comments?
Looks good at first sight and passes all regression tests for me.
--
Alexey Klyukinhttp://www.commandprompt.com
The PostgreSQL Company – Command Prompt, Inc.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pl/perl example in the doc no longer works in 9.1
On Oct 13, 2011, at 9:02 PM, Tom Lane wrote: Alex Hunsaker bada...@gmail.com writes: On Wed, Oct 12, 2011 at 15:33, Alex Hunsaker bada...@gmail.com wrote: On Wed, Oct 12, 2011 at 15:00, Tom Lane t...@sss.pgh.pa.us wrote: The core of the problem seems to be that if SvROK(sv) then the code assumes that it must be intended to convert that to an array or composite, no matter whether the declared result type of the function is compatible with such a thing. PFA my attempt at a fix. This gets rid of of most of the if/else chain and the has_retval crap in plperl_handl_func(). Instead we let plperl_sv_to_datum() do most of the lifting. It also now handles VOIDOID and checks that the request result oid can be converted from the perl structure. For example if you passed in a hashref with a result oid that was not an rowtype it will error out with PL/Perl cannot convert hash to non rowtype %s. Arrays behave similarly. I'm working through this patch now. Does anyone object to having the array-to-non-array-result-type and hash-to-non-rowtype-result-type cases throw errors, rather than returning the rather useless ARRAY(...) and HASH(...) strings as pre-9.1 did? No objections here. -- Alexey Klyukinhttp://www.commandprompt.com The PostgreSQL Company – Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] ALTER TABLE ONLY ...DROP CONSTRAINT is broken in HEAD.
This works in 9.1, but not in HEAD: CREATE TABLE parent(id INTEGER, CONSTRAINT id_check CHECK(id1)); CREATE TABLE child() INHERITS(parent); ALTER TABLE ONLY parent DROP CONSTRAINT id_check; I'm getting: ERROR: relation 16456 has non-inherited constraint id_check where 16456 is the oid of the child table. It seems that the pg_constraint scan at ATExecDropConstraint (tablecmds.c:6751) is re-reading those tuples that were updated in the previous iterations of this scan, at least that's what I've observed in gdb. I'm not sure how to fix this yet. -- Alexey Klyukinhttp://www.commandprompt.com The PostgreSQL Company – Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] REVIEW proposal: a validator for configuration files
On Sep 12, 2011, at 10:24 PM, Peter Eisentraut wrote: On ons, 2011-09-07 at 10:00 -0400, Tom Lane wrote: There has however been some debate about the exact extent of ignoring bad values during reload --- currently the theory is ignore the whole file if anything is wrong, but there's some support for applying all non-bad values as long as the overall file syntax is okay. That could be a problem if you have some values that depend on each other, and then you change both of them, but because of an error only one gets applied. I think ACID(-like) changes is a feature, also on this level. I think exactly this argument has already been discussed earlier in this thread: http://archives.postgresql.org/message-id/21310d95-eb8d-4b15-a8bc-0f05505c6...@phlo.org -- Alexey Klyukinhttp://www.commandprompt.com The PostgreSQL Company – Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] REVIEW proposal: a validator for configuration files
Hi Andy, On Sep 7, 2011, at 6:40 AM, Andy Colson wrote: Hi Alexey, I was taking a quick look at this patch, and have a question for ya. ... Where did the other warnings go? Its right though, line 570 is bad. It also seems to have killed the server. I have not gotten through the history of messages regarding this patch, but is it supposed to kill the server if there is a syntax error in the config file? Thank you for looking at this patch. v4 was more a what if concept that took a lot of time for somebody to look at. There were a lot of problems with it, but I think I've nailed down most of them. Attached is v5. It should fix both problems you've experienced with v4. As with the current code, the startup process gets interrupted on any error detected in the configuration file. Unlike the current code, the patch tries to report all of them before bailing out. The behavior during configuration reload has changed significantly. Instead of ignoring all changes after the first error, the code reports the problematic value and continues. It only skips applying new values completely on syntax errors and invalid configuration option names. In no cases should it bring the server down during reload. One problem I'm not sure how to address is the fact that we require 2 calls of set_config_option for each option, one to verify the new value and another to actually apply it. Normally, this function returns true for a valid value and false if it is unable to set the new value for whatever reason (either the value is invalid, or the value cannot be changed in the context of the caller). However, calling it once per value in the 'apply' mode during reload produces false for every unchanged value that can only be changed during startup (i.e. shared_buffers, or max_connections). If we ignore its return value, we'll lose the ability to detect whether invalid values were encountered during the reload and report this fact at the end of the function. I think the function should be changed, as described in my previous email (http://archives.postgresql.org/message-id/97a66029-9d3e-43af-95aa-46fe1b447...@commandprompt.com) and I'd like to hear other opinions on that. Meanwhile, due to 2 calls to set_config_option, it currently reports all invalid values twice. If others will be opposed to changing the set_config_option, I'll fix this by removing the first, verification call and final 'errors were detected' warning to avoid 'false positives' on that (i.e. the WARNING you saw with the previous version for the valid .conf). I'd appreciate your further comments and suggestions. Thank you. -- Alexey Klyukinhttp://www.commandprompt.com The PostgreSQL Company – Command Prompt, Inc. pg_parser_continue_on_error_v5.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] REVIEW proposal: a validator for configuration files
Hello,
On Sep 7, 2011, at 5:00 PM, Tom Lane wrote:
Andy Colson a...@squeakycode.net writes:
Where did the other warnings go? Its right though, line 570 is bad. It
also seems to have killed the server. I have not gotten through the history
of messages regarding this patch, but is it supposed to kill the server if
there is a syntax error in the config file?
The historical behavior is that a configuration file error detected
during postmaster startup should prevent the server from starting, but
an error detected during reload should only result in a LOG message and
the reload not occurring. I don't believe anyone will accept a patch
that causes the server to quit on a failed reload. There has however
been some debate about the exact extent of ignoring bad values during
reload --- currently the theory is ignore the whole file if anything is
wrong, but there's some support for applying all non-bad values as long
as the overall file syntax is okay.
This patch actually aims to do the latter, applying all good values and
reporting the bad ones. It removes the calls to set_config_option with
changeVal = false from ProcessConfigFile, trying to apply every option at once
and incrementing the warnings counter if set_config_option returns false. After
some investigation I've discovered that set_config_option returns false even if
a variable value is unchanged, but is applied in the wrong GucContext. The
particular case is trying to reload the configuration file variables during
SIGHUP: if, say, shared_buffers are commented out, the call to
set_config_option with changeVal = true will return false due to this code:
if (prohibitValueChange)
{
if (*conf-variable != newval)
ereport(elevel,
(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
errmsg(parameter \%s\ cannot be changed without restarting the server,
name)));
return false;
}
Due to the above, set_config_option returns false for unchanged PGC_POSTMASTER
variables during SIGHUP, so it's impossible to distinguish between valid and
non valid values and report the latter ones with a single call of this function
per var. What do you think about changing the code above to return true if the
variable is actually unchanged?
This explains the WARNINGs emitted during reload even for a pristine
configuration file right after the installation. I'm looking into why the
server gets killed during reload if there is a parse error, it might be as well
related to the problem above.
--
Alexey Klyukinhttp://www.commandprompt.com
The PostgreSQL Company – Command Prompt, Inc.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] plperl crash with Debian 6 (64 bit), pl/perlu, libwww and https
On Aug 4, 2011, at 5:25 PM, Alvaro Herrera wrote:
Excerpts from Hannu Krosing's message of jue ago 04 09:53:40 -0400 2011:
On Thu, 2011-08-04 at 09:42 -0400, Andrew Dunstan wrote:
On 08/04/2011 09:07 AM, Hannu Krosing wrote:
I have been helping some people to debug a SIGALARM related crash
induced by using pl/perlu http get functionality
I have been so far able to repeat the crash only on Debian 64 bit
computers. DB create script and instructions for reproducing the crash
attached
The crash is related to something leaving begind a bad SIGALARM handler,
as it can be (kind of) fixed by resetting sigalarm to nothing using perl
function
So doesn't this look like a bug in the perl module that sets the signal
handler and doesn't restore it?
I vaguely remember looking in the guts of LWP::UserAgent a few years ago
and being rather annoyed at the way it dealt with sigalrm -- it
interfered with other uses we had for the signal. I think we had to run
a patched version of that module or something, not sure.
What happens if you wrap the calls to the module like this?:
{
local $SIG{ALRM};
# do LWP stuff here
}
return 'OK';
That should restore the old handler on exit from the block.
Sure, but how expensive would it be for pl/perl to do this
automatically ?
Probably too much, but then since this is an untrusted pl my guess is
that it's OK to request the user to do it only in functions that need
it. I wonder if we could have a check on return from a function that
the sighandler is still what we had before the function was called, to
help discover this problem.
If we can do that, than why won't we move a step further and restore an old
signal handler on mismatch?
--
Command Prompt, Inc. http://www.CommandPrompt.com
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Jul 16, 2011, at 9:55 PM, Tom Lane wrote: I wrote: I think that it might be sensible to have the following behavior: 1. Parse the file, where parse means collect all the name = value pairs. Bail out if we find any syntax errors at that level of detail. (With this patch, we could report some or all of the syntax errors first.) 2. Tentatively apply the new custom_variable_classes setting if any. 3. Check to see whether all the names are valid. If not, report the ones that aren't and bail out. 4. Apply each value. If some of them aren't valid, report that, but continue, and apply all the ones that are valid. We can expect that the postmaster and all backends will agree on the results of steps 1 through 3. They might differ as to the validity of individual values in step 4 (as per my example of a setting that depends on database_encoding), but we should never end up with a situation where a globally correct value is not globally applied. Attached is my first attempt to implement your plan. Basically, I've reshuffled pieces of the ProcessConfigFile on top of my previous patch, dropped verification calls of set_config_option and moved the check for custom_variable_class existence right inside the loop that assigns new values to GUC variables. I'd think that removal of custom_variable_classes or setting it from the extensions could be a separate patch. I appreciate your comments and suggestions. I thought some more about this, and it occurred to me that it's not that hard to foresee a situation where different backends might have different opinions about the results of step 3, ie, different ideas about the set of valid GUC names. This could arise as a result of some of them having a particular extension module loaded and others not. Right now, whether or not you have say plpgsql loaded will not affect your ability to do SET plpgsql.junk = foobar --- as long as plpgsql is listed in custom_variable_classes, we'll accept the command and create a placeholder variable for plpgsql.junk. But it seems perfectly plausible that we might someday try to tighten that up so that once a module has done EmitWarningsOnPlaceholders(plpgsql), we'll no longer allow creation of new placeholders named plpgsql.something. If we did that, we could no longer assume that all backends agree on the set of legal GUC variable names. So that seems like an argument --- not terribly strong, but still an argument --- for doing what I suggested next: The original argument for the current behavior was to avoid applying settings from a thoroughly munged config file, but I think that the checks involved in steps 1-3 would be sufficient to reject files that had major problems. It's possible that step 1 is really sufficient to cover the issue, in which case we could drop the separate step-3 pass and just treat invalid GUC names as a reason to ignore the particular line rather than the whole file. That would make things simpler and faster, and maybe less surprising too. IOW, I'm now pretty well convinced that so long as the configuration file is syntactically valid, we should go ahead and attempt to apply each name = value setting individually, without allowing the invalidity of any one name or value to prevent others from being applied. -- Command Prompt, Inc. http://www.CommandPrompt.com PostgreSQL Replication, Consulting, Custom Development, 24x7 support pg_parser_continue_on_error_v4.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Jul 14, 2011, at 4:38 AM, Alvaro Herrera wrote: Excerpts from Florian Pflug's message of mié jul 13 20:12:28 -0400 2011: On Jul14, 2011, at 01:38 , Alvaro Herrera wrote: One strange thing here is that you could get two such messages; say if a file has 100 parse errors and there are also valid lines that contain bogus settings (foo = bar). I don't find this to be too problematic, and I think fixing it would be excessively annoying. For example, a bogus run would end like this: 95 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 4, near end of line 96 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 41, near end of line 97 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 104, near end of line 98 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 156, near end of line 99 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 208, near end of line 100 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 260, near end of line 101 LOG: too many errors found, stopped processing file /pgsql/install/HEAD/data/postgresql.conf 102 LOG: unrecognized configuration parameter plperl.err 103 LOG: unrecognized configuration parameter this1 104 LOG: too many errors found, stopped processing file /pgsql/install/HEAD/data/postgresql.conf 105 FATAL: errors detected while parsing configuration files How about changing ParseConfigFile to say too many *syntax* error found instead? It'd be more precise, and we wouldn't emit exactly the same message twice. Yeah, I thought about doing it that way but refrained because it'd be one more string to translate. That's a poor reason, I admit :-) I'll change it. This is happening because a check for total number of errors so far is happening only after coming across at least one non-recognized configuration option. What about adding one more check right after ParseConfigFile, so we can bail out early when overwhelmed with syntax errors? This would save a line in translation :). Do you want me to take a closer look at your modified version of the patch before you commit, or did you post it more as a FYI, this is how it's going to look like? I know I'd feel more comfortable if you (and Alexey, and Selena) gave it another look :-) I have checked it here and don't see any more problems with it. -- Command Prompt, Inc. http://www.CommandPrompt.com PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] storing TZ along timestamps
Hi, On May 27, 2011, at 11:43 PM, Alvaro Herrera wrote: One of our customers is interested in being able to store original timezone along with a certain timestamp. It is currently possible to store a TZ in a separate column, but this is a bit wasteful and not very convenient anyway. There are all sorts of UI issues that need to be resolved in order for this to be a complete feature proposal, but the first thing that we discussed was what is the storage going to look like. Of course, one thing we don't want is to store the complete TZ name as text. So the first thing is cataloguing timezone names, and assigning an ID to each (maybe an OID). If we do that, then we can store the OID of the timezone name along the int64/float8 of the actual timestamp value. So, I'd think there are 2 reasonable approaches to storing the timezone part: 1. Store the timezone abbreviation (i.e. 'EST' along w/ the timestamp data). 2. Assign OID to each of the timezones and store it w/ the timestamp. The first option seem to avoid the necessity of creating a new system catalog for timezone information and the burden of updating it, because current implementation is already capable of translating abbreviations to useful timezone information. The question is, whether just a TZ abbreviation is sufficient to uniquely identify the timezone and get the offset and DST rules. If it's not sufficient, how conflicting TZ short names are handled in the current code (i.e. 'AT TIME ZONE ...')? The second choice doesn't avoids the issue of ambiguous names, although it requires moving TZ information inside the database and providing some means to update it. There were mentions of potential problems w/ pg_upgrade and pg_dump, if we add a massive amount of oids for the timezones. What are these problems specifically? I'd thing that storing TZ abbreviations is more straightforward and easier to implement, unless there are too ambiguous to identify the timezone correctly. Note that I am currently proposing to store only the zone names in the catalog, not the full TZ data. Where would we store other bits of timezone information? Wouldn't it be inconvenient to update names in system catalogs and DST rules elsewhere? Alexey. -- Command Prompt, Inc. http://www.CommandPrompt.com PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Jun 20, 2011, at 6:22 PM, Florian Pflug wrote: On Jun20, 2011, at 17:02 , Alexey Klyukin wrote: I don't think it has changed at all. Previously, we did goto cleanup_list (or cleanup_exit in ParseConfigFp) right after the first error, no matter whether that was a postmaster or its child. What I did in my patch is removing the goto for the postmaster's case. It was my intention to exit after the initial error for the postmaster's child, to avoid complaining about all errors both in the postmaster and in the normal backend (imagine seeing 100 errors from the postmaster and the same 100 from each of the backends if your log level is DEBUG2). I think the postmaster's child case won't cause any problems, since we do exactly what we used to do before. Hm, I think you miss-understood what I was trying to say, probably because I explained it badly. Let me try again. I fully agree that there *shouldn't* be any difference in behaviour, because it *shouldn't* matter whether we abort early or not - we won't have applied any of the settings anway. But. The code the actually implements the check settings first, apply later logic isn't easy to read. Now, assume that this code has a bug. Then, with your patch applied, we might end up with the postmaster applying a setting (because it didn't abort early) but the backend ignoring it (because they did abort early). This is obviously bad. Depending on the setting, the consequences may range from slightly confusing behaviour to outright crashes I guess... Now, the risk of that happening might be very small. But on the other hand, the benefit is pretty small also - you get a little less output for log level DEBUG2, that's it. A level that people probably don't use for the production databases anyway. This convinced me that the risk/benefit ratio isn't high enough to warrant the early abort. Another benefit of removing the check is that it reduces code complexity. Maybe not when measured in line counts, but it's one less outside factor that changes ProcessConfigFiles()'s behaviour and thus one thing less you need to think when you modify that part again in the future. Again, it's a small benefit, but IMHO it still outweights the benefit. While I agree that making the code potentially less bug prone is a good idea, I don't agree with the statement that since DEBUG2 output gets rarely turned on we can make it less usable, hoping that nobody would use in production. Having said that, this is my personal opinion and whoever will eventually commit this may very will assess the cost/benefit ratio differently. So, if after this more detailed explanations of my reasoning, you still feel that it makes sense to keep the early abort, then feel free to mark the patch Ready for Committer nevertheless. I'd say that this issue is a judgement call. Depending on a point of view, both arguments are valid. I've marked this patch as 'Ready for Committer' w/o removing the early abort stuff, but if there will be more people willing to remove them - I'll do that. Thank you, Alexey. -- Command Prompt, Inc. http://www.CommandPrompt.com PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Identifying no-op length coercions
Hi,
On Jun 19, 2011, at 2:10 PM, Noah Misch wrote:
On Sat, Jun 18, 2011 at 11:32:20PM -0400, Robert Haas wrote:
On Sat, Jun 18, 2011 at 11:12 PM, Robert Haas robertmh...@gmail.com wrote:
On Sat, Jun 18, 2011 at 11:06 PM, Noah Misch n...@leadboat.com wrote:
On Sat, Jun 18, 2011 at 10:57:13PM -0400, Robert Haas wrote:
On Sat, Jun 11, 2011 at 5:13 PM, Noah Misch n...@leadboat.com wrote:
Sounds good. ?Updated patch attached, incorporating your ideas. ?Before
applying
it, run this command to update the uninvolved pg_proc.h DATA entries:
?perl -pi -e 's/PGUID(\s+\d+){4}/$ 0/' src/include/catalog/pg_proc.h
This doesn't quite apply any more. ?I think the pgindent run broke it
slightly.
Hmm, I just get two one-line offsets when applying it to current master.
?Note
that you need to run the perl invocation before applying the patch. ?Could
you
provide full output of your `patch' invocation, along with any reject
files?
Ah, crap. ?You're right. ?I didn't follow your directions for how to
apply the patch. ?Sorry.
I think you need to update the comment in simplify_function() to say
that we have three strategies, rather than two. I think it would also
be appropriate to add a longish comment just before the test that
calls protransform, explaining what the charter of that function is
and why the mechanism exists.
Good idea. See attached.
Documentation issues aside, I see very little not to like about this.
Great! Thanks for reviewing.
noop-length-coercion-v3.patch
Here is my review of this patch.
The patch applies cleanly to the HEAD, produces no additional warnings. It
doesn't include additional regression tests. One can include a test, using the
commands like the ones included below.
Changes to the documentation are limited to the a description of a new
pg_class attribute. It would probably make sense to document all the
exceptions to the table's rewrite on ALTER TABLE documentation page, although
it could wait for more such exceptions.
The feature works as intended and I haven't noticed any crashes or assertion
failures, i.e.
postgres=# create table test(id integer, name varchar);
CREATE TABLE
postgres=# insert into test values(1, 'test');
INSERT 0 1
postgres=# select relfilenode from pg_class where oid='test'::regclass;
relfilenode
-
66302
(1 row)
postgres=# alter table test alter column name type varchar(10);
ALTER TABLE
postgres=# select relfilenode from pg_class where oid='test'::regclass;
relfilenode
-
66308
(1 row)
postgres=# alter table test alter column name type varchar(65535);
ALTER TABLE
postgres=# select relfilenode from pg_class where oid='test'::regclass;
relfilenode
-
66308
(1 row)
The code looks good and takes into account all the previous suggestions.
The only nitpick code-wise is these lines in varchar_transform:
+ int32 old_max = exprTypmod(source) - VARHDRSZ;
+ int32 new_max = new_typmod - VARHDRSZ;
I have a hard time understanding why VARHDRSZ is subtracted here, so I'd
assume that's a bug.
Other than that, I haven't noticed any issues w/ this patch.
Sincerely,
Alexey.
--
Command Prompt, Inc. http://www.CommandPrompt.com
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Identifying no-op length coercions
On Jun 21, 2011, at 9:58 PM, Noah Misch wrote: A pg_regress test needs stable output, so we would do it roughly like this: CREATE TEMP TABLE relstorage AS SELECT 0::regclass AS oldnode; ... UPDATE relstorage SET oldnode = (SELECT relfilenode FROM pg_class WHERE oid = 'test'::regclass); ALTER TABLE test ALTER name TYPE varchar(65535); SELECT oldnode relfilenode AS rewritten FROM pg_class, relstorage WHERE oid = 'test'::regclass; I originally rejected that as too ugly to read. Perhaps not. Yes, your example is more appropriate. I think you can make it more straightforward by getting rid of the temp table: CREATE TABLE test(oldnode oid, name varchar(5)); INSERT INTO test(oldnode) SELECT relfilenode FROM pg_class WHERE oid='test'::regclass; ALTER TABLE test ALTER name TYPE varchar(10); SELECT oldnode relfilenode AS rewritten FROM pg_class, test WHERE oid='test'::regclass; The only nitpick code-wise is these lines in varchar_transform: +int32 old_max = exprTypmod(source) - VARHDRSZ; +int32 new_max = new_typmod - VARHDRSZ; I have a hard time understanding why VARHDRSZ is subtracted here, so I'd assume that's a bug. We track the varchar typmod internally as (max length) + VARHDRSZ. Oh, right, haven't thought that this is a varchar specific thing. Thank you, Alexey. -- Command Prompt, Inc. http://www.CommandPrompt.com PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Florian, On Jun 18, 2011, at 5:40 PM, Florian Pflug wrote: On Jun16, 2011, at 22:34 , Alexey Klyukin wrote: Attached is the v2 of the patch to show all parse errors at postgresql.conf. Changes (per review and suggestions from Florian): - do not stop on the first error during postmaster's start. - fix errors in processing files from include directives. - show only a single syntax error per line, i.e. fast forward to the EOL after coming across the first one. - additional comments/error messages, code cleanup Looks mostly good. I found one issue which I missed earlier. As it stands, the behaviour of ProcessConfigFile() changes subtly when IsUnderPostmaster because of the early abort on errors. Now, in theory the outcome should still be the same, since we don't apply any settings if there's an error in one of them. But still, there's a risk that this code isn't 100% waterproof, and then we'd end up with different settings in the postmaster compared to the backends. The benefit seems also quite small - since the backends emit their messages at DEBUG2, you usually won't see the difference anyway. I don't think it has changed at all. Previously, we did goto cleanup_list (or cleanup_exit in ParseConfigFp) right after the first error, no matter whether that was a postmaster or its child. What I did in my patch is removing the goto for the postmaster's case. It was my intention to exit after the initial error for the postmaster's child, to avoid complaining about all errors both in the postmaster and in the normal backend (imagine seeing 100 errors from the postmaster and the same 100 from each of the backends if your log level is DEBUG2). I think the postmaster's child case won't cause any problems, since we do exactly what we used to do before. The elevel setting at the start of ProcessConfigFile() also seemed needlessly complex, since we cannot have IsUnderPostmaster and context == PGCS_POSTMASTER at the same time. Agreed. I figured it'd be harder to explain the fixes I have in mind than simply do them and let the code speak. Attached you'll find an updated version of your v2 patch (called v2a) as well as an incremental patch against your v2 (called v2a_delta). The main changes are the removal of the early aborts when IsUnderPostmaster and the simplification of the elevel setting logic in ProcessConfigFile(). Attached is the new patch and the delta. It includes some of the changes from your patch, while leaving the early abort stuff for postmaster's children. Thank you, Alexey. -- Command Prompt, Inc. http://www.CommandPrompt.com PostgreSQL Replication, Consulting, Custom Development, 24x7 support pg_parser_continue_on_error_v2b.patch Description: Binary data pg_parser_continue_on_error_v2b_delta.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Florian, On Jun 16, 2011, at 2:34 PM, Florian Pflug wrote: Hi On May14, 2011, at 00:49 , Alexey Klyukin wrote: The patch forces the parser to report all errors (max 100) from the ProcessConfigFile/ParseConfigFp. Currently, only the first parse error or an invalid directive is reported. Reporting all of them is crucial to automatic validation of postgres config files. This patch is based on the one submitted earlier by Selena Deckelmann: http://archives.postgresql.org/pgsql-hackers/2009-03/msg00345.php It incorporates suggestions by Tom Lane for avoiding excessive bloat in logs in case there is a junk instead of postgresql.conf. http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php Here's my review of your patch. The patch is in context diff format and applies cleanly to HEAD. It doesn't contain superfluous whitespace changes any more is and quite readable. First, the behaviour. The first problem I ran into when I tried to test this is that it *only* reports multiple errors during config file reload on SIHUP, not during postmaster startup. I guess it's been done that way because we ereport(ERROR,..) not ereport(LOG,...) during postmaster startup, so it's not immediatly clear how to report multiple errors. But that proplem seems solvable. What if you ereport(LOG,..)ed the individual errors during postmaster startup, and then emitted an ereport(ERROR) at the end if errors occurred? The ERROR could either repeat the first error that was encountered, or simply say config file contains errors. Makes sense. One problem I came across is that set_config_option from guc.c sets the elevel by itself. I've changed it to emit LOG errors on PGC_S_FILE source, apparently all the callers of this function with this source are from guc-file.l, so hopefully I won't break anything with this change. Now to the code. I see that you basically replaced goto cleanup... in both ParseConfigFp() and ProcessConfigFile() with ++errorcount, and arranged for ParseConfigFp() to return false, and for ProcessConfigFile() to skip the GUC updates if errorcount 0. The actual value of errorcount is never inspected. The value is also wrong in the case of include files containing more than error, since ParseConfigFp() simply increments errorcount by one for each failed ParseConfigFile() of an included file. I thus suggest that you replace errorcount with a boolean erroroccurred. I can actually pass errorcount down from the ParseConfigFp() to report the total number of errors (w/ the include files) at the end of ProcessConfigFile if there is any interest in having the number of errors reported to a user. If not - I'll change it to boolean. You've also introduced a bug in ParseConfigFp(). Previously, if an included file contained an error, it did goto cleanup_exit() and thus didn't ereport() on it's own. With your patch applied it ereport()s a parse error at the location of the include directive, which seems wrong. Right, I noticed that I skipped switching the buffers and restoring the Lineno as well. I'll fix it in the next revision. Finally, I believe that ParseConfigFp() should make at least some effort to resync after hitting a parser error. I suggest that you simply fast-forward to the next GUC_EOL token after reporting a parser error. Makes sense. Thank you for the review. I'll post an updated patch, addressing you concerns, shortly. Alexey. -- Command Prompt, Inc. http://www.CommandPrompt.com PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Jun 16, 2011, at 6:49 PM, Florian Pflug wrote: On Jun16, 2011, at 17:23 , Alexey Klyukin wrote: On Jun 16, 2011, at 2:34 PM, Florian Pflug wrote: The first problem I ran into when I tried to test this is that it *only* reports multiple errors during config file reload on SIHUP, not during postmaster startup. I guess it's been done that way because we ereport(ERROR,..) not ereport(LOG,...) during postmaster startup, so it's not immediatly clear how to report multiple errors. But that proplem seems solvable. What if you ereport(LOG,..)ed the individual errors during postmaster startup, and then emitted an ereport(ERROR) at the end if errors occurred? The ERROR could either repeat the first error that was encountered, or simply say config file contains errors. Makes sense. One problem I came across is that set_config_option from guc.c sets the elevel by itself. I've changed it to emit LOG errors on PGC_S_FILE source, apparently all the callers of this function with this source are from guc-file.l, so hopefully I won't break anything with this change. Hm, wouldn't a test for context == PGC_POSTMASTER be more appropriate? In such a case the errors caused by command-line arguments won't stop the postmaster. PGC_S_FILE seems to handle this correctly. I'm not sure whether it is appropriate to use there though. I see that you basically replaced goto cleanup... in both ParseConfigFp() and ProcessConfigFile() with ++errorcount, and arranged for ParseConfigFp() to return false, and for ProcessConfigFile() to skip the GUC updates if errorcount 0. The actual value of errorcount is never inspected. The value is also wrong in the case of include files containing more than error, since ParseConfigFp() simply increments errorcount by one for each failed ParseConfigFile() of an included file. I thus suggest that you replace errorcount with a boolean erroroccurred. I can actually pass errorcount down from the ParseConfigFp() to report the total number of errors (w/ the include files) at the end of ProcessConfigFile if there is any interest in having the number of errors reported to a user. If not - I'll change it to boolean. Nah, just use a boolean, unless you have concrete plans to actually use the errorcount for something other than test a la errorcount 0. I just recalled a reason for counting the total number of errors. There is a condition that checks that the total number of errors is less than 100 and bails out if it's more than that (100 is arbitrary). The reason is to avoid bloating the logs w/ something totally unrelated to postgresql.conf. That was suggested by Tom Lane here: http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php Thank you, Alexey. -- Command Prompt, Inc. http://www.CommandPrompt.com PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Jun 16, 2011, at 8:01 PM, Florian Pflug wrote: On Jun16, 2011, at 18:46 , Alexey Klyukin wrote: On Jun 16, 2011, at 6:49 PM, Florian Pflug wrote: Hm, wouldn't a test for context == PGC_POSTMASTER be more appropriate? In such a case the errors caused by command-line arguments won't stop the postmaster. PGC_S_FILE seems to handle this correctly. I'm not sure whether it is appropriate to use there though. Ah, yeah, you're right. PGC_S_FILE sounds fine, then. I guess this means you can drop the check for context == PGC_SIGHUP though, because surely the source must be PGC_S_DEFAULT or PGC_S_FILE if context == PGC_SIGHUP, right? So the check would become if (source == PGC_S_FILE || source == PGC_S_DEFAULT) where it now says if (context == PGC_SIGHUP || source == PGC_S_DEFAULT) Yes, AFAIK PGC_SIGHUP is redundant, thank you for the suggestion. I just recalled a reason for counting the total number of errors. There is a condition that checks that the total number of errors is less than 100 and bails out if it's more than that (100 is arbitrary). The reason is to avoid bloating the logs w/ something totally unrelated to postgresql.conf. That was suggested by Tom Lane here: http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php Ah, right, I missed that. Guess it'll have to stay a counter, then. Still, I don't think it's worth the effort to make the count correct in case of included files, so I'd say just add a comment explaining that the count isn't totally accurate. Well, while thinking about this I decided to leave the counter for the ParseConfigFp, but drop it in ProcessConfigFile. The case we are protecting against is a single file full of junk. It's unlikely that this junk would contain include directives with valid file paths, neither it's likely to find a file with a correct syntax, but full of invalid directives. Thank you, Alexey. -- Command Prompt, Inc. http://www.CommandPrompt.com PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Hi, On Jun 16, 2011, at 9:18 PM, Florian Pflug wrote: On Jun16, 2011, at 20:14 , Alexey Klyukin wrote: Well, while thinking about this I decided to leave the counter for the ParseConfigFp, but drop it in ProcessConfigFile. The case we are protecting against is a single file full of junk. It's unlikely that this junk would contain include directives with valid file paths, neither it's likely to find a file with a correct syntax, but full of invalid directives. Sounds good. Attached is the v2 of the patch to show all parse errors at postgresql.conf. Changes (per review and suggestions from Florian): - do not stop on the first error during postmaster's start. - fix errors in processing files from include directives. - show only a single syntax error per line, i.e. fast forward to the EOL after coming across the first one. - additional comments/error messages, code cleanup Questions: - Should we add a comment for the changes in guc.c? I think the existing ones are still valid, but they might be harder go grasp, given that we've removed PGC_SIGHUP from the condition. - The error message that we emit when the parsing is unsuccessful, will it cause incompatibility w/ 3rd party tools, which may, in theory, show only one error message (would it better to show the first error instead, as proposed by Florian?). I'd appreciate your comments and suggestions. Thank you, Alexey. -- Command Prompt, Inc. http://www.CommandPrompt.com PostgreSQL Replication, Consulting, Custom Development, 24x7 support parser_continue_on_errors_v2.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Operator families vs. casts
Noah, Providing my thoughts on the 'mundane' question first. On Tue, May 24, 2011 at 1:40 PM, Noah Misch n...@leadboat.com wrote: I also had a more mundane design question in the second paragraph of [2]. It can probably wait for the review of the next version of the patch. However, given that it affects a large percentage of the patch, I'd appreciate any early feedback on it. Here's the relevant part of the original post: ATPostAlterTypeCleanup has this comment: /* * Re-parse the index and constraint definitions, and attach them to the * appropriate work queue entries. We do this before dropping because in * the case of a FOREIGN KEY constraint, we might not yet have exclusive * lock on the table the constraint is attached to, and we need to get * that before dropping. It's safe because the parser won't actually look * at the catalogs to detect the existing entry. */ Is the second sentence true? I don't think so, so I deleted it for now. Here is the sequence of lock requests against the table possessing the FOREIGN KEY constraint when we alter the parent/upstream column: transformAlterTableStmt - ShareRowExclusiveLock ATPostAlterTypeParse - lockmode of original ALTER TABLE RemoveTriggerById() for update trigger - ShareRowExclusiveLock RemoveTriggerById() for insert trigger - ShareRowExclusiveLock RemoveConstraintById() - AccessExclusiveLock CreateTrigger() for insert trigger - ShareRowExclusiveLock CreateTrigger() for update trigger - ShareRowExclusiveLock RI_Initial_Check() - AccessShareLock (3x) I think the statement in the second sentence of the comment is true. ATPostAlterTypeParse is called only from ATPostAlterTypeCleanup. It has to grab the lock on the table the constraint is attached to before dropping the constraint. What it does is it opens that relation with the same lock that is grabbed for AT_AlterColumnType subtype, i.e. AccessExclusiveLock. I think there is no preceding place in AlterTable chain, that grabs stronger lock on this relation. ShareRowExclusiveLock is taken in transformAlterTableStmt (1st function in your sequence), but this function is ultimately called from ATPostAlterTypeParse, just before the latter grabs the AccessExclusiveLock, so ultimately at the point where this comment is located no locks are taken for the table with a FOREIGN KEY constraint. Alexey. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Identifying no-op length coercions
Hi, On Jun 2, 2011, at 10:22 PM, Noah Misch wrote: Hi Alexey, ... Is your interest in cheap varchar(N)-varchar(N+M) conversions specifically, or in some broader application of this facility? Exactly varchar conversions. Thanks for volunteering to review; that will be a big help. Actually, I could especially use some feedback now on a related design and implementation: http://archives.postgresql.org/message-id/20110524104029.gb18...@tornado.gateway.2wire.net Note that the third and fifth sentences of that description are incorrect. The rest stands without them. Even just some feedback on the mundane issue noted in the last paragraph would help. Ok, I'll review it. Thank you, Alexey. -- Command Prompt, Inc. http://www.CommandPrompt.com PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Estimating total amount of shared memory required by postmaster
On Jun 2, 2011, at 10:49 PM, Tom Lane wrote: Alexey Klyukin al...@commandprompt.com writes: We've recently come across the task of estimating the size of shared memory required for PostgreSQL to start. ... - Try to actually allocate the shared memory in a way postmaster does this nowadays, if the process fails - analyze the error code to check whether the failure is due to the shmmax or shmmall limits being too low. This would need to be run as a separate process (not postmaster's child) to avoid messing with the postmaster's own shared memory, which means that this would be hard to implement as a user-callable stored function. The results of such a test wouldn't be worth the electrons they're written on anyway: you're ignoring the likelihood that two instances of shared memory would overrun the kernel's SHMALL limit, when a single instance would be fine. As Alvaro already pointed out, I'm not ignoring shmall. I think that: - shmmax case is more frequent. - there is a way to detect that shmall is a problem. The other question is what to do when we detect that shmall is a problem. I don't have a good answer for that ATM. Given that you can't do it in the context of a live installation, just trying to start the postmaster and seeing if it works (same as initdb does) seems as good as anything else. In a context of configuration files validator a postmaster restart is definitely not what we are looking for. Alexey. -- Command Prompt, Inc. http://www.CommandPrompt.com PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Identifying no-op length coercions
On May 24, 2011, at 12:15 AM, Noah Misch wrote: On Mon, May 23, 2011 at 03:01:40PM -0400, Tom Lane wrote: Noah Misch n...@leadboat.com writes: Good deal. Given that conclusion, the other policy decision I anticipate affecting this particular patch is the choice of syntax. Presumably, it will be a new common_func_opt_item. When I last looked at the keywords list and tried to come up with something, these were the best I could do: CREATE FUNCTION ... PARSER MAPPING helperfunc(args) CREATE FUNCTION ... PLANS CONVERSION helperfunc(args) We could go with your previous idea of not bothering to expose this in the SQL syntax. Given that the helper function is going to have a signature along the lines of (internal, internal) - internal, it's going to be difficult for anyone to use it for non-builtin functions anyhow. But if you really don't like that, what about That would be just fine with me. We can always expose it later. TRANSFORM helperfunctionname Although TRANSFORM isn't currently a keyword for us, it is a non-reserved keyword in SQL:2008, and it seems possible that we might someday think about implementing the associated features. I was thinking of that word too, along the lines of PLAN TRANSFORM FUNCTION helperfunctionname. Then again, that wrongly sounds somewhat like it's transforming planner nodes. Perhaps plain TRANSFORM or TRANSFORM FUNCTION would be the way to go. Looks like this thread has silently died out. Is there an agreement on the syntax and implementation part? We (CMD) have a customer, who is interested in pushing this through, so, if we have a patch, I'd be happy to assist in reviewing it. -- Alexey Klyukin The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Estimating total amount of shared memory required by postmaster
Hello, We've recently come across the task of estimating the size of shared memory required for PostgreSQL to start. This comes from the problem of validating postgresql.conf files (http://archives.postgresql.org/pgsql-hackers/2011-03/msg01831.php), i.e. checking that the server will be able to start with new configuration options without actually performing the restart. Currently, I see a couple of ways to get the estimate: - Use the code from ipci.c to get the total size of the shared memory segment that Postmaster would be allocating with the given configuration options (shared_buffers, etc.). This would require getting the actual amount of available shared memory somehow, which is platform dependent and might not be very reliable. The other downside is that the code would need to be updated if the original estimates in ipci.c changes. - Try to actually allocate the shared memory in a way postmaster does this nowadays, if the process fails - analyze the error code to check whether the failure is due to the shmmax or shmmall limits being too low. This would need to be run as a separate process (not postmaster's child) to avoid messing with the postmaster's own shared memory, which means that this would be hard to implement as a user-callable stored function. I'm also looking for other ideas. Any suggestions? Thank you, Alexey -- Command Prompt, Inc. http://www.CommandPrompt.com PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Hi, On Apr 14, 2011, at 9:50 PM, Robert Haas wrote: On Mon, Apr 4, 2011 at 2:03 PM, Alexey Klyukin al...@commandprompt.com wrote: Here's the update of Selena's patch, which also shows all errors in configuration parameters (as well as parser errors) during reload. You should add this here: https://commitfest.postgresql.org/action/commitfest_view/open On a quick glance, this patch appears to contain some superfluous hunks where you changed whitespace or variable names. You might want to remove those and repost before adding to the CF app. Also, some submission notes would be very helpful - when you send in the revised version, detail in the email the exact purpose of the changes so that someone can review the patch without having to read this thread and all preceding threads in their entirety. Thank you for the feedback, I've updated the patch, attached is a new version. I'll add it to the commitfest after posting this message. The patch forces the parser to report all errors (max 100) from the ProcessConfigFile/ParseConfigFp. Currently, only the first parse error or an invalid directive is reported. Reporting all of them is crucial to automatic validation of postgres config files. This patch is based on the one submitted earlier by Selena Deckelmann: http://archives.postgresql.org/pgsql-hackers/2009-03/msg00345.php It incorporates suggestions by Tom Lane for avoiding excessive bloat in logs in case there is a junk instead of postgresql.conf. http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php Regards, -- Alexey Klyukin The PostgreSQL Company - Command Prompt, Inc. parser_continue_on_errors.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 'tuple concurrently updated' error for alter role ... set
On May 13, 2011, at 2:07 AM, Alexey Klyukin wrote: On May 13, 2011, at 1:28 AM, Tom Lane wrote: We're not likely to do that, first because it's randomly different from the handling of every other system catalog update, and second because it would serialize all updates on this catalog, and probably create deadlock cases that don't exist now. (BTW, as the patch is given I'd expect it to still fail, though perhaps with lower probability than before. For this to actually stop all such cases, you'd have to hold the lock till commit, which greatly increases the risks of deadlock.) I see no particular reason why conflicting updates like those *shouldn't* be expected to fail occasionally. Excellent question, I don't have enough context to properly answer that (other than a guess that an unexpected transaction rollback is too unexpected :)) Let me ask the customer first. The original use case is sporadical failures of some internal unit tests due to the error message in subject. -- Alexey Klyukin The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 'tuple concurrently updated' error for alter role ... set
Hello, We have recently observed a problem with concurrent execution of ALTER ROLE SET... in several sessions. It's similar to the one from http://git.postgresql.org/gitweb?p=postgresql.git;a=commitdiff;h=fbcf4b92aa64d4577bcf25925b055316b978744a The result is the 'tuple concurrently updated' error message, and the problem is easily reproducible: CREATE SCHEMA test; CREATE SCHEMA test2; CREATE ROLE testrole; session 1: while [ 1 ]; do psql postgres -c 'alter role testrole set search_path=test2';done session 2: while [ 1 ]; do psql postgres -c 'alter role testrole set search_path=test';done The error message appears almost immediately on my system. After digging in the code I've found that a RowExclusiveLock is acquired on a pg_db_role_setting table in AlterSetting(). While the name of the locks suggests that it should conflict with itself, it doesn't. After I've replaced the lock in question with ShareUpdateExclusiveLock, the problem disappeared. Attached is the simple patch with these changes. Regards, -- Alexey Klyukin The PostgreSQL Company - Command Prompt, Inc. db_role_setting.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 'tuple concurrently updated' error for alter role ... set
On May 13, 2011, at 1:28 AM, Tom Lane wrote: Alexey Klyukin al...@commandprompt.com writes: After digging in the code I've found that a RowExclusiveLock is acquired on a pg_db_role_setting table in AlterSetting(). While the name of the locks suggests that it should conflict with itself, it doesn't. After I've replaced the lock in question with ShareUpdateExclusiveLock, the problem disappeared. Attached is the simple patch with these changes. We're not likely to do that, first because it's randomly different from the handling of every other system catalog update, and second because it would serialize all updates on this catalog, and probably create deadlock cases that don't exist now. (BTW, as the patch is given I'd expect it to still fail, though perhaps with lower probability than before. For this to actually stop all such cases, you'd have to hold the lock till commit, which greatly increases the risks of deadlock.) Fair enough. I think the AlterSetting holds the lock till commit (it does heap_close with NoLock). The DropSetting doesn't do this though. I see no particular reason why conflicting updates like those *shouldn't* be expected to fail occasionally. Excellent question, I don't have enough context to properly answer that (other than a guess that an unexpected transaction rollback is too unexpected :)) Let me ask the customer first. -- Alexey Klyukin The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Apr 1, 2011, at 12:08 AM, Alexey Klyukin wrote: Hi Selena, On Mar 30, 2011, at 11:42 PM, Selena Deckelmann wrote: Hi! On Wed, Mar 30, 2011 at 8:40 AM, Alexey Klyukin al...@commandprompt.com wrote: I did a little bit of work on this, and we discussed it here: http://archives.postgresql.org/pgsql-hackers/2009-03/msg00345.php http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php Probably there's a bit of bitrot in there. Cool, I was not aware of your work in this direction. I've updated your patch to apply to the latest HEAD, implementing Tom Lane's suggestions (attached). I think I'll implement the other part (reporting all invalid parameters, as opposed to only the first one) tomorrow. Here's the update of Selena's patch, which also shows all errors in configuration parameters (as well as parser errors) during reload. When I talked to Alvaro the other day he suggested starting with a stand-alone process, which would load the postgresql.conf in a postmaster context, load other configuration files and do some of the checks I've mentioned in my initial proposal (particularly it would check that system's shared memory limit is high enough by trying to allocate a new shared memory segment). Afterwards, I'd like to implement checks from a user-callable function, and not all checks would be available from it. -- Alexey Klyukin The PostgreSQL Company - Command Prompt, Inc. guc-file.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Hi Selena, On Mar 30, 2011, at 11:42 PM, Selena Deckelmann wrote: Hi! On Wed, Mar 30, 2011 at 8:40 AM, Alexey Klyukin al...@commandprompt.com wrote: I did a little bit of work on this, and we discussed it here: http://archives.postgresql.org/pgsql-hackers/2009-03/msg00345.php http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php Probably there's a bit of bitrot in there. Cool, I was not aware of your work in this direction. I've updated your patch to apply to the latest HEAD, implementing Tom Lane's suggestions (attached). I think I'll implement the other part (reporting all invalid parameters, as opposed to only the first one) tomorrow. The development plan consists of 2 parts. The first one is to add new code that would allow running the checks in both a stand-alone process, when postmaster is not running, and as a function call from a backend postgres process. Initially the code would simply loads configuration files, without performing any of the validation checks. The second part consists of adding specific checks. Cool! Mine was only going to work if the system started up or was reloaded. Well, I think a stand-alone check is an easy part :) As I said above, some of what you've suggested seems more like a non-postgres core thing.. maybe an extension? Or maybe offer the option to read Well, initially I'm going to start with just a check that configuration files are valid, and add other checks afterwards. I think it makes sense for them to be optional. My idea was to just check that settings were *valid* not that they met some other, more subjective criteria. Well, my definition of valid configuration is not only the one that server is able to parse and load, but also to actually apply (i.e. can bind to a listen_address or read SSL certificate files). I agree that's not always necessary (i.e. when checking configuration on a different server than the one it should be applied to), so we can add a flag to turn them off. -- Alexey Klyukin The PostgreSQL Company - Command Prompt, Inc. parser_continue_on_errors.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] proposal: a validator for configuration files
(success, failure, warnings), and an exact error message would be available in the server's log. It's possible to address these shortcomings in the future. Ideas, suggestions are welcome. -- Alexey Klyukin The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] arrays as pl/perl input arguments [PATCH]
On Feb 12, 2011, at 9:53 AM, Alex Hunsaker wrote: On Fri, Feb 11, 2011 at 17:17, Alexey Klyukin al...@commandprompt.com wrote: Anyway in playing with this patch a bit more I found another bug return [[]]; would segfault. So find attached a v9 that: - fixes above segfault - made plperl_sv_to_literal vastly simpler (no longer uses SPI and calls regtypin directly) - removed extraneous /para added in v8 in plperl.sgml (my docbook stuff is broken, so I can't build them, hopefully there are no other problems) - we now on the fly (when its requested) make the backwards compatible string for arrays (it was a few lines now that we have plperl_sv_to_literal) - make plperl.o depend on plperl_helpers.h (should have been done in the utf8 patch) Anyway barring any more bugs, I think this patch is ready. pg_to_perl_arrays_v9.patch.gz Thank you for finding and fixing the bugs there. I think removing the para was a mistake. I specifically added it to fix the problem I had when building the docs: openjade:plperl.sgml:353:8:E: end tag for PARA omitted, but OMITTAG NO was specified openjade:plperl.sgml:201:2: start tag was here After I re-added the closing /para in plperl.sgml:235 these errors disappeared, and the resulting html looks fine too. v10 with just this single change is attached. /A -- Alexey Klyukin The PostgreSQL Company - Command Prompt, Inc. pg_to_perl_arrays_v10.patch.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] arrays as pl/perl input arguments [PATCH]
On Feb 15, 2011, at 7:45 PM, David E. Wheeler wrote: On Feb 15, 2011, at 6:39 AM, Alexey Klyukin wrote: After I re-added the closing /para in plperl.sgml:235 these errors disappeared, and the resulting html looks fine too. v10 with just this single change is attached. So is this ready for committer? Yes. -- Alexey Klyukin The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] arrays as pl/perl input arguments [PATCH]
On Feb 10, 2011, at 11:26 PM, Alexey Klyukin wrote:
On Feb 10, 2011, at 9:44 PM, Andrew Dunstan wrote:
On 02/10/2011 08:15 AM, Alexey Klyukin wrote:
Let me try implementing that as an XS interface to plperl_array_to_datum.
Are you intending this as a completion of the current patch or as 9.2 work?
If the former you need to send it in real fast.
I'd like to extend the current patch, going to post the update by tomorrow.
So, here is the v8. Instead of rewriting the encode_array_literal I've added
another function, encode_type_literal (could use a better name). basically a
user-level interface to plperl_sv_to_datum, which accepts the perl variable
and the type name as a text string and returns the string representation of
the input variable according to the output function of the argument type, e..g:
do $$ elog(NOTICE, encode_type_literal([[1],[2],[3]], 'int[]'));$$ language
plperl;
NOTICE: {{1},{2},{3}}
I can easily convert the encode_array_literal to just call this function, but
not encode_array_constructor, since the latter needs to produce its own string
representation, different from the result of the type output function. One
option would be traversing through the SV *, duplicating the efforts of
plperl_sv_to_datum, but I actually wonder what do we need the
encode_array_constructor (and encode_array_literal) functions for ? I
guess they were useful to pass array references to SPI, but per my
understanding it's possible to use perl array references in SPI functions
directly now, so are there any other use cases for these functions, which
justify the time to spend on updating them to support arrays of composites
(and shouldn't we also provide encode_composite_literal and
encode_composite_constructor as well) ?
/A
--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.
pg_to_perl_arrays_v8.patch.gz
Description: GNU Zip compressed data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] arrays as pl/perl input arguments [PATCH]
On Feb 9, 2011, at 9:28 PM, Alex Hunsaker wrote: On Wed, Feb 9, 2011 at 08:24, Alexey Klyukin al...@commandprompt.com wrote: What was actually broken in encode_array_literal support of composite types (it converted perl hashes to the literal composite-type constants, expanding nested arrays along the way) ? I think it would be a useful extension of the existing encode_array_literal. Yeah, It does not work because it did not take into account the order of composite columns. It always put them alphabetically by column name. To do it properly we would need to pass in a typid or a column order or something. Ideally we could expose the new plperl_array_to_datum() to plperl functions in some manner. Damn, right. Each perl hash corresponds to multiple composite types, different by the order of the type elements. Passing the typid sounds like a fair requirement (and if it's missing we could assume that the order of columns in composites doesn't matter to the caller). Let me try implementing that as an XS interface to plperl_array_to_datum. /A -- Alexey Klyukin The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] arrays as pl/perl input arguments [PATCH]
On Feb 10, 2011, at 9:44 PM, Andrew Dunstan wrote: On 02/10/2011 08:15 AM, Alexey Klyukin wrote: On Feb 9, 2011, at 9:28 PM, Alex Hunsaker wrote: On Wed, Feb 9, 2011 at 08:24, Alexey Klyukinal...@commandprompt.com wrote: What was actually broken in encode_array_literal support of composite types (it converted perl hashes to the literal composite-type constants, expanding nested arrays along the way) ? I think it would be a useful extension of the existing encode_array_literal. Yeah, It does not work because it did not take into account the order of composite columns. It always put them alphabetically by column name. To do it properly we would need to pass in a typid or a column order or something. Ideally we could expose the new plperl_array_to_datum() to plperl functions in some manner. Damn, right. Each perl hash corresponds to multiple composite types, different by the order of the type elements. Passing the typid sounds like a fair requirement (and if it's missing we could assume that the order of columns in composites doesn't matter to the caller). Let me try implementing that as an XS interface to plperl_array_to_datum. Are you intending this as a completion of the current patch or as 9.2 work? If the former you need to send it in real fast. I'd like to extend the current patch, going to post the update by tomorrow. /A -- Alexey Klyukin The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] arrays as pl/perl input arguments [PATCH]
On Feb 9, 2011, at 3:44 AM, Alex Hunsaker wrote: So the merge while not exactly trivial was fairly simple. However it would be great if you could give it another look over. Find attached v7 changes include: - rebased against HEAD - fix potential use of uninitialized dims[cur_depth] - took out accidental (broken) hack to try and support composite types in ::encode_array_literal (added in v4 or something) - make_array_ref() now uses plperl_hash_from_datum() for composite types instead of its own hand rolled version - get_perl_array_ref() now grabs the 'array' directly instead of through the magic interface for simplicity - moved added static declarations to the bottom instead of being half on top and half on bottom pg_to_perl_arrays_v7.patch.gz Thank you very much, the new patch applies cleanly and passes all tests on my system. The new get_perl_array_ref seems to be much more clear to me, than the prev. magic call. What was actually broken in encode_array_literal support of composite types (it converted perl hashes to the literal composite-type constants, expanding nested arrays along the way) ? I think it would be a useful extension of the existing encode_array_literal. /A -- Alexey Klyukin The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] arrays as pl/perl input arguments [PATCH]
On Feb 6, 2011, at 9:43 AM, Alex Hunsaker wrote: On Thu, Feb 3, 2011 at 18:29, Alex Hunsaker bada...@gmail.com wrote: On Mon, Jan 31, 2011 at 01:34, Alexey Klyukin al...@commandprompt.com wrote: I've looked at the patch and added a test for arrays exceeding or equal maximum dimensions to check, whether the recursive function won't bring surprises there. I've also added check_stack_depth calls to both split_array and plperl_hash_from_tuple. Note that the regression fails currently due to the incorrect error reporting in PostgreSQL, per http://archives.postgresql.org/pgsql-hackers/2011-01/msg02888.php. The v5 is attached. One thing I find odd is we allow for nested arrays, but not nested composites. For example: ... On the other end, the same is true when returning. If you try to return a nested composite or array, the child better be a string as it wont let you pass a hash: So here is a v6 that does the above. It does so by providing a generic plperl_sv_to_datum() function and converting the various places to use it. One cool thing is you can now use the composite types or arrays passed in (or returned from another spi!) as arguments for spi_exec_prepared(). Before you would have to convert the hashes to strings. It also provides a real plperl_convert_to_pg_array (now plperl_array_to_datum) that can handle composite and other random datatypes. This also means we don't have to convert arrays to a string representation first. (which removes part of the argument for #5 @ http://archives.postgresql.org/pgsql-hackers/2011-02/msg00197.php) I have attached a stand alone version of the above so its easier to look over. The diffstat is fairly small (ignoring added regression tests) 1 files changed, 259 insertions(+), 165 deletions(-) Comments? Thanks, looks great to me. It passes all the tests on my OS X system. I wonder what's the purpose of the amagic_call in get_perl_array_ref, instead of calling newRV_noinc on the target SV * ? Also, in array_to_datum (line 1050), if get_perl_array_ref returns NULL for the first element of the corresponding dimension, but non-NULL for the second one - it would use uninitialized dims[cur_depth] value in comparison (which, although, would probably lead to the desired comparison result). /A -- Alexey Klyukin The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] arrays as pl/perl input arguments [PATCH]
Hi,
On Feb 2, 2011, at 7:16 PM, Tim Bunce wrote:
I'm sorry I'm late to this party. I haven't been keeping up with
pgsql-hackers.
Better late than never :)
I'd kind'a hoped that this functionality could be tied into extending
PL/Perl to handle named arguments. That way the perl variables
corresponding to the named arguments could be given references without
breaking any code.
Franky I don't see a direct connection between conversion of arrays into array
references and supporting named arguments. Could you, please, show some
examples of how you hoped the functionality could be extended?
Some observations on the current code (based on a quick skim):
- I'd like to see the conversion function exposed as a builtin
$ref = decode_array_literal({...});
In principal, I think that's not hard to built with the functionality provided
by this patch. I see this as an XS function which takes the array string,
calls the array_in to convert it to the array datum, and, finally, calls
plperl_ref_from_pg_array (provided by the patch) to produce the resulting
array reference.
- Every existing plperl function that takes arrays is going to get
slower due to the overhead of parsing the string and allocating the
array and all its elements.
Well, per my understanding of Alex changes, the string parsing is not invoked
unless requested by referencing the array in a string context. Normally, onle
plperl_ref_from_pg_array will be invoked every time the function is called
with an array argument, which would take little time to convert the PostgreSQL
internal array representation (not a sting) to the perl references, but that's
no different from what is already done with composite type arguments, which
are converted to perl hash references on every corresponding function call.
- Some of those functions may not use the array at all and some may
simply pass it on as an argument to another function.
I don't see how it would be good to optimize for the functions that are
declared to get the array but in fact do nothing with it. And considering the
case of passing an array through to another function, it's currently not
possible to call another pl/perl function from an existing one directly, and
when passing muti-dimensional arrays to a perl function one would need to
convert it to the array references anyway.
- Making the conversion lazy would be a big help.
Converting it to string is already lazy, and, per the argumens above, I don't
see a real benefit of lazy conversion to the perl reference (as comparing to
the hurdles of implementing that).
/A
--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] off-by-one mistake in array code error reporting
Hi,
While working on PL/Perl patch for arrays as input arguments I've noticed that
PostgreSQL reports one less dimension in the 'number of array dimensions (%d)
exceeds the maximum allowed (%d), i.e.
select
'{{{1,2},{3,4}},{{5,6},{7,8}}},{{{9,10},{11,12}},{{13,14},{15,16,
17,18},{19,20}},{{21,22},{23,24}}},{{{25,26},{27,28}},{{29,30},{31,32},
{1,2},{3,4}},{{5,6},{7,8}}},{{{9,10},{11,12}},{{13,14},{15,16,
17,18},{19,20}},{{21,22},{23,24}}},{{{25,26},{27,28}},{{29,30},{31,32}},
{{1,2},{3,4}},{{5,6},{7,8}}},{{{9,10},{11,12}},{{13,14},{15,16,
17,18},{19,20}},{{21,22},{23,24}}},{{{25,26},{27,28}},{{29,30},{31,32},
{1,2},{3,4}},{{5,6},{7,8}}},{{{9,10},{11,12}},{{13,14},{15,16,
17,18},{19,20}},{{21,22},{23,24}}},{{{25,26},{27,28}},{{29,30},{31,32}}}'
::int[];
ERROR: number of array dimensions (6) exceeds the maximum allowed (6)
Attached is the simple fix for that.
/A
--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.
array_error_msg_fix.diff
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] arrays as pl/perl input arguments [PATCH]
On Jan 29, 2011, at 12:27 AM, Alex Hunsaker wrote: On Thu, Jan 27, 2011 at 03:38, Alexey Klyukin al...@commandprompt.com wrote: Hi, On Jan 27, 2011, at 9:31 AM, Alex Hunsaker wrote: Find attached v3 of the patch. changes include: - fix deep recursion due to accidental reversal of check in encode_array_literal - add proper support for stringifying composite/row types. I did not find a good way to quote these from the perl on the fly, so instead we compute it the same way we used to and store the string inside the new object along with the array :(. - misc whitespace and code touchups pg_to_perl_arrays_v3.patch.gz Nice improvement. It passes all the regression tests on my OS X system. I have only a minor suggestion, I think is_array is worth mentioning in the utility functions chapter of the pl/perl documentation, it would be also more clear to use it in regression tests as opposed to manually checking whether the ref is equal to 'PostgreSQL::InServer::ARRAY'. Wait a second... Just who is reviewing who's patch? :P Oh, sorry, got confused along the way :) Both done in the attached. I also renamed is_array() to is_array_ref() for clarity (hopefully). pg_to_perl_arrays_v4.patch.gz Thank you. I've looked at the patch and added a test for arrays exceeding or equal maximum dimensions to check, whether the recursive function won't bring surprises there. I've also added check_stack_depth calls to both split_array and plperl_hash_from_tuple. Note that the regression fails currently due to the incorrect error reporting in PostgreSQL, per http://archives.postgresql.org/pgsql-hackers/2011-01/msg02888.php. The v5 is attached. pg_to_perl_arrays_v5.patch.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] arrays as pl/perl input arguments [PATCH]
Hi, On Jan 27, 2011, at 9:31 AM, Alex Hunsaker wrote: Find attached v3 of the patch. changes include: - fix deep recursion due to accidental reversal of check in encode_array_literal - add proper support for stringifying composite/row types. I did not find a good way to quote these from the perl on the fly, so instead we compute it the same way we used to and store the string inside the new object along with the array :(. - misc whitespace and code touchups pg_to_perl_arrays_v3.patch.gz Nice improvement. It passes all the regression tests on my OS X system. I have only a minor suggestion, I think is_array is worth mentioning in the utility functions chapter of the pl/perl documentation, it would be also more clear to use it in regression tests as opposed to manually checking whether the ref is equal to 'PostgreSQL::InServer::ARRAY'. /A -- Alexey Klyukin The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] arrays as pl/perl input arguments [PATCH]
Hi, On Jan 26, 2011, at 8:45 PM, Alex Hunsaker wrote: On Sat, Jan 15, 2011 at 15:48, Alex Hunsaker bada...@gmail.com wrote: On Wed, Jan 12, 2011 at 13:04, Alexey Klyukin al...@commandprompt.com wrote: On Jan 12, 2011, at 8:52 PM, David E. Wheeler wrote: On Jan 12, 2011, at 5:14 AM, Alexey Klyukin wrote: You mean packing both a string representation and a reference to a single SV * value? Dunno, I'm not a guts guy. Well, neither me (I haven't used much of the guts api there). Find attached a proof of concept that modifies Alexey's patch to do the above (using the overload example I and others posted). [ ... ] Thoughts? Should I polish this a bit more? Or do we like the GUC better? So its been over a week with no comments. ISTM there were more people against adding yet another GUC. Barring objection ill finish the missing parts of the POC patch I posted and submit that. I've played with that patch just today. I found a problem with it, when I tried to use the array in a string context the backend segfaulted with: WARNING: Deep recursion on subroutine main::encode_array_literal at -e line 74 just before the segfault. I think the problem is in the regexp check in 'encode_array_literal' (it's obviously reversed comparing with the original one), but it still segfaults after I fixed that. Other than that, the approach looks good to me, I'm for eliminating the GUC setting in favor of it. /A -- Alexey Klyukin The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] arrays as pl/perl input arguments [PATCH]
On Jan 26, 2011, at 10:08 PM, Alex Hunsaker wrote: On Wed, Jan 26, 2011 at 12:44, Alexey Klyukin al...@commandprompt.com wrote: Hi, On Jan 26, 2011, at 8:45 PM, Alex Hunsaker wrote: On Sat, Jan 15, 2011 at 15:48, Alex Hunsaker bada...@gmail.com wrote: On Wed, Jan 12, 2011 at 13:04, Alexey Klyukin al...@commandprompt.com wrote: On Jan 12, 2011, at 8:52 PM, David E. Wheeler wrote: On Jan 12, 2011, at 5:14 AM, Alexey Klyukin wrote: You mean packing both a string representation and a reference to a single SV * value? Dunno, I'm not a guts guy. Well, neither me (I haven't used much of the guts api there). Find attached a proof of concept that modifies Alexey's patch to do the above (using the overload example I and others posted). [ ... ] Thoughts? Should I polish this a bit more? Or do we like the GUC better? So its been over a week with no comments. ISTM there were more people against adding yet another GUC. Barring objection ill finish the missing parts of the POC patch I posted and submit that. I've played with that patch just today. I found a problem with it, when I tried to use the array in a string context the backend segfaulted with: WARNING: Deep recursion on subroutine main::encode_array_literal at -e line 74 just before the segfault. I think the problem is in the regexp check in 'encode_array_literal' (it's obviously reversed comparing with the original one), Yeah, I noticed that after I sent it out :(. but it still segfaults after I fixed that. I seem to recall fixing this post email as well... Can you provide the function that broke so I can double check? (Or was it part of the regression test?) Sure, it's really simple (and the plperl_array regressions tests exposes this problem as well): CREATE OR REPLACE FUNCTION test_array(INTEGER[]) RETURNS TEXT AS $$ my $array = shift; print $array.\n; $$ LANGUAGE plperl; /A I will look into this problem tomorrow unless you'll be lucky to fix it before that. Thank you for the review and your patch. -- Alexey Klyukin The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] arrays as pl/perl input arguments [PATCH]
On Jan 12, 2011, at 1:07 AM, David E. Wheeler wrote: On Jan 11, 2011, at 2:25 PM, Alexey Klyukin wrote: Hello, Here's the patch that improves handling of arrays as pl/perl function input arguments, converting postgres arrays of arbitrary dimensions into perl array references. Awesome! I've wanted this for *years*. It includes regression tests and a documentation changes, and it builds and runs successfully on my mac os x and linux boxes. To maintain compatibility with existing pl/perl code a new variable, plperl.convert_array_arguments (better name?), is introduced. Its default value is false, when set to true it triggers the new behavior, i.e. Have you considered instead passing an array-based object with is string overloading designed to return the pg array string format? That would make for nice, transparent compatibility without the need for a GUC. You mean packing both a string representation and a reference to a single SV * value? I haven't considered that (lack of extensive perlgus-foo) although I think that's an interesting idea. One drawback would be that it would require both conversion to a string format and to a perl reference, performing unnecessary actions during every time arrays are passed to a pl/perl function. If there is a strong dislike of the proposed 'compatibility' GUC option - I think I can change the existing patch to incorporate array string representation into the reference-holding SV quite easily. /A -- Alexey Klyukin The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] arrays as pl/perl input arguments [PATCH]
On Jan 12, 2011, at 4:06 AM, Robert Haas wrote: On Tue, Jan 11, 2011 at 7:55 PM, Andrew Dunstan and...@dunslane.net wrote: On 01/11/2011 07:17 PM, David E. Wheeler wrote: On Jan 11, 2011, at 3:44 PM, Andrew Dunstan wrote: I think there's at least a danger of breaking legacy code doing that. Say you have some code that does a ref test on the argument, for example. The behavior would now be changed. I think that'd be pretty rare. Possibly it would. But we usually try pretty hard to avoid that sort of breakage. By the same token, I'm not convinced it's a good idea for this behavior to be off by default. Surely many people will altogether fail to notice that it's an option? If we're going to have a backward-compatibility GUC at all, ISTM that you ought to get the good stuff unless you ask for the old way. I think the number of people failing to notice the changes would be the same whenever we set the new or the old behavior by default. I decided to default to the the old behavior since it won't break the existing code as opposed to just hiding the good stuff, although it would slower the adoption of the new behavior. /A -- Alexey Klyukin The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] arrays as pl/perl input arguments [PATCH]
On Jan 12, 2011, at 8:52 PM, David E. Wheeler wrote: On Jan 12, 2011, at 5:14 AM, Alexey Klyukin wrote: You mean packing both a string representation and a reference to a single SV * value? Dunno, I'm not a guts guy. Well, neither me (I haven't used much of the guts api there). I haven't considered that (lack of extensive perlgus-foo) although I think that's an interesting idea. One drawback would be that it would require both conversion to a string format and to a perl reference, performing unnecessary actions during every time arrays are passed to a pl/perl function. If there is a strong dislike of the proposed 'compatibility' GUC option - I think I can change the existing patch to incorporate array string representation into the reference-holding SV quite easily. Andrew's objections have merit. So perhaps just add this patch to the commit fest as is? Already done :) /A -- Alexey Klyukin The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] arrays as pl/perl input arguments [PATCH]
On Jan 12, 2011, at 9:36 PM, Alvaro Herrera wrote: Excerpts from Alex Hunsaker's message of mié ene 12 16:22:55 -0300 2011: [ Id actually vote for _not_ having a compatibility option at all, we change more major things than this IMHO every major release. (And even then some major things in minor releases, for example the removal of Safe.pm) ] I think the main question here is: how loudly is existing code going to break? If the breakage is silent, it's going to be very problematic. If functions fail to run at all, then we can live without the compatibility option. Not really loud. Perl won't even complain when you try to interpret a reference as a string. Since almost everyone votes for making the new behavior a default option I'm inclined to do that change, although I'm against throwing out the compatibility option. There are many other reasons except for PL/Perl for people to upgrade to 9.1, let's not force them to rewrite their Perl code if they were not planning to do that. /A -- Alexey Klyukin The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] arrays as pl/perl input arguments [PATCH]
Hello,
Here's the patch that improves handling of arrays as pl/perl function input
arguments, converting postgres arrays of arbitrary dimensions into perl array
references. It includes regression tests and a documentation changes, and it
builds and runs successfully on my mac os x and linux boxes. To maintain
compatibility with existing pl/perl code a new variable,
plperl.convert_array_arguments (better name?), is introduced. Its default
value is false, when set to true it triggers the new behavior, i.e.
SET plperl.convert_array_arguments = true;
CREATE OR REPLACE FUNCTION test_array(text[]) RETURNS TEXT AS $$
my $arg = shift;
if (ref $arg eq 'ARRAY') {
return array conversion is enabled;
} else {
return array conversion is disabled;
}
$$ LANGUAGE plperl;
test=# select test_array('{1,2,3}');
test_array
-
array conversion is enabled
(1 row)
You can find other, less trivial examples, by examining plperl_array
regression test.
The implementation detects whether the input argument of a perl function is an
array, and calls plperl_ref_from_pg_array if it is. The latter obtains a flat
array of Datums from deconstruct_array and, using information about array
dimensions, recursively creates perl array references in split_array. To pass
array information between recursive calls a new 'plperl_array_info' struct was
added. Arrays as members of composite types are also handled in
plperl_hash_from_tuple.
/A
--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.
pg_to_perl_arrays.diff
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optimize PL/Perl function argument passing [PATCH]
On Dec 9, 2010, at 7:32 PM, Tim Bunce wrote:
On Wed, Dec 08, 2010 at 09:21:05AM -0800, David E. Wheeler wrote:
On Dec 8, 2010, at 9:14 AM, Tim Bunce wrote:
Do you have any more improvements in the pipeline?
I'd like to add $arrayref = decode_array_literal('{2,3}') and
maybe $hashref = decode_hstore_literal('x=1, y=2').
I don't know how much works would be involved in those though.
Those would be handy, but for arrays, at least, I'd rather have a GUC
to turn on so that arrays are passed to PL/perl functions as array
references.
Understood. At this stage I don't know what the issues are so I'm
nervous of over promising (plus I don't know how much time I'll have).
It's possible a blessed ref with string overloading would avoid
backwards compatibility issues.
I used to work on a patch that converts postgres arrays into perl array
references:
http://archives.postgresql.org/pgsql-hackers/2009-11/msg01552.php
I have a newer patch, which is, however, limited to one-dimensional resulting
arrays. If there's an interest in that approach I can update it for the
current code base, add support multi-dimensional arrays (I used to implement
that, but lost the changes accidentally) and post it for review.
/A
--
Alexey Klyukin http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Another proposal for table synonyms
On Dec 3, 2010, at 2:17 AM, Alvaro Herrera wrote: Excerpts from Robert Haas's message of jue dic 02 21:10:48 -0300 2010: On Thu, Dec 2, 2010 at 3:43 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Robert Haas's message of jue dic 02 17:27:01 -0300 2010: Yeah, the Oracle system is a lot more complex than SQL Server's, but I was only talking about the latter, for which see here: http://www.databasejournal.com/features/mssql/article.php/3635426/SYNONYM-in-SQL-Server-2005.htm Well, that seems primarily designed to cut down on three and four part names. We don't have that problem anyway. Right. (My point here is that SQL Server is not a good guidance on what the synonym system should do.) The list of objects for which they support synonyms is also interesting. The bit that allows a synonym to reference another synonym seems like worth considering further (either reject them altogether, or have some way to deal with possible cycles). It's pretty trivial to do cycle-detection at runtime. No disagreement on that, but something needs to be decided. I don't think it makes sense to allow synonyms for synonyms. It would make resolution code slower, and I don't see any situation where they make sense. The original proposal didn't mention them, but limited the list of initially supported objects to those to tables, views and sequences, implicitly excluding synonyms referring to another synonyms. -- Alexey Klyukin http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Another proposal for table synonyms
On Nov 30, 2010, at 10:05 PM, Josh Berkus wrote: Alexey, Here is the proposal to add synonyms to PostgreSQL. Initial goal is to add synonyms for relations (tables, views, sequences) and an infrastructure to allow synonyms for other database objects in the future. Can you explain, for our benefit, the use case for this? Specifically, what can be done with synonyms which can't be done with search_path and VIEWs? Well, porting applications from other database systems that support synonyms (i.e. Oracle, DB2, SQL Server). I ask partly because I've migrated some Oracle databases to PostgreSQL, and did not find replacing the functionality of synonyms to be at all difficult. Presumably you've run into a case which was difficult? Frankly, I don't have a specific use case, but there were some requests in this list asking for synonyms, and adding support for them is a TODO item in wiki. BTW, I have a specific use case for *column* synonyms which isn't currently covered by our existing tools. Is this the feature the community would benefit from? We can consider adding column synonyms if we won't hardwire synonyms to pg_class objects. -- Alexey Klyukin http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Another proposal for table synonyms
Hello,
Here is the proposal to add synonyms to PostgreSQL. Initial goal is to add
synonyms
for relations (tables, views, sequences) and an infrastructure to allow synonyms
for other database objects in the future.
A thread with discussion of an old proposal by Jonah Harris is here:
http://archives.postgresql.org/pgsql-hackers/2006-03/msg00519.php
-
Synonyms are database objects, which act as an alias for other objects. In
this proposal the synonyms for tables, views and sequences will be considered.
The new command, CREATE SYNONYM, defines a synonym. The syntax is:
CREATE SYNONYM synonym_name FOR object_type object_name
where
synonym_name: fully-qualified name (FQN) of the synonym
object_type: {TABLE | VIEW | SEQUENCE}. In the future, new object_types, such as
functions, can be added.
object_name: FQN of a database table, view or sequence.
Another new command, DROP SYNONYM, is used for deleting an already existing
synonym without removing the object the synonym references. The syntax is:
DROP SYNONYM synonym_name
where synonym_name is a FQN of a synonym.
Comments will be supported on synonyms with the following command:
COMMENT ON SYNONYM synonym_name IS comment_text
where synonym_name is a FQN of a synonym.
To support addition of new database objects types that can be referenced by
synonyms a new system catalog, pg_synonym, is to be added, with an oid to
support comments on synonym, and the following schema:
synname name name of the synonym
synnamespace oid OID of the namespace that contains the synonym
synclassid oid OID of the system catalog that contains the referenced object
synobjid oid OID of the referenced object
When resolving the synonym name, the usual search_path lookup rules apply,
i.e. first, the object of the appropriate type is looked into the schema, then
the synonym, afterwards the process iterates with the next schema from the
search_path. Note that the table synonym with the same FQN as an existing
table will be masked by that table.
To speedup the synonym name resolution a new syscache, SYNNAMENSPCLASS
{synname, synnamespace, synclassid} will be introduced. This cache will be
accessed if the query to the RELNAMENSP syscache will return no result, with
the DB object's catalog OID set to pg_class OID.
For table and view synonyms, INSERT/UPDATE/DELETE/SELECT will be supported.
For sequences SELECT will be supported. The commands will translate synonyms
to the referenced database objects on the parser stage.
All types of synonyms will be supported as table arguments/return value types,
as well as actual values (i.e. currval/nextval will accept a sequence
synonym).
The following DDL will work transparently with table synonyms (sequences and
views if the corresponding command applies to them):
COPY, LOCK, TRUNCATE, EXPLAIN, EXPLAIN ANALYZE.
The following DDL commands will cause an error when called for tables
(sequences, views) synonyms:
ALTER {TABLE|VIEW|SEQUENCE},
ANALYZE,
CLUSTER,
COMMENT ON {TABLE | VIEW | SEQUENCE} .. IS,
DROP {TABLE | VIEW | SEQUENCE},
GRANT,
REVOKE,
VACUUM.
For these commands additional checks for synonyms will be introduced on a
per-command basis.
A dependency of the referenced object on a synonym will be added when adding a
new synonym to forbid removing a referenced object without removing the
synonym first (without using CASCADE). On DROP SYNONYM the related dependency
will be removed.
--
Alexey Klyukin http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Another proposal for table synonyms
On Nov 30, 2010, at 6:28 PM, Tom Lane wrote: Alexey Klyukin al...@commandprompt.com writes: To support addition of new database objects types that can be referenced by synonyms a new system catalog, pg_synonym, is to be added, with an oid to support comments on synonym, and the following schema: This is not going to work, at least not without making every type of lookup consult pg_synonym too, which I think can be considered DOA because of its performance impact on people who aren't even using the feature. For those not using synonyms it would result in an extra syscache lookup for each schema from the search_path that doesn't contain the table with the specified name. If the table is specified with A FQN or contained in the first schema from the search_path no extra lookup would occur. Is it considered a big impact? The number of such lookups can be reduced if we traverse the search_path for the tables first, and then look for the synonyms, although that would change the lookup rules stated in this proposal It's also quite unclear how you prevent duplicate names if the synonyms are in their own catalog. (And no, the part of your proposal that says you're not preventing that isn't acceptable from a usability standpoint.) What's wrong with the usability of that feature? The fact that the table with the same FQN as a synonym masks the latter can be clearly stated in the documentation. Are you expecting lots of people to name the synonym exactly the same as one of the database tables and wonder why is the table and not the synonym gets accessed? As an alternative, a warning during table/synonym creation/renaming can be emitted if the name clash occurs. You could reasonably support synonyms for tables/views by storing them in pg_class with a new relkind. This doesn't cover synonyms for other object types, but since the total world demand for such a feature is approximately zero, that's not really a problem. I think that would almost guarantee that synonyms for other kinds of objects (i.e. databases, such kind of synonyms were requested in the past) would never be added. -- Alexey Klyukin http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] ps buffer is incorrectly padded on the (latest) OS X
Hi, I always wondered why ps ax|grep postgres shows several extra blank lines after the process name, i.e. 972 ?? Ss 0:00.69 postgres: writer process 973 ?? Ss 0:00.51 postgres: wal writer process (I put newlines instead of spaces there). By looking into the code I've found this part of set_ps_display: #ifdef PS_USE_CLOBBER_ARGV /* pad unused memory; need only clobber remainder of old status string */ if (last_status_len ps_buffer_cur_len) MemSet(ps_buffer + ps_buffer_cur_len, PS_PADDING, last_status_len - ps_buffer_cur_len); last_status_len = ps_buffer_cur_len; #endif /* PS_USE_CLOBBER_ARGV */ PS_PADDING padding on __darwin__ is set to ' '. Apparently this doesn't work correctly with OS X 10.6. After I changed the define to use '\0' on darwin extra blank likes (actually consisting of hundreds of spaces without a line break) disappeared. The one-liner change follows: === diff --git a/src/backend/utils/misc/ps_status.c b/src/backend/utils/misc/ps_status.c index f27a52f..c2ddf33 100644 --- a/src/backend/utils/misc/ps_status.c +++ b/src/backend/utils/misc/ps_status.c @@ -76,7 +76,7 @@ bool update_process_title = true; /* Different systems want the buffer padded differently */ -#if defined(_AIX) || defined(__linux__) || defined(__svr4__) +#if defined(_AIX) || defined(__linux__) || defined(__svr4__) || defined(__darwin__) #define PS_PADDING '\0' #else #define PS_PADDING ' ' === I don't have different OS X versions to test, so I'm not sure whether 10.5 or below are also affected. Also, the patch should specifically check for 10.6, though I don't know how to distinguish between different OS X versions in postgres sources (any suggestions?). Regards, -- Alexey Klyukin http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Specification for Trusted PLs?
On Fri, May 21, 2010 at 7:25 PM, Magnus Hagander mag...@hagander.net wrote: On Fri, May 21, 2010 at 12:22 PM, David Fetter da...@fetter.org wrote: On Fri, May 21, 2010 at 11:57:33AM -0400, Magnus Hagander wrote: On Fri, May 21, 2010 at 11:55 AM, Josh Berkus j...@agliodbs.com wrote: So, here's a working definition: 1) cannot directly read or write files on the server. 2) cannot bind network ports To make that more covering, don't yu really need something like cannot communicate with outside processes? These need to be testable conditions, and new tests need to get added any time we find that we've missed something. Making this concept fuzzier is exactly the wrong direction to go. Well, the best way to define what a trusted language can do is to define a *whitelist* of what it can do, not a blacklist of what it can't do. That's the only way to get a complete definition. It's then up to the implementation step to figure out how to represent that in the form of tests. Yes, PL/Perl is following this approach. For a whitelist see plperl_opmask.h (generated by plperl_opmask.pl at build phase). -- Alexey Klyukin www.CommandPrompt.com The PostgreSQL Company - Command Prompt, Inc -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] missing file in git repo
I think postgres git repo is broken. The compilation of REL7_4_STABLE from git fails on my system with: make -C src all make -C port all make[2]: *** No rule to make target `sprompt.o', needed by `libpgport.a'. Stop. There is no sprompt.c in src/port in the sources obtained from git. However, there is one in CVS: http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/port/sprompt.c?only_with_tag=REL7_4 This looks like the initial synchronization issue, since this file is there for really long time and appears not to be touched by any commit since 2003. -- Alexey Klyukin http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] inlining SQL functions
Hi, Is there a reason why only a table free SQL functions are allowed to be inlined ? I wonder why a simple SQL function containing only a SELECT * FROM table can't be expanded inline ? Is there an interest in expanding the class of SQL functions that can be inlined ? Thanks, -- Alexey Klyukin http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] a common place for pl/perlu modules
Hello, When developing pl/perlu functions common definitions and methods are often stored in external .pm modules. During deployment the modules should be installed somewhere in @INC to be reachable by the perl interpreter. However, installing the modules to a location outside of the PG installation makes it hard to have a consistent environment when running multiple PG versions on the same host. What do you think about defining a canonical place for pl/perlu .pm modules (i.e. PKGLIBDIR) and adding this location to @INC during the interpreter initialization ? Another idea is to allow a user to specify such location by adding a new custom GUC variable. -- Alexey Klyukin http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] a common place for pl/perlu modules
On Feb 11, 2010, at 6:24 PM, Andrew Dunstan wrote: Alexey Klyukin wrote: Hello, When developing pl/perlu functions common definitions and methods are often stored in external .pm modules. During deployment the modules should be installed somewhere in @INC to be reachable by the perl interpreter. However, installing the modules to a location outside of the PG installation makes it hard to have a consistent environment when running multiple PG versions on the same host. What do you think about defining a canonical place for pl/perlu .pm modules (i.e. PKGLIBDIR) and adding this location to @INC during the interpreter initialization ? Another idea is to allow a user to specify such location by adding a new custom GUC variable. Why won't setting this in the new on_perl_init setting work? It's even included in to documented examples using the standard lib module: http://developer.postgresql.org/pgdocs/postgres/plperl-under-the-hood.html#PLPERL-CONFIG The lack of support for SPI functions makes this hardly an adequate solution. I do have both modules and SPI calls in several pl/perlu functions. -- Alexey Klyukin http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] a common place for pl/perlu modules
On Feb 11, 2010, at 7:07 PM, Andrew Dunstan wrote: Alexey Klyukin wrote: On Feb 11, 2010, at 6:24 PM, Andrew Dunstan wrote: Alexey Klyukin wrote: Hello, When developing pl/perlu functions common definitions and methods are often stored in external .pm modules. During deployment the modules should be installed somewhere in @INC to be reachable by the perl interpreter. However, installing the modules to a location outside of the PG installation makes it hard to have a consistent environment when running multiple PG versions on the same host. What do you think about defining a canonical place for pl/perlu .pm modules (i.e. PKGLIBDIR) and adding this location to @INC during the interpreter initialization ? Another idea is to allow a user to specify such location by adding a new custom GUC variable. Why won't setting this in the new on_perl_init setting work? It's even included in to documented examples using the standard lib module: http://developer.postgresql.org/pgdocs/postgres/plperl-under-the-hood.html#PLPERL-CONFIG The lack of support for SPI functions makes this hardly an adequate solution. I do have both modules and SPI calls in several pl/perlu functions. That has nothing to do with what you asked about, namely setting the include path. You can set the include path in on_perl_init with use lib and then use your modules in your plperlu functions, at which point SPI will be available. Ah, it seems I misinterpreted the documentation. This is much better, but still it requires setting the path explicitly. What about having a default location for the modules that is automatically added to @INC and is recommended in the documentation as a place to put .pm files ? -- Alexey Klyukin http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] plperl db access documentation enhancement
Hello,
We were asked by Enova Financial to improve the documentation of PL/Perl
database access functions.
Alvaro and me worked on that and we produced the patch that is attached. It
splits initial block of functions
into the groups with the description directly following each of the group,
corrects couple of mistakes and
adds an example.
One of the existing mistakes was confusion in definitions of spi_exec_prepared
and spi_query_prepared.
Another one is usage of INTEGER type to return the result of spi_prepare in the
example for prepared queries.
When trying to execute that function I've got the following error:
postgres=# CREATE OR REPLACE FUNCTION init() RETURNS INTEGER AS $#
$_SHARED{my_plan} = spi_prepare( 'SELECT (now() + $1)::date AS now',
'INTERVAL');
$$ LANGUAGE plperl;
CREATE FUNCTION
postgres=# select init();
ERROR: invalid input syntax for integer: 0x1007d6f40
CONTEXT: PL/Perl function init
Since the return value is not used anyway, I've changed the return type of the
function declaration in the example to VOID.
I think this is a good reason to suggest backpatching these changes down to
8.2.
plperl_db.diff
Description: Binary data
--
Alexey Klyukin http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] warn in plperl logs as... NOTICE??
On Jan 22, 2010, at 2:55 AM, Andrew Dunstan wrote: Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: Jim Nasby wrote: Why does warn; in plperl log as NOTICE in Postgres? Where would you like the warning to go? This has been this way for nearly 5 years, it's not new (and before that the warning didn't go anywhere). I think he's suggesting that it ought to translate as elog(WARNING) not elog(NOTICE). *shrug* I don't have a strong opinion about it, and it's pretty easy to change, if there's a consensus we should. I have certainly found over the years that perl warnings from some modules can be annoyingly verbose, which is probably why the original patch didn't make them have a higher level in Postgres. If this were a big issue we'd have surely heard about it before now - there are plenty of plperl users out there. I think elog(WARNING) is less surprising for the end-user, unless there's an objection strong enough to include it into the documentation :) -- Alexey Klyukin http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] warn in plperl logs as... NOTICE??
On Jan 22, 2010, at 4:38 PM, Robert Haas wrote: On Fri, Jan 22, 2010 at 9:13 AM, Alexey Klyukin al...@waki.ru wrote: I think elog(WARNING) is less surprising for the end-user, unless there's an objection strong enough to include it into the documentation :) I think the main possible objection would what Simon just wrote on the other thread - that it's been this way for a while, and while someone might think that a different decision about how to handle it would have been better, there may be people counting on the current behavior who will have to spend time and perhaps money making changes if we change it. Well, then we have to choose between a fixed number of unhappy users in the past and potentially increasing number of unhappy users in the future (if we admit the fact that this behavior is illogical). IMO if something behaves counterintuitively to most users the behavior should be at least documented, if not fixed. -- Alexey Klyukin http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] plperl and inline functions -- first draft
On Nov 29, 2009, at 4:40 AM, Tom Lane wrote: Alexey Klyukin al...@commandprompt.com writes: Isn't it also the case with the existing plperl code ? I've noticed that free(prodesc) is called when it's no longer used (i.e. in plperl_compile_callback:1636), but refcount of desc-reference is never decremented. I've been experimenting with this and confirmed that there is a leak; not only in the DO patch but in the pre-existing code, if a plperl function is redefined repeatedly. Is this the correct way to release the SV* reference? if (reference) SvREFCNT_dec(reference); Yes. In fact this only decreases the reference count, making the interpreter free the memory referred to when it becomes 0, but since prodesc-reference has refcount of 1 this would do the right thing. -- Alexey Klyukin http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] arrays as input parameters in plperl
Hi, I'd like to improve the way PL/Perl handles arrays as function input parameters. In PL/Perl arrays are passed as plain text strings, and getting a value of an array element requires additional perl code to parse that string. I'd like to make this easier by converting each postgresq array passed as an input parameter to the reference to the corresponding perl array. The patch in attachment illustrates this. it's limited to one-dimensional array output. The list of upcoming improvements is: - convert n-dimensional array to the perl equivalent (a reference to a list of references) - proper support for arrays of composite types - compatibility with existing plperl functions by introducing a new custom variable, i.e. plperl.pass_array_refs that triggers the new behavior. I think it should be disabled by default. - documentation and additional tests The patch adds a new attribute to the plperl_proc_desc struct, that records whether Nth argument is an array. The function plperl_ref_from_pg_array does the actual job of converting array input parameter to the perl array reference. I considered writing a perl routine instead of a C function, although I though it would be less readable, more complex and slower due to double conversion of input. The disadvantage of a C function is a code duplication with array_out, on which my function is based, although it can be avoided by putting a relevant part of array_out into a separate function. The patch is attached. Anybody interested in this feature ? Ideas, improvements, suggestions ? Regards, -- Alexey Klyukin http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc plperl_array.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] plperl and inline functions -- first draft
On Nov 20, 2009, at 2:04 AM, Joshua Tolley wrote: On Wed, Nov 18, 2009 at 12:38:00PM +0200, Alexey Klyukin wrote: Yes, current_call_data can't be allocate in the SPI memory context, since it's used to extract the result after SPI_finish is called, although it doesn't lead to problems here since no result is returned. Anyway, I'd move SPI_connect after the current_call_data initialization. I also noticed that no error context is set in the inline handler, not sure whether it really useful except for the sake of consistency, but in case it is - here is the patch: Makes sense on both counts. Thanks for the help. How does the attached look? These two problems seem to be fixed now, thank you. -- Alexey Klyukin http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] plperl and inline functions -- first draft
On Nov 20, 2009, at 3:50 PM, Tim Bunce wrote: When does the reference held by desc.reference get freed? At the moment it looks like this would leak memory for each DO. Isn't it also the case with the existing plperl code ? I've noticed that free(prodesc) is called when it's no longer used (i.e. in plperl_compile_callback:1636), but refcount of desc-reference is never decremented. -- Alexey Klyukin http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] plperl and inline functions -- first draft
On Nov 18, 2009, at 5:46 AM, Andrew Dunstan wrote: Joshua Tolley wrote: +plperl_call_data *save_call_data = current_call_data; +boololdcontext = trusted_context; + + if (SPI_connect() != SPI_OK_CONNECT) +elog(ERROR, could not connect to SPI manager); ... +current_call_data = (plperl_call_data *) palloc0(sizeof(plperl_call_data)); +current_call_data-fcinfo = fake_fcinfo; +current_call_data-prodesc = desc; I don't think this is done in the right order. If it is then this comment in plperl_func_handler is wrong (as well as containing a typo): /* * Create the call_data beforing connecting to SPI, so that it is not * allocated in the SPI memory context */ Yes, current_call_data can't be allocate in the SPI memory context, since it's used to extract the result after SPI_finish is called, although it doesn't lead to problems here since no result is returned. Anyway, I'd move SPI_connect after the current_call_data initialization. I also noticed that no error context is set in the inline handler, not sure whether it really useful except for the sake of consistency, but in case it is - here is the patch: inline_callback.diff Description: Binary data -- Alexey Klyukin http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] plperl and inline functions -- first draft
On Nov 9, 2009, at 6:07 PM, Joshua Tolley wrote: Ok, updated patch attached. As far as I know, this completes all outstanding issues: 1) weird comment in plperl.c is corrected and formatted decently 2) plperlu vs. plperl actually works (thanks again, Andrew) 3) docs included 4) regression tests included Some items of note include that this makes the regression tests add not only plperl to the test database but also plperlu, which is a new thing. I can't see why this might cause problems, but thought I'd mention it. The tests specifically try to verify that plperl doesn't allow 'use Data::Dumper', and plperlu does. Since Data::Dumper is part of perl core, that seemed safe, but it is another dependency, and perhaps we don't want to do that. If not, is there some other useful way of testing plperlu vs. plperl, and does it really matter? I've noticed that the patch doesn't install current_call_data before calling plperl_call_perl_func, although it saves and restores its previous value. This breaks spi code, which relies on current_call_data-prodesc, i.e.: postgres=# DO $$ $result = spi_exec_query(select 1); $$ LANGUAGE plperl; server closed the connection unexpectedly This probably means the server terminated abnormally before or while processing the request. The connection to the server was lost. Attempting reset: Failed. rogram received signal EXC_BAD_ACCESS, Could not access memory. Reason: KERN_INVALID_ADDRESS at address: 0x 0x0001006f0336 in plperl_spi_exec (query=0x1007ecb60 select 1, limit=0) at plperl.c:1895 warning: Source file is more recent than executable. 1895spi_rv = SPI_execute(query, current_call_data-prodesc-fn_readonly, (gdb) bt #0 0x0001006f0336 in plperl_spi_exec (query=0x1007ecb60 select 1, limit=0) at plperl.c:1895 Also, a call to to plperl_call_perl_func should be cast to void to avoid a possible compiler warning (although It doesn't emit one on my system): (void) plperl_call_perl_func(desc, fake_fcinfo); -- Alexey Klyukin http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PL/Perl backed crashed during spi_exec_query
On Oct 31, 2009, at 7:30 PM, Tom Lane wrote:
Alexey Klyukin al...@commandprompt.com writes:
One of our customers is running 8.2.14 and use a couple of pl/perl
and
pl/perlu functions written by CMD. Everything worked normally until
they tried to call one particular pl/perl function from pl/perl via
spi. It appears that a die call inside the callee just crashes the
backend.
I think the critical point is actually that you're calling plperl from
plperlu, and we're being careless about restoring the former
interpreter
selection on error exit. The attached patch moves the responsibility
for that into plperl_call_handler, which already has a suitable
PG_TRY block.
The patch solves the problem, thank you!
regards, tom lane
Index: plperl.c
===
RCS file: /cvsroot/pgsql/src/pl/plperl/plperl.c,v
retrieving revision 1.152
diff -c -r1.152 plperl.c
*** plperl.c28 Sep 2009 17:31:12 - 1.152
--- plperl.c31 Oct 2009 17:27:14 -
***
*** 380,390
}
}
!
static void
restore_context(bool old_context)
{
! if (trusted_context != old_context)
{
if (old_context)
PERL_SET_CONTEXT(plperl_trusted_interp);
--- 380,392
}
}
! /*
! * Restore previous interpreter selection, if two are active
! */
static void
restore_context(bool old_context)
{
! if (interp_state == INTERP_BOTH trusted_context != old_context)
{
if (old_context)
PERL_SET_CONTEXT(plperl_trusted_interp);
***
*** 870,878
plperl_call_handler(PG_FUNCTION_ARGS)
{
Datum retval;
! plperl_call_data *save_call_data;
- save_call_data = current_call_data;
PG_TRY();
{
if (CALLED_AS_TRIGGER(fcinfo))
--- 872,880
plperl_call_handler(PG_FUNCTION_ARGS)
{
Datum retval;
! plperl_call_data *save_call_data = current_call_data;
! boololdcontext = trusted_context;
PG_TRY();
{
if (CALLED_AS_TRIGGER(fcinfo))
***
*** 883,893
--- 885,897
PG_CATCH();
{
current_call_data = save_call_data;
+ restore_context(oldcontext);
PG_RE_THROW();
}
PG_END_TRY();
current_call_data = save_call_data;
+ restore_context(oldcontext);
return retval;
}
***
*** 1226,1232
Datum retval;
ReturnSetInfo *rsi;
SV *array_ret = NULL;
- boololdcontext = trusted_context;
ErrorContextCallback pl_error_context;
/*
--- 1230,1235
***
*** 1376,1384
if (array_ret == NULL)
SvREFCNT_dec(perlret);
- current_call_data = NULL;
- restore_context(oldcontext);
-
return retval;
}
--- 1379,1384
***
*** 1391,1397
Datum retval;
SV *svTD;
HV *hvTD;
- boololdcontext = trusted_context;
ErrorContextCallback pl_error_context;
/*
--- 1391,1396
***
*** 1491,1498
if (perlret)
SvREFCNT_dec(perlret);
- current_call_data = NULL;
- restore_context(oldcontext);
return retval;
}
--- 1490,1495
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] PL/Perl backed crashed during spi_exec_query
Hi,
One of our customers is running 8.2.14 and use a couple of pl/perl and
pl/perlu functions written by CMD. Everything worked normally until
they tried to call one particular pl/perl function from pl/perl via
spi. It appears that a die call inside the callee just crashes the
backend. Here is the simple example:
CREATE OR REPLACE FUNCTION caller() RETURNS VOID AS $$
my $res = spi_exec_query('select callee()');
$$ LANGUAGE plperlu;
CREATE OR REPLACE FUNCTION callee() RETURNS VOID AS $$
die callee died;
$$ LANGUAGE plperl;
On my system (8.2.14 running OS X 10.6.1, perl 5.8.9 installed from
macports, relevant flags from perl -V: usethreads=define
use5005threads=undef useithreads=define usemultiplicity=define ):
postgres=# select callee();
ERROR: error from Perl function: callee died at line 2.
postgres=# select caller();
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
and in the server log:
ERROR: error from Perl function: callee died at line 2.
STATEMENT: select callee();
error from Perl function: callee died at line 2..
LOG: server process (PID 36132) exited with exit code 255
LOG: terminating any other active server processes
FATAL: the database system is in recovery mode
LOG: all server processes terminated; reinitializing
Here is gdb output with a backtrace of the process just before it
exits. It seems to terminate on croak call. The edata structure seems
to b valid, so I suspect there is something with the interpreter that
is executing croak.
Breakpoint 1, plperl_spi_exec (query=0x10053d650 select callee(),
limit=0) at plperl.c:1814
1814SPI_restore_connection();
(gdb) n
1817croak(%s, edata-message);
(gdb) p edata
$1 = (ErrorData *) 0x100949290
(gdb) p *edata
$2 = {
elevel = 20,
output_to_server = 1 '\001',
output_to_client = 1 '\001',
show_funcname = 0 '\0',
filename = 0x100684b08 plperl.c,
lineno = 1131,
funcname = 0x1006860af plperl_call_perl_func,
sqlerrcode = 2600,
message = 0x100949238 error from Perl function: callee died at
line 2.,
detail = 0x0,
hint = 0x0,
context = 0x100949328 SQL statement \select callee()\,
cursorpos = 0,
internalpos = 0,
internalquery = 0x0,
saved_errno = 0
}
(gdb) bt
#0 plperl_spi_exec (query=0x10053d650 select callee(), limit=0) at
plperl.c:1817
#1 0x0001006831c1 in XS__spi_exec_query (my_perl=value
temporarily unavailable, due to optimizations, cv=value temporarily
unavailable, due to optimizations) at SPI.xs:118
#2 0x0001007056d2 in Perl_pp_entersub ()
#3 0x0001006fdbba in Perl_runops_standard ()
#4 0x0001006f7c8d in Perl_call_sv ()
#5 0x00010067cec4 in plperl_call_perl_func (desc=0x10093f600,
fcinfo=0x7fff5fbfda20) at plperl.c:1110
#6 0x00010067fdc9 in plperl_func_handler [inlined] () at /private/
tmp/postgresql-8.2.14/src/pl/plperl/plperl.c:1240
#7 0x00010067fdc9 in plperl_call_handler (fcinfo=0x7fff5fbfda20)
at plperl.c:858
#8 0x0001000f8f0e in ExecMakeFunctionResult (fcache=0x1008ecab0,
econtext=0x1008ec980, isNull=0x1008f06a8 , isDone=0x1008f06c8) at
execQual.c:1340
#9 0x0001000f6c2f in ExecTargetList [inlined] () at /private/tmp/
postgresql-8.2.14/src/backend/executor/execQual.c:4190
#10 0x0001000f6c2f in ExecProject (projInfo=value temporarily
unavailable, due to optimizations, isDone=0x7fff5fbfde9c) at
execQual.c:4391
#11 0x00010010a2c3 in ExecResult (node=0x1008ec868) at
nodeResult.c:157
#12 0x0001000f63b2 in ExecProcNode (node=0x1008ec868) at
execProcnode.c:334
#13 0x0001000f51fc in ExecutePlan [inlined] () at /private/tmp/
postgresql-8.2.14/src/backend/executor/execMain.c:1172
#14 0x0001000f51fc in ExecutorRun (queryDesc=value temporarily
unavailable, due to optimizations, direction=ForwardScanDirection,
count=0) at execMain.c:244
#15 0x000100195099 in PortalRunSelect (portal=0x1008cc438,
forward=value temporarily unavailable, due to optimizations,
count=0, dest=0x1008b39b0) at pquery.c:831
#16 0x000100196d8f in PortalRun (portal=0x1008cc438,
count=9223372036854775807, dest=0x1008b39b0, altdest=0x1008b39b0,
completionTag=0x7fff5fbfe260 ) at pquery.c:656
#17 0x000100191a95 in exec_simple_query (query_string=0x1008b3238
select caller();) at postgres.c:957
#18 0x000100192c19 in PostgresMain (argc=4, argv=0x100836300,
username=0x10081bdf8 alexk) at postgres.c:3472
#19 0x000100164254 in ServerLoop () at postmaster.c:2934
#20 0x0001001650c9 in PostmasterMain (argc=3, argv=0x100500470) at
postmaster.c:966
#21 0x000100119f89 in main (argc=3, argv=0x100500470) at main.c:188
(gdb) s
Program exited with code 0377.
--
Alexey Klyukin http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt
[HACKERS] PL/Perl crash when using threaded perl
fullname = 0x84d248 /usr/local/pgsql/lib/plperl.so
lib_handle = value temporarily unavailable, due to optimizations
retval = (PGFunction) 0x12
__func__ = load_external_function
#19 0x000703b7 in fmgr_c_validator (fcinfo=0x852400) at pg_proc.c:509
funcoid = value temporarily unavailable, due to optimizations
libraryhandle = value temporarily unavailable, due to optimizations
tuple = (HeapTuple) 0x84d248
isnull = 0 '\0'
tmp = value temporarily unavailable, due to optimizations
prosrc = 0x84d090 plperl_call_handler
__func__ = fmgr_c_validator
#20 0x002692f9 in OidFunctionCall1 (functionId=8725504, arg1=8725504) at
fmgr.c:1533
flinfo = {
fn_addr = 0x702a0 fmgr_c_validator,
fn_oid = 2247,
fn_nargs = 1,
fn_strict = 1 '\001',
fn_retset = 0 '\0',
fn_extra = 0x0,
fn_mcxt = 0x80f7d8,
fn_expr = 0x0
}
fcinfo = {
flinfo = 0xbfffde38,
context = 0x0,
resultinfo = 0x0,
isnull = 0 '\0',
nargs = 1,
arg = {40960, 3221216440, 1693582, 54, 0, 0, 0, 8708240, 8442524, 8444804,
2545207, 8444812, 8444812, 20, 0, 8661112, 8442524, 8444804, 2545207, 8444812,
3221216652, 20, 1, 52335640, 8442508, 8444984, 2545207, 8444992, 8444992, 20,
1679523, 3221216464, 8442508, 8444984, 2545207, 8444992, 3221216716, 20,
543661892, 1273804351, 8440928, 20, 1683774, 2482979872, 8444812, 3221216552,
2621638, 8440856, 8440928, 20, 2621638, 2482979872, 8661888, 844,
3123422035, 1281850385, 2487039675, 8, 2436859, 7487432, 4952088, 3221216600,
2624137, 844, 7487432, 2558151799, 2, 0, 7487432, 3221216648, 2436930,
7487432, 48, 3221216648, 1683832, 8440856, 8444992, 7487224, 2437115, 7487432,
4952088, 3221216712, 2437344, 48, 0, 0, 0, 0, 0, 3358052, 0, 51977820, 8661272,
2437115, 2445025, 52335640, 3468288, 16768504, 16777472, 31},
argnull =
\000Å\030ð\005\032\000\033\031\000\000\000\000\000\020Þÿ¿8Þÿ¿|\031\000\005\000\000\000\001\000\000\000\020Þÿ¿L%\000t©\000\003\000\000\000\000\000\000\000Ók%\000d©\000ç\004\000\000ÐN%\000
¨\000\000\000\000\000$¨\000\033\031\000ùP%\000\000\000\000\000x
}
result = value temporarily unavailable, due to optimizations
__func__ = OidFunctionCall1
#21 0x0006fa59 in ProcedureCreate (procedureName=0x847740
plperl_call_handler, procNamespace=11, replace=0 '\0', returnsSet=0 '\0',
returnType=2280, languageObjectId=13, languageValidator=2247, prosrc=0x852400
\004È, probin=0x852400 \004È, isAgg=0 '\0', security_definer=0 '\0',
isStrict=0 '\0', volatility=118 'v', parameterTypes=0x847e40,
allParameterTypes=0, parameterModes=0, parameterNames=0, proconfig=0,
procost=1.22270354e-38, prorows=1.22270354e-38) at pg_proc.c:413
retval = 40960
parameterCount = 0
allParamCount = 0
allParams = (Oid *) 0x847e58
genericInParam = value temporarily unavailable, due to optimizations
genericOutParam = value temporarily unavailable, due to optimizations
internalInParam = 0 '\0'
internalOutParam = 0 '\0'
rel = (Relation) 0x4b8ed0
tup = (HeapTuple) 0xa000
oldtup = (HeapTuple) 0x0
nulls = ' ' repeats 14 times, nnn nn
values = {3221217068, 11, 10, 13, 8704032, 8704048, 0, 0, 0, 0, 118, 0,
2280, 8683072, 0, 0, 0, 8704064, 8704104, 0, 0}
replaces = 'r' repeats 21 times
relid = value temporarily unavailable, due to optimizations
procname = {
data = plperl_call_handler, '\0' repeats 44 times,
alignmentDummy = 1701866608
}
tupDesc = (TupleDesc) 0x4d20a0
is_update = 0 '\0'
myself = {
classId = 1255,
objectId = 40960,
objectSubId = 0
}
referenced = {
classId = 1247,
objectId = 2280,
objectSubId = 0
}
i = 0
__func__ = ProcedureCreate
#22 0x000cae74 in CreateProceduralLanguage (stmt=0x82e660) at proclang.c:130
funcname = (List *) 0x847650
languageName = 0x847640 plperl
pltemplate = (PLTemplate *) 0x847990
handlerOid = 0
valOid = value temporarily unavailable, due to optimizations
funcrettype = value temporarily unavailable, due to optimizations
funcargtypes = value temporarily unavailable, due to optimizations
__func__ = CreateProceduralLanguage
#23 0x001acca8 in MemoryContextSwitchTo [inlined] () at palloc.h:1173
__func__ = PortalRunUtility
#24 0x001acca8 in PortalRunUtility (portal=0x848618, utilityStmt=0x82e660,
isTopLevel=1 '\001', dest=0x852400, completionTag=0x852400 \004È) at
pquery.c:1181
context = (MemoryContext) Cannot access memory at address 0x0
Thanks,
--
Alexey Klyukin http://www.CommandPrompt.com
The PostgreSQL Company - Command Prompt, Inc.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PL/Perl crash when using threaded perl
On Mon, Aug 10, 2009 at 10:09 PM, Andrew Dunstan and...@dunslane.netwrote: I wonder if this is another case of the lack of perl library initialisation bug we have seen before. Can you try with this patch to the postgres 8.3 sources? http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/pl/plperl/plperl.c.diff?r1=1.136;r2=1.136.2.2 We haven't put out an 8.3 release that includes that patch yet. Thanks, Andrew, this patch solved the problem. -- Alexey Klyukin .commandprompt.com The PostgreSQL Company - Command Prompt, Inc
[HACKERS] errcontext support in PL/Perl
Hi, While trying to build a custom error reporting function for one of our clients we came across the fact that PL/Perl doesn't set errcontext that we relied on to get the traceback of running functions. Exactly the same problem with PL/Python was fixed recently by Peter Eisentraut (http://archives.postgresql.org/pgsql-committers/2009-07/msg00168.php). Attached is a patch (HEAD) that sets errcontext with PL/Perl function name, making a distinction between compilation and execution stages, fixes error messages where function name was already included in the message itself and updates regression tests. I'll appreciate any suggestions on how to improve it. plperl_error_callback.diff Description: Binary data -- Alexey Klyukin http://www.CommandPrompt.com The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] errcontext support in PL/Perl
On Jul 21, 2009, at 6:39 PM, Alvaro Herrera wrote:
Alexey Klyukin wrote:
Attached is a patch (HEAD) that sets errcontext with PL/Perl function
name, making a distinction between compilation and execution stages,
fixes error messages where function name was already included in the
message itself and updates regression tests. I'll appreciate any
suggestions on how to improve it.
Hmm, in plperl_exec_callback(), does the global variable work if you
call one plperl function from another?
PL/Perl functions can't call each other directly. I don't see any
problems with SPI calls:
test=# create function perl_log1() returns void language plperl as $$
test$# elog(NOTICE, Test from function one);
test$# $$
test-# ;
CREATE FUNCTION
test=# create function perl_log2() returns void language plperl as $
$
elog
(NOTICE, Test from function two);
my $rv = spi_exec_query('SELECT * FROM perl_log1()');
$$;
CREATE FUNCTION
test=# select perl_log2();
NOTICE: Test from function two
CONTEXT: PL/Perl function perl_log2
NOTICE: Test from function one
CONTEXT: PL/Perl function perl_log1
SQL statement SELECT * FROM perl_log1()
PL/Perl function perl_log1
perl_log2
---
(1 row)
--
Alexey Klyukin http://www.CommandPrompt.com
The PostgreSQL Company - Command Prompt, Inc.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] errcontext support in PL/Perl
On Jul 21, 2009, at 7:20 PM, Alvaro Herrera wrote: Alexey Klyukin wrote: NOTICE: Test from function one CONTEXT: PL/Perl function perl_log1 SQL statement SELECT * FROM perl_log1() PL/Perl function perl_log1 Shouldn't the second PL/Perl function line say perl_log2 instead? Hm, yeah, seems to be a problem. I'll change the callback to avoid using global data. -- Alexey Klyukin http://www.CommandPrompt.com The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] errcontext support in PL/Perl
On Jul 21, 2009, at 7:47 PM, Alexey Klyukin wrote: On Jul 21, 2009, at 7:20 PM, Alvaro Herrera wrote: Alexey Klyukin wrote: NOTICE: Test from function one CONTEXT: PL/Perl function perl_log1 SQL statement SELECT * FROM perl_log1() PL/Perl function perl_log1 Shouldn't the second PL/Perl function line say perl_log2 instead? Hm, yeah, seems to be a problem. I'll change the callback to avoid using global data. Attached is the updated version of the patch (the original description is here: http://archives.postgresql.org/pgsql-hackers/2009-07/msg01332.php) that doesn't use global variables. It also shows the last line of the context in the example above correctly: psql (8.5devel) Type help for help. test=# select perl_log2(); NOTICE: Test from function two CONTEXT: PL/Perl function perl_log2 NOTICE: Test from function one CONTEXT: PL/Perl function perl_log1 SQL statement SELECT * FROM perl_log1() PL/Perl function perl_log2 perl_log2 --- (1 row) plperl_error_callback_v2.diff Description: Binary data -- Alexey Klyukin http://www.CommandPrompt.com The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Postgres-R: internal messaging
Markus Wanner wrote: Besides the communication between the replication manager and the backends, which is currently done by using these imessages, the replication manager also needs to communicate with the postmaster: it needs to be able to request new helper backends and it wants to be notified upon termination (or crash) of such a helper backend (and other backends as well...). I'm currently doing this with imessages as well, which violates the rule that the postmaster may not to touch shared memory. I didn't look into ripping that out, yet. I'm not sure it can be done with the existing signaling of the postmaster. In Replicator we avoided the need for postmaster to read/write backend's shmem data by using it as a signal forwarder. When a backend wants to inform a special process (i.e. queue monitor) about replication-related event (such as commit) it sends SIGUSR1 to Postmaster with a related reason flag and the postmaster upon receiving this signal forwards it to the destination process. Termination of backends and special processes are handled by the postmaster itself. Let's have a simple example: consider a local transaction which changes some tuples. Those are being collected into a change set, which gets written to the shared memory area as an imessage for the replication manager. The backend then also signals the manager, which then awakes from its select(), checks its imessages queue and processes the message, delivering it to the GCS. It then removes the imessage from the shared memory area again. Hm...what would happen with the new data under heavy load when the queue would eventually be filled with messages, the relevant transactions would be aborted or they would wait for the manager to release the queue space occupied by already processed messages? ISTM that having a fixed size buffer limits the maximum transaction rate. My initial design features only a single doubly linked list as the message queue, holding all messages for all processes. An imessages lock blocks concurrent writing acces. That's still what's in there, but I realize that's not enough. Each process should better have its own queue, and the single lock needs to vanish to avoid contention on that lock. However, that would require dynamically allocatable shared memory... What about keeping the per-process message queue in the local memory of the process, and exporting only the queue head to the shmem, thus having only one message per-process there. When the queue manager gets a message from the process it may signal that process to copy the next message from the process local memory into the shmem. To keep a correct ordering of queue messages an additional shared memory queue of pid_t can be maintained, containing one pid per each message. -- Alexey Klyukin http://www.commandprompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Permanent settings
Aidan Van Dyk wrote: * Magnus Hagander [EMAIL PROTECTED] [080220 14:03]: I think the first step is really for some people to show code that rewrites the config file changing a setting reliably and correctly. But what we're donig now is discussing *how to do that*, no? Sort of, but of course, we're getting caught up in extra syntactic stuff.. If someone *is* writing this config-rewriter now, these are the types of quesitons I think they need to be asking, some of which have been touched on, some not. But I think a first cut could pick any answer for them, and still be easily adaptable... 1) What file to we rewrite? Main one, or some other specified one? I think the file that is the source for the option that we write. If a parser can determine what is the last occurence of the option in the configuration files - this can be done as well for SET PERMANENT. 2) Do we follow includes to find our setting? Yes and no. We have to follow includes, otherwise the effect of remote changes to the option won't be the same as the effect of manual changes via the text editor. However, following the previous proposals we can store a file that is the source of the option, i.e. the file that sets the value for the option that is active for the time of SET PERMANENT. 3) Which setting do we change, the 1st, or last found in the config file? I think that is active, presumably last. 4) What do we do about comments *on the same line* as the setting we're changing (I'm assuming all other lines won't be touched) Just drop them. Another idea is to comment the old value, preserving the old inline comments, and put a new value to the next line, but I don't really like it since eventually it would make the config file unreadable with lots of 'dead' commented values. Anyway, we can't parse that comments unless we teach the backend to speak several hundreds of available world languages, that is not what the database development about :). 5) How do we want to handle errors like ENOSPC, or EPERM (I'm assuming of course that the file rewrite will be a tmp+rename, not a trunc+write) don't change anything, return ERROR. 6) Do we want to distinguish between restart only settings, and reloadable settings, and if so, how? I think now, since we don't digstinguish between them when writing the config file manually. -- Alexey Klyukin http://www.commandprompt.com/ The PostgreSQL Company - Command Prompt, Inc. ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [HACKERS] Debugger
PostgreSQL consists of multiple processes, launched by postmaster, make sure you are debugging the right one. After connecting to a database with your favorite client (i.e. psql) you can use ps (if you have a unix-like system) or a task manager to determine a pid of postgres process and then attach to that process with a debugger. Pedro Belmino wrote: Hellow, I continue to have problems using the debugger, can not achieve any breakpoint that is outside the postmaster.c? Can anyone give me any hint? Where do I mark a breakpoint so that I can follow the creation of an index to file index.c for example? Thank, -- Pedro Belmino. # Ciência da Computação - UNIFOR # [EMAIL PROTECTED] -- Alexey Klyukin http://www.commandprompt.com/ The PostgreSQL Company - Command Prompt, Inc. ---(end of broadcast)--- TIP 6: explain analyze is your friend
