Re: [HACKERS] pgcrypto compilation error due to stack-allocated EVP_CIPHER_CTX
It make sense. I would like to share more comments as following i.e.
static int
> bf_check_supported_key_len(void)
> {
> ...
> /* encrypt with 448bits key and verify output */
> evp_ctx = EVP_CIPHER_CTX_new();
> if (!evp_ctx)
> return 1;
> if (!EVP_EncryptInit_ex(evp_ctx, EVP_bf_ecb(), NULL, NULL, NULL))
> goto leave;
> if (!EVP_CIPHER_CTX_set_key_length(evp_ctx, 56))
> goto leave;
> if (!EVP_EncryptInit_ex(evp_ctx, NULL, NULL, key, NULL))
> goto leave;
> if (!EVP_EncryptUpdate(evp_ctx, out, &outlen, data, 8))
> goto leave;
> if (memcmp(out, res, 8) != 0)
> goto leave;/* Output does not match ->
> strong cipher is
> * not supported */
> status = 1;
> leave:
> EVP_CIPHER_CTX_free(evp_ctx);
> return status;
> }
It seems that it need to return 0 instead of 1 in case of failure i.e.
/* encrypt with 448bits key and verify output */
> evp_ctx = EVP_CIPHER_CTX_new();
> if (!evp_ctx)
> return 0;
We can avoid multiple if conditions and goto statement something like i.e.
if (EVP_EncryptInit_ex(evp_ctx, EVP_bf_ecb(), NULL, NULL, NULL) &&
> EVP_CIPHER_CTX_set_key_length(evp_ctx, 56) &&
> EVP_EncryptInit_ex(evp_ctx, NULL, NULL, key, NULL) &&
> EVP_EncryptUpdate(evp_ctx, out, &outlen, data, 8) &&
> memcmp(out, res, 8) == 0 )) /* Output does not match -> strong
> cipher is not supported */
> status = 1;
> EVP_CIPHER_CTX_free(evp_ctx);
> return status;
> }
What is your opinion ?. I am hopeful I will be able to share all my
findings tomorrow. Thanks.
On Wed, Dec 7, 2016 at 2:23 AM, Michael Paquier
wrote:
> On Tue, Dec 6, 2016 at 11:42 PM, Asif Naeem wrote:
> > Thanks for updated patch. Although EVP_CIPHER_CTX_cleanup() seems
> deprecated
> > in OpenSSL >= 1.1.0 i.e.
> >
> >> # if OPENSSL_API_COMPAT < 0x1010L
> >> # define EVP_CIPHER_CTX_init(c) EVP_CIPHER_CTX_reset(c)
> >> # define EVP_CIPHER_CTX_cleanup(c) EVP_CIPHER_CTX_reset(c)
> >> # endif
> >
> >
> > I guess use of deprecated function is fine, until OpenSSL library support
> > it.
>
> We could use some ifdef block with the OpenSSL version number, but I
> am not sure if that's worth complicating the code at this stage.
> --
> Michael
>
Re: [HACKERS] pgcrypto compilation error due to stack-allocated EVP_CIPHER_CTX
Thanks for updated patch. Although EVP_CIPHER_CTX_cleanup() seems deprecated in OpenSSL >= 1.1.0 i.e. # if OPENSSL_API_COMPAT < 0x1010L > # define EVP_CIPHER_CTX_init(c) EVP_CIPHER_CTX_reset(c) > # define EVP_CIPHER_CTX_cleanup(c) EVP_CIPHER_CTX_reset(c) > # endif I guess use of deprecated function is fine, until OpenSSL library support it. On Tue, Dec 6, 2016 at 6:15 PM, Michael Paquier wrote: > On Tue, Dec 6, 2016 at 9:31 PM, Asif Naeem wrote: > > Thank you for v2 patch, I would like to comment on it. It seems that you > > have used function EVP_CIPHER_CTX_reset in the patch that was introduced > in > > OpenSSL 1.1.0, older library version might not work now, is it > intentional > > change ?. > > I thought I tested that... But yes, that would not compile when linked > with 1.0.2 or older. Using EVP_CIPHER_CTX_cleanup() is safe instead as > that's available down to 0.9.8. > -- > Michael >
Re: [HACKERS] pgcrypto compilation error due to stack-allocated EVP_CIPHER_CTX
Hi Michael,
Thank you for v2 patch, I would like to comment on it. It seems that you
have used function EVP_CIPHER_CTX_reset in the patch that was introduced in
OpenSSL 1.1.0, older library version might not work now, is it intentional
change ?.
Regards,
Muhammad Asif Naeem
On Tue, Dec 6, 2016 at 12:15 PM, Michael Paquier
wrote:
> On Mon, Dec 5, 2016 at 6:09 PM, Michael Paquier
> wrote:
> > On Mon, Dec 5, 2016 at 5:11 PM, Heikki Linnakangas
> wrote:
> >> I'm afraid if we just start using EVP_CIPHER_CTX_new(), we'll leak the
> >> context on any error. We had exactly the same problem with
> EVP_MD_CTX_init
> >> being removed, in the patch that added OpenSSL 1.1.0 support. We'll
> have to
> >> use a resource owner to track it, just like we did with EVP_MD_CTX in
> commit
> >> 593d4e47. Want to do that, or should I?
> >
> > I'll send a patch within 24 hours.
>
> And within the delay, attached is the promised patch.
>
> While testing with a linked list, I have found out that the linked
> list could leak with cases like that, where decryption and encryption
> are done in a single transaction;
> select pgp_sym_decrypt(pgp_sym_encrypt(repeat('x',65530),'1'),'1') =
> repeat('x',65530);
>
> What has taken me a bit of time was to figure out that this bit is
> needed to free correctly elements in the open list:
> @@ -219,6 +220,8 @@ encrypt_free(void *priv)
> {
> struct EncStat *st = priv;
>
> + if (st->ciph)
> + pgp_cfb_free(st->ciph);
> px_memset(st, 0, sizeof(*st));
> px_free(st);
> }
> This does not matter on back-branches as things get cleaned up once
> the transaction memory context gets freed.
> --
> Michael
>
>
> --
> 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] Truncating/vacuuming relations on full tablespaces
Thank you for useful suggestions. PFA patch, I have tried to cover all the
points mentioned.
Regards,
Muhammad Asif Naeem
On Wed, Apr 6, 2016 at 9:06 PM, Robert Haas wrote:
> On Wed, Apr 6, 2016 at 3:32 AM, Asif Naeem wrote:
> >> Oh, I see. I think it's probably not a good idea to skip truncating
> >> those maps, but perhaps the option could be defined as making no new
> >> entries, rather than ignoring them altogether, so that they never
> >> grow. It seems that both of those are defined in such a way that if
> >> page Y follows page X in the heap, the entries for page Y in the
> >> relation fork will follow the one for page X. So truncating them
> >> should be OK; it's just growing them that we need to avoid.
> >
> > Thank you Robert. PFA basic patch, it introduces EMERGENCY option to
> VACUUM
> > that forces to avoid extend any entries in the VM or FSM. It seems
> working
> > fine in simple test scenarios e.g.
> >
> >> postgres=# create table test1 as (select generate_series(1,10));
> >> SELECT 10
> >> postgres=# vacuum EMERGENCY test1;
> >> VACUUM
> >> postgres=# select pg_relation_filepath('test1');
> >> pg_relation_filepath
> >> --
> >> base/13250/16384
> >> (1 row)
> >> [asif@centos66 inst_96]$ find . | grep base/13250/16384
> >> ./data/base/13250/16384
> >> postgres=# vacuum test1;
> >> VACUUM
> >> [asif@centos66 inst_96]$ find . | grep base/13250/16384
> >> ./data/base/13250/16384
> >> ./data/base/13250/16384_fsm
> >> ./data/base/13250/16384_vm
> >
> >
> > Please do let me know if I missed something or more information is
> required.
> > Thanks.
>
> This is too late for 9.6 at this point and certainly requires
> discussion anyway, so please add it to the next CommitFest. But in
> the meantime, here are a few quick comments:
>
> You should only support EMERGENCY using the new-style, parenthesized
> options syntax. That way, you won't need to make EMERGENCY a keyword.
> We don't want to do that, and we especially don't want it to be
> partially reserved, as you have done here.
>
> Passing the EMERGENCY flag around in a global variable is probably not
> a good idea; use parameter lists. That's what they are for. Also,
> calling the variable that decides whether EMERGENCY was set
> Extend_VM_FSM is confusing.
>
> I see why you changed the calling convention for visibilitymap_pin()
> and RecordPageWithFreeSpace(), but that's awfully invasive. I wonder
> if there's a better way to do that, like maybe having vacuumlazy.c ask
> the VM and FSM for their length in pages and then not trying to use
> those functions for block numbers that are too large.
>
> Don't forget to update the documentation.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
VACUUM_EMERGENCY_Option_v2.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] Truncating/vacuuming relations on full tablespaces
On Wed, Apr 6, 2016 at 9:06 PM, Robert Haas wrote:
> On Wed, Apr 6, 2016 at 3:32 AM, Asif Naeem wrote:
> >> Oh, I see. I think it's probably not a good idea to skip truncating
> >> those maps, but perhaps the option could be defined as making no new
> >> entries, rather than ignoring them altogether, so that they never
> >> grow. It seems that both of those are defined in such a way that if
> >> page Y follows page X in the heap, the entries for page Y in the
> >> relation fork will follow the one for page X. So truncating them
> >> should be OK; it's just growing them that we need to avoid.
> >
> > Thank you Robert. PFA basic patch, it introduces EMERGENCY option to
> VACUUM
> > that forces to avoid extend any entries in the VM or FSM. It seems
> working
> > fine in simple test scenarios e.g.
> >
> >> postgres=# create table test1 as (select generate_series(1,10));
> >> SELECT 10
> >> postgres=# vacuum EMERGENCY test1;
> >> VACUUM
> >> postgres=# select pg_relation_filepath('test1');
> >> pg_relation_filepath
> >> --
> >> base/13250/16384
> >> (1 row)
> >> [asif@centos66 inst_96]$ find . | grep base/13250/16384
> >> ./data/base/13250/16384
> >> postgres=# vacuum test1;
> >> VACUUM
> >> [asif@centos66 inst_96]$ find . | grep base/13250/16384
> >> ./data/base/13250/16384
> >> ./data/base/13250/16384_fsm
> >> ./data/base/13250/16384_vm
> >
> >
> > Please do let me know if I missed something or more information is
> required.
> > Thanks.
>
> This is too late for 9.6 at this point and certainly requires
> discussion anyway, so please add it to the next CommitFest. But in
> the meantime, here are a few quick comments:
>
Sure. Sorry for delay caused.
> You should only support EMERGENCY using the new-style, parenthesized
> options syntax. That way, you won't need to make EMERGENCY a keyword.
> We don't want to do that, and we especially don't want it to be
> partially reserved, as you have done here.
>
Sure. I have removed EMERGENCY keyword, it made code lot small now i.e.
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
> index b9aeb31..89c4ee0 100644
> --- a/src/backend/parser/gram.y
> +++ b/src/backend/parser/gram.y
> @@ -9298,6 +9298,20 @@ vacuum_option_elem:
> | VERBOSE { $$ =
> VACOPT_VERBOSE; }
> | FREEZE{ $$ =
> VACOPT_FREEZE; }
> | FULL { $$ =
> VACOPT_FULL; }
> + | IDENT
> + {
> + /*
> +* We handle identifiers that
> aren't parser keywords with
> +* the following special-case
> codes.
> +*/
> + if (strcmp($1, "emergency") == 0)
> + $$ = VACOPT_EMERGENCY;
> + else
> + ereport(ERROR,
> +
> (errcode(ERRCODE_SYNTAX_ERROR),
> +
> errmsg("unrecognized vacuum option \"%s\"", $1),
> +
> parser_errposition(@1)));
> + }
> ;
>
> AnalyzeStmt:
Passing the EMERGENCY flag around in a global variable is probably not
> a good idea; use parameter lists. That's what they are for. Also,
> calling the variable that decides whether EMERGENCY was set
> Extend_VM_FSM is confusing.
>
Sure. I adopted this approach by find other similar cases in the same
source code file i.e.
src/backend/commands/vacuumlazy.c
> /* A few variables that don't seem worth passing around as parameters */
> static int elevel = -1;
> static TransactionId OldestXmin;
> static TransactionId FreezeLimit;
> static MultiXactId MultiXactCutoff;
> static BufferAccessStrategy vac_strategy;
Should I modify code to use parameter lists for these variables too ?
I see why you changed the calling convention for visibilitymap_pin()
> and RecordPageWithFreeSpace(), but that's awfully invasive. I wonder
> if there's a better way to do that, like maybe having vacuumlazy.c ask
> the VM and FSM for their length in pages and then not trying to use
> those functions for block numbers that are too large.
>
> Don't forget to update the documentation.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
Re: [HACKERS] Truncating/vacuuming relations on full tablespaces
On Thu, Apr 7, 2016 at 2:15 AM, Jim Nasby wrote: > On 4/6/16 11:06 AM, Robert Haas wrote: > >> This is too late for 9.6 at this point and certainly requires >> discussion anyway, so please add it to the next CommitFest. >> > > If the goal here is to free up space via truncation when there's a real > emergency, perhaps there's some other steps that should be taken as well. > What immediately comes to mind is scanning the heap backwards and stopping > on the first page we can't truncate. > > There might be some other non-critical things we could skip in emergency > mode, with an eye towards making it as fast as possible. > > BTW, if someone really wanted to put some effort into this, it would be > possible to better handle filling up a single filesystem that has both data > and WAL by slowly shrinking the VM/FSM to make room in the WAL for vacuum > records. ISTM that's a much more common problem people run into than > filling up a separate tablespace. Thank you Jim. I will look into it and share my findings about it. > -- > Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX > Experts in Analytics, Data Architecture and PostgreSQL > Data in Trouble? Get it in Treble! http://BlueTreble.com >
Re: [HACKERS] Truncating/vacuuming relations on full tablespaces
Thank you Robert. Sure, I will add the updated patch on the next CommitFest
with all the suggested changes.
On Wed, Apr 6, 2016 at 9:06 PM, Robert Haas wrote:
> On Wed, Apr 6, 2016 at 3:32 AM, Asif Naeem wrote:
> >> Oh, I see. I think it's probably not a good idea to skip truncating
> >> those maps, but perhaps the option could be defined as making no new
> >> entries, rather than ignoring them altogether, so that they never
> >> grow. It seems that both of those are defined in such a way that if
> >> page Y follows page X in the heap, the entries for page Y in the
> >> relation fork will follow the one for page X. So truncating them
> >> should be OK; it's just growing them that we need to avoid.
> >
> > Thank you Robert. PFA basic patch, it introduces EMERGENCY option to
> VACUUM
> > that forces to avoid extend any entries in the VM or FSM. It seems
> working
> > fine in simple test scenarios e.g.
> >
> >> postgres=# create table test1 as (select generate_series(1,10));
> >> SELECT 10
> >> postgres=# vacuum EMERGENCY test1;
> >> VACUUM
> >> postgres=# select pg_relation_filepath('test1');
> >> pg_relation_filepath
> >> --
> >> base/13250/16384
> >> (1 row)
> >> [asif@centos66 inst_96]$ find . | grep base/13250/16384
> >> ./data/base/13250/16384
> >> postgres=# vacuum test1;
> >> VACUUM
> >> [asif@centos66 inst_96]$ find . | grep base/13250/16384
> >> ./data/base/13250/16384
> >> ./data/base/13250/16384_fsm
> >> ./data/base/13250/16384_vm
> >
> >
> > Please do let me know if I missed something or more information is
> required.
> > Thanks.
>
> This is too late for 9.6 at this point and certainly requires
> discussion anyway, so please add it to the next CommitFest. But in
> the meantime, here are a few quick comments:
>
> You should only support EMERGENCY using the new-style, parenthesized
> options syntax. That way, you won't need to make EMERGENCY a keyword.
> We don't want to do that, and we especially don't want it to be
> partially reserved, as you have done here.
>
> Passing the EMERGENCY flag around in a global variable is probably not
> a good idea; use parameter lists. That's what they are for. Also,
> calling the variable that decides whether EMERGENCY was set
> Extend_VM_FSM is confusing.
>
> I see why you changed the calling convention for visibilitymap_pin()
> and RecordPageWithFreeSpace(), but that's awfully invasive. I wonder
> if there's a better way to do that, like maybe having vacuumlazy.c ask
> the VM and FSM for their length in pages and then not trying to use
> those functions for block numbers that are too large.
>
> Don't forget to update the documentation.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
Re: [HACKERS] Truncating/vacuuming relations on full tablespaces
On Tue, Jan 19, 2016 at 2:04 AM, Robert Haas wrote:
> On Mon, Jan 18, 2016 at 2:26 PM, Tom Lane wrote:
> > Robert Haas writes:
> >> On Fri, Jan 15, 2016 at 2:16 PM, Tom Lane wrote:
> >>> Presumably the hope would be that VACUUM would truncate off some of the
> >>> heap, else there's not much good that's going to happen. That leaves
> >>> me wondering exactly what the invariant is for the maps, and if it's
> >>> okay to not touch them during a heap truncation.
> >
> >> No, you're missing the point, or at least I think you are. Suppose
> >> somebody creates a big table and then deletes all the tuples in the
> >> second half, but VACUUM never runs. When at last VACUUM does run on
> >> that table, it will try to build the VM and FSM forks as it scans the
> >> table, and will only truncate AFTER that has been done. If building
> >> the VM and FSM forks fails because there is no freespace, we will
> >> never reach the part of the operation that could create some.
> >
> > No, I follow that perfectly. I think you missed *my* point, which is:
> > suppose that we do have a full-length VM and/or FSM fork for a relation,
> > and VACUUM decides to truncate the relation. Is it okay to not truncate
> > the VM/FSM? If it isn't, we're going to have to have very tricky
> > semantics for any "don't touch the map forks" option, because it will
> > have to suppress only some of VACUUM's map updates.
>
> Oh, I see. I think it's probably not a good idea to skip truncating
> those maps, but perhaps the option could be defined as making no new
> entries, rather than ignoring them altogether, so that they never
> grow. It seems that both of those are defined in such a way that if
> page Y follows page X in the heap, the entries for page Y in the
> relation fork will follow the one for page X. So truncating them
> should be OK; it's just growing them that we need to avoid.
>
Thank you Robert. PFA basic patch, it introduces EMERGENCY option to VACUUM
that forces to avoid extend any entries in the VM or FSM. It seems working
fine in simple test scenarios e.g.
postgres=# create table test1 as (select generate_series(1,10));
> SELECT 10
> postgres=# vacuum EMERGENCY test1;
> VACUUM
> postgres=# select pg_relation_filepath('test1');
> pg_relation_filepath
> --
> base/13250/16384
> (1 row)
> [asif@centos66 inst_96]$ find . | grep base/13250/16384
> ./data/base/13250/16384
> postgres=# vacuum test1;
> VACUUM
> [asif@centos66 inst_96]$ find . | grep base/13250/16384
> ./data/base/13250/16384
> ./data/base/13250/16384_fsm
> ./data/base/13250/16384_vm
Please do let me know if I missed something or more information is
required. Thanks.
Regards,
Muhammad Asif Naeem
> > An alternative approach that might avoid such worries is to have a mode
> > wherein VACUUM always truncates the map forks to nothing, rather than
> > attempting to update them.
>
> That could work, too, but might be stronger medicine than needed.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
VACUUM_EMERGENCY_Option_v1.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] 9.5: Can't connect with PGSSLMODE=require on Windows
Thank you Tom. The issue seems not reproducible anymore with latest PG95 source code (commit 60fcee9e5e77dc748a9787fae34328917683b95e) Windows build i.e. C:\PG\postgresql\pg95_with_openssl>bin\psql.exe -d postgres -h > 172.16.141.232 > psql (9.5alpha2) > WARNING: Console code page (437) differs from Windows code page (1252) > 8-bit characters might not work correctly. See psql reference > page "Notes for Windows users" for details. > SSL connection (protocol: TLSv1.2, cipher: ECDHE-RSA-AES256-GCM-SHA384, > bits: 256, compression: off) > Type "help" for help. > postgres=# select version(); > version > - > PostgreSQL 9.5alpha2, compiled by Visual C++ build 1800, 64-bit > (1 row) Regards, Muhammad Asif Naeem On Tue, Sep 29, 2015 at 3:03 AM, Tom Lane wrote: > Thom Brown writes: > > With 9.5 alpha 2 on Windows 8 (64-bit), trying to require SSL results > > in a blocking error: > > I've pushed a patch for this; can you verify it on Windows? > > regards, tom lane > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
Re: [HACKERS] 9.5: Can't connect with PGSSLMODE=require on Windows
I have spent sometime to investigate the issue, it is reproduciable. In case of Windows, when pqsecure_raw_read() function error code WSAEWOULDBLOCK (EWOULDBLOCK) when no data queued to be read from the non blocking socket there is a need to log retry flag. Related error code can be retrieved via Windows WSAGetLastError() instead of errno, preprocessor SOCK_ERRNO handle it gracefully. PFA patch, it resolve the issue i.e. C:\PG\postgresql\pg_with_openssl_inst_v1_patch>bin\psql.exe -d postgres -h > 172.16.141.210 > psql (9.5alpha2) > WARNING: Console code page (437) differs from Windows code page (1252) > 8-bit characters might not work correctly. See psql reference > page "Notes for Windows users" for details. > SSL connection (protocol: TLSv1.2, cipher: ECDHE-RSA-AES256-GCM-SHA384, > bits: 256, compression: off) > Type "help" for help. > postgres=# select version(); > version > - > PostgreSQL 9.5alpha2, compiled by Visual C++ build 1800, 64-bit > (1 row) Regards, Muhammad Asif Naeem On Thu, Sep 24, 2015 at 5:12 PM, Thom Brown wrote: > On 23 September 2015 at 13:10, Michael Paquier > wrote: > > > > > > On Wed, Sep 23, 2015 at 2:15 AM, Robert Haas > wrote: > >> > >> On Tue, Sep 22, 2015 at 11:23 AM, Andrew Dunstan > >> wrote: > >> > "git bisect" is your friend. > >> > >> Yeah, but finding someone who has a working Windows build environment > >> and a lot of time to run this down is my enemy. We're trying, but if > >> anyone else has a clue, that would be much appreciated. > > > > > > That's not cool. I have added this problem in the list of open items for > > 9.5. > > This appears that it might be related to the version of OpenSSL that's > been packaged with PostgreSQL 9.5 alpha 2. When swapping this out for > the version that's shipped with 9.4, it works. I don't have the > specific OpenSSL versions to hand, but I'll report back anything as I > learn more. > > -- > Thom > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > win_ssl_issue_v1.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] Fix broken Install.bat when target directory contains a space
Thank you Michael, latest patch looks good to me. I have changed its status to ready for committer. Regards, Muhammad Asif Naeem On Tue, Apr 21, 2015 at 6:02 PM, Michael Paquier wrote: > > > On Tue, Apr 21, 2015 at 4:33 PM, Asif Naeem wrote: > >> The v2 patch looks good to me, just a minor concern on usage message i.e. >> >> C:\PG\postgresql\src\tools\msvc>install >>> Invalid command line options. >>> Usage: "install.bat [installtype]" >>> installtype: client >> >> >> It seems that there are two install options i.e. client, all (any other >> string other than client is being considered or treated as all), the >> following install command works i.e. >> >> install C:\PG\postgresql\inst option_does_not_exist >> >> >> As your patch effects this area of code, I thought to share these >> findings with you,o BTW, it is a minor thing that can be handled in another >> patch, >> > > Well, that's the same behavior that this script has been having for ages. > Let's just update the usage message to mention both "all" and "client". I > see no point in breaking a behavior that has been like that for ages, and > the main point of this patch is to fix the install path issue. > > >> If you like please feel free to change status to ready for committer. >> > > Well, I don't think that the patch author should do that. So I won't do it > by myself. > > Attached is an updated patch. > Regards, > -- > Michael >
Re: [HACKERS] Fix broken Install.bat when target directory contains a space
The v2 patch looks good to me, just a minor concern on usage message i.e.
C:\PG\postgresql\src\tools\msvc>install
> Invalid command line options.
> Usage: "install.bat [installtype]"
> installtype: client
It seems that there are two install options i.e. client, all (any other
string other than client is being considered or treated as all), the
following install command works i.e.
install C:\PG\postgresql\inst option_does_not_exist
As your patch effects this area of code, I thought to share these findings
with you, BTW, it is a minor thing that can be handled in another patch, If
you like please feel free to change status to ready for committer. Thanks.
On Fri, Apr 17, 2015 at 10:36 AM, Michael Paquier wrote:
> On Thu, Apr 16, 2015 at 5:40 PM, Asif Naeem wrote:
> > Along with fixing the space in installation path, it is also changing the
> > behavior of install script, why not just "if NOT [%1]==[] GOTO
> RUN_INSTALL",
> > checking for "/?" seems good for help message but it seem not well
> handled
> > in the script, it is still saying "Invalid command line options.", along
> > with this, option "/?" seems not handled by any other .bat build script.
> > Other than this, with the patch applied, is it an acceptable behavior
> that
> > (2) shows usage message as 'Usage: install.pl
> [installtype]' but
> > (3) shows usage message as 'Usage: "install.bat "'. Thanks.
>
> Thanks for your review!
>
> OK, let's remove the use of /? then, consistency with the other
> scripts is a good argument for its removal.Attached is an updated
> patch that does the following regarding missing arguments:
> >install
> Invalid command line options.
> Usage: "install.bat [installtype]"
> installtype: client
> >install
> Installing version 9.5 for release in /?
> Copying build output files...Could not copy release\postgres\postgres.exe
> to /?\
> bin\postgres.exe
> at Install.pm line 40.
> Install::lcopy("release\\postgres\\postgres.exe",
> "/?\\bin\\postgres.exe
> ") called at Install.pm line 324
> Install::CopySolutionOutput("release", "/?") called at Install.pm
> line 9
> 3
> Install::Install("/?", undef) called at install.pl line 13
>
> This patch fixes of course the issue with spaces included in the
> target path. I updated as well the Usage in install.bat to be
> consistent with install.pl.
> Regards,
> --
> Michael
>
Re: [HACKERS] Fix broken Install.bat when target directory contains a space
Hi Michael,
I spend spend time look into the patch. Good catch, I am also surprised to
see that current Windows install script don’t support spaces in the path.
Please see my findings as following i.e.
*Without the patch*
1.
> C:\PG\postgresql\src\tools\msvc>install "C:\PG\postgresql\inst with space
> without patch"
> with was unexpected at this time.
>
2.
> C:\PG\postgresql\src\tools\msvc>install
> Invalid command line options.
> Usage: "install.bat "
>
3.
> C:\PG\postgresql\src\tools\msvc>install /?
> Installing version 9.5 for release in /?
> Copying build output files...Could not copy release\postgres\postgres.exe
> to /?\bin\postgres.exe
> at Install.pm line 40
> Install::lcopy('release\postgres\postgres.exe', '/?\bin\postgres.exe')
> called at Install.pm line 324
> Install::CopySolutionOutput('release', '/?') called at Install.pm line 93
> Install::Install('/?', undef) called at install.pl line 13
*With the patch*
1.
> C:\PG\postgresql\src\tools\msvc>install "C:\PG\postgresql\inst with space
> without patch"
> Works fine.
>
2.
> C:\PG\postgresql\src\tools\msvc>install
> Usage: install.pl [installtype]
> installtype: client
>
3.
> C:\PG\postgresql\src\tools\msvc>install /?
> Invalid command line options.
> Usage: "install.bat "
Following change looks confusing to me i.e.
> -if NOT "%1"=="" GOTO RUN_INSTALL
> +if NOT [%1]==[/?] GOTO RUN_INSTALL
Along with fixing the space in installation path, it is also changing the
behavior of install script, why not just "if NOT [%1]==[] GOTO
RUN_INSTALL", checking for "/?" seems good for help message but it seem not
well handled in the script, it is still saying “Invalid command line
options.”, along with this, option "/?" seems not handled by any other .bat
build script. Other than this, with the patch applied, is it an acceptable
behavior that (2) shows usage message as 'Usage:* install.pl
<http://install.pl>* [installtype]' but (3) shows usage message
as 'Usage: "*install.bat* "'. Thanks.
Regards,
Muhammad Asif Naeem
On Mon, Mar 2, 2015 at 9:27 AM, Michael Paquier
wrote:
> Hi all,
>
> When using install.bat with a path containing spaces, I got surprised
> by a couple of errors.
> 1) First with this path:
> $ install "c:\Program Files\pgsql"
> I am getting the following error:
> Files\pgsql""=="" was unexpected at this time.
> This is caused by an incorrect evaluation of the first parameter in
> install.bat.
> 2) After correcting the first error, the path is truncated to
> c:\Program because first argument value does not seem to be correctly
> parsed when used with install.pl.
>
> Attached is a patch fixing both problems. I imagine that it would be
> good to get that backpatched.
> Regards,
> --
> Michael
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>
[HACKERS] Re: Install shared libs in lib/ and bin/ with MSVC (Was: install libpq.dll in bin directory on Windows / Cygwin)
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: tested, failed Spec compliant: not tested Documentation:not tested Thank you Michael. v4 patch looks good to me. The new status of this patch is: Ready for Committer -- 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] Install shared libs in lib/ and bin/ with MSVC (Was: install libpq.dll in bin directory on Windows / Cygwin)
Thank you Michael overall v3 patch looks good to me, There is one
observation that it is not installing following lib files that are required
for dev work i.e.
>
> inst\lib\libpq.lib
> inst\lib\libpgtypes.lib
> inst\lib\libecpg_compat.lib
> inst\lib\libecpg.lib
Please do let me know if I missed something but was not there a need to
avoid installing related .dll files in lib (duplicate) along with bin
directory e.g.
src\tools\msvc\Install.pm
>
> if ($is_sharedlib)
> {
> @dirs = qw(bin);
> }
> else
> {
> @dirs = qw(lib);
> }
Thanks.
On Mon, Mar 9, 2015 at 1:16 PM, Michael Paquier
wrote:
> On Wed, Mar 4, 2015 at 4:13 AM, Michael Paquier wrote:
> > Those entries are removed as well in the patch.
>
> Please find attached a new version of the patch, fixing a failure for
> plperl installation that contains GNUmakefile instead of Makefile.
> --
> Michael
>
Re: [HACKERS] chkpass with RANDOMIZE_ALLOCATED_MEMORY
Thank you Tom, Thank you Amit. Regards, Muhammad Asif Naeem On Wed, Mar 4, 2015 at 9:30 AM, Tom Lane wrote: > Amit Kapila writes: > > On Sat, Feb 14, 2015 at 10:26 PM, Tom Lane wrote: > >> It's not a false alarm, unfortunately, because chkpass_in actually does > >> give different results from one call to the next. We could fix the > aspect > >> of that involving failing to zero out unused bytes (which it appears was > >> introduced by sloppy replacement of strncpy with strlcpy). But we can't > >> really do anything about the dependency on random(), because that's part > >> of the fundamental specification of the data type. It was a bad idea, > >> no doubt, to design the input function to do this; but we're stuck with > >> it now. > > > It seems to me that fix for this issue has already been committed > > (commit-id: 80986e85). So isn't it better to mark as Committed in > > CF app [1] or are you expecting anything more related to this issue? > > > [1]: https://commitfest.postgresql.org/4/144/ > > Ah, I didn't realize there was a CF entry for it, I think. Yeah, > I think we committed as much as we should of this, so I marked the > CF entry as committed. > > regards, tom lane >
Re: [HACKERS] Install shared libs in lib/ and bin/ with MSVC (Was: install libpq.dll in bin directory on Windows / Cygwin)
Thank you Michael. I have looked the patch. Overall logic looks good to me,
I have checked it with MSVC{2013,2008}. It works for MSVC 2013 but fail for
MSVC 2008, I think the condition "if ($proj =~
qr{ResourceCompile\s*Include="([^"]+)”})” is not going to work for MSVC2008
and MSVC2005 i.e.
MSVC2013
>
MSVC2008
>
For more details please check i.e.
> src/tools/msvc/MSBuildProject.pm (Visual C++ 2010 or greater)
> src/tools/msvc/VCBuildProject.pm (Visual C++ 2005/2008)
It seems that search criteria can be narrowed to skip reading related
Makefile for SO_MAJOR_VERSION string when VS project file is related to
StaticLibrary or Application. Although this patch is going to make MSVC
build consistent with Cygwin and MinGW build, following files seems
redundant now, is there any use for them other than backward compatibility
? i.e.
inst\lib\libpq.dll
> inst\lib\libpgtypes.dll
> inst\lib\libecpg_compat.dll
> inst\lib\libecpg.dll
>
Thanks.
Regards,
Muhammad Asif Naeem
On Tue, Jan 20, 2015 at 6:06 AM, Michael Paquier
wrote:
> Hi all,
>
> Here is a continuation of the thread which covered $subject for MinGW
> and Cygwin:
> http://www.postgresql.org/message-id/51f19059.7050...@pgexperts.com
> But this time it is for MSVC.
>
> Just a bit of background... Since commit cb4a3b04 we are installing
> shared libraries in bin/ and lib/ for Cygwin and MinGW on Windows, but
> we are still missing MSVC, so as of now build method is inconsistent.
> Attached is a patch aimed at fixing this inconsistency. What it does
> is simple: when kicking Install.pm to install the deliverables of each
> project, we check if the project Makefile contains SO_MAJOR_VERSION.
> If yes, libraries of this project are installed in bin/ and lib/. The
> path to the Makefile is found by scanning ResourceCompile in the
> vcproj file, this method having the advantage to make the patch
> independent of build process.
>
> This also removes a wart present in Install.pm installing copying
> manually libpq.dll:
> - lcopy($target . '/lib/libpq.dll', $target . '/bin/libpq.dll');
>
> I am adding an entry in the upcoming CF for this patch, and let's use
> this thread for this discussion.
> Regards,
> --
> Michael
>
>
> --
> 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] New CF app deployment
On Fri, Feb 27, 2015 at 11:32 AM, Stefan Kaltenbrunner < ste...@kaltenbrunner.cc> wrote: > On 02/26/2015 01:59 PM, Michael Paquier wrote: > > On Thu, Feb 26, 2015 at 9:15 PM, Asif Naeem wrote: > >> This thread seems relevant, Please guide me to how can access older CF > pages > >>> The MSVC portion of this fix got completely lost in the void: > >>> https://commitfest.postgresql.org/action/patch_view?id=1330 > >> > >> Above link result in the following error i.e. > >> > >>> Not found > >>> The specified URL was not found. > >> > >> Please do let me know if I missed something. Thanks. > > > > Try commitfest-old instead, that is where the past CF app stores its > > data, like that: > > https://commitfest-old.postgresql.org/action/patch_view?id=1330 > > hmm maybe we should have some sort of handler the redirects/reverse > proxies to the old commitfest app for this. > 1+ for seamless integration, at least error message seems require to be more helpful. Thanks. > Stefan >
Re: [HACKERS] New CF app deployment
Thank you Michael, It works. On Thu, Feb 26, 2015 at 5:59 PM, Michael Paquier wrote: > On Thu, Feb 26, 2015 at 9:15 PM, Asif Naeem wrote: > > This thread seems relevant, Please guide me to how can access older CF > pages > >> The MSVC portion of this fix got completely lost in the void: > >> https://commitfest.postgresql.org/action/patch_view?id=1330 > > > > Above link result in the following error i.e. > > > >> Not found > >> The specified URL was not found. > > > > Please do let me know if I missed something. Thanks. > > Try commitfest-old instead, that is where the past CF app stores its > data, like that: > https://commitfest-old.postgresql.org/action/patch_view?id=1330 > -- > Michael >
Re: [HACKERS] New CF app deployment
Hi, This thread seems relevant, Please guide me to how can access older CF pages e.g. Thread http://www.postgresql.org/message-id/flat/51f19059.7050...@pgexperts.com#51f19059.7050...@pgexperts.com mentions following link i.e. The MSVC portion of this fix got completely lost in the void: > https://commitfest.postgresql.org/action/patch_view?id=1330 Above link result in the following error i.e. Not found > The specified URL was not found. > Please do let me know if I missed something. Thanks. Regards, Muhammad Asif Naeem On Tue, Feb 24, 2015 at 11:59 PM, Peter Eisentraut wrote: > On 2/22/15 3:12 PM, Magnus Hagander wrote: > > Would you suggest removing the automated system completely, or keep it > > around and just make it possible to override it (either by removing the > > note that something is a patch, or by making something that's not listed > > as a patch become marked as such)? > > I would remove it completely. It has been demonstrated to not work. > > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
[HACKERS] chkpass with RANDOMIZE_ALLOCATED_MEMORY
Hi,
It is been observed on RANDOMIZE_ALLOCATED_MEMORY enabled PG95 build that
chkpass is failing because of uninitialized memory and seems showing false
alarm. I have tried to add code snippets to explain as following i.e.
postgres=# CREATE EXTENSION chkpass;
> WARNING: type input function chkpass_in should not be volatile
> CREATE EXTENSION
> postgres=# CREATE TABLE test_tbl ( name text, pass chkpass );
> CREATE TABLE
> postgres=# INSERT INTO test_tbl VALUES('foo','foo1');
> WARNING: type chkpass has unstable input conversion for "foo1"
> LINE 1: INSERT INTO test_tbl VALUES('foo','foo1');
> ^
> INSERT 0 1
It is giving warning "type chkpass has unstable input conversion" because
chkpass structure hold uninitialized memory area’s that are left unused.
When chkpass_in() is called with input “foo1”, it allocate 16 bytes of
randomized memory for chkpass i.e.
contrib/chkpass/chkpass.c
> /*
> * This is the internal storage format for CHKPASSs.
> * 15 is all I need but add a little buffer
> */
> typedef struct chkpass
> {
> charpassword[16];
> } chkpass;
chkpass_in()
> result = (chkpass *) palloc(sizeof(chkpass));
*result memory*
> 0x7ffc93047a68: 0xdd 0xde 0xdf 0xe0 0xe1 0xe2 0xe3 0xe4
> 0x7ffc93047a70: 0xe5 0xe6 0xe7 0xe8 0xe9 0xea 0xeb 0xec
>
It copies string (16 bytes max) returned from crypt() function, it copies
until null character reached i.e.
strlcpy(result->password, crypt_output, sizeof(result->password));
*crypt_output memory*
> 0x7fff7d1577f0: 0x6a 0x6e 0x6d 0x54 0x44 0x53 0x55 0x33
> 0x7fff7d1577f8: 0x4b 0x48 0x52 0x55 0x6f 0x00 0x00 0x00
*result memory*
> 0x7ffc93047a68: 0x6a 0x6e 0x6d 0x54 0x44 0x53 0x55 0x33
> 0x7ffc93047a70: 0x4b 0x48 0x52 0x55 0x6f 0x00 0xeb 0xec
It left remaining last 2 byte left untouched. It is the cause for "unstable
input conversion” warning.
I think these uninitialized memory areas are false alarms. Along with this
there seems another issue that makes chkpass troubling
RANDOMIZE_ALLOCATED_MEMORY comparison logic is that crypt() function is
being passed with random salt value for DES based result. PFA patch, to
generate consistent results, it uses constant salt value.
It seems a minor inconvenience but it will make results better. Please do
let me know if I missed something or more information is required. Thanks.
Regards,
Muhammad Asif Naeem
chkpass_RANDOMIZE_ALLOCATED_MEMORY_v1.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] Add shutdown_at_recovery_target option to recovery.conf
Hi Petr,
I have spent sometime to review the patch, overall patch looks good, it
applies fine and make check run without issue. If recovery target is
specified and shutdown_at_recovery_target is set to true, it shutdown the
server at specified recovery point. I do have few points to share i.e.
1. It seems that following log message need to be more descriptive about
reason for shutdown i.e.
+ if (recoveryShutdownAtTarget && reachedStopPoint)
> + {
> + ereport(LOG, (errmsg("shutting down")));
2. As Simon suggesting following recovery settings are not clear i.e.
shutdown_at_recovery_target = true
> pause_at_recovery_target = true
It is going to make true both but patch describe as following i.e.
+Setting this to true will set linkend="pause-at-recovery-target">
> +pause_at_recovery_target to false.
>
3. As it don’t rename reconvery.conf, subsequent attempt (without any
changes in reconvery.conf) to start of server keep showing the following
i.e.
> ...
> LOG: redo starts at 0/1803620
> DEBUG: checkpointer updated shared memory configuration values
> LOG: consistent recovery state reached at 0/1803658
> LOG: restored log file "00010002" from archive
> DEBUG: got WAL segment from archive
> LOG: restored log file "00010003" from archive
> DEBUG: got WAL segment from archive
> LOG: restored log file "00010004" from archive
> DEBUG: got WAL segment from archive
> LOG: restored log file "00010005" from archive
> DEBUG: got WAL segment from archive
> LOG: restored log file "000000010006" from archive
> DEBUG: got WAL segment from archive
> …
>
Is that right ?. Thanks.
Regards,
Muhammad Asif Naeem
On Thu, Oct 16, 2014 at 7:24 AM, Simon Riggs wrote:
> On 11 September 2014 16:02, Petr Jelinek wrote:
>
> >> What about adding something like
> action_at_recovery_target=pause|shutdown
> >> instead of increasing the number of parameters?
> >>
> >
> > That will also increase number of parameters as we can't remove the
> current
> > pause one if we want to be backwards compatible. Also there would have
> to be
> > something like action_at_recovery_target=none or off or something since
> the
> > default is that pause is on and we need to be able to turn off pause
> without
> > having to have shutdown on. What more, I am not sure I see any other
> actions
> > that could be added in the future as promote action already works and
> listen
> > (for RO queries) also already works independently of this.
>
> I accept your argument, though I have other thoughts.
>
> If someone specifies
>
> shutdown_at_recovery_target = true
> pause_at_recovery_target = true
>
> it gets a little hard to work out what to do; we shouldn't allow such
> lack of clarity.
>
> In recovery its easy to do this
>
> if (recoveryShutdownAtTarget)
>recoveryPauseAtTarget = false;
>
> but it won't be when these become GUCs, so I think Fuji's suggestion
> is a good one.
>
> No other comments on patch, other than good idea.
>
> --
> Simon Riggs http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>
>
> --
> 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] [BUGS] BUG #9652: inet types don't support min/max
Thank you for sharing updated patch. With latest 9.5 source code, patch
build is failing with following error message i.e.
/Applications/Xcode.app/Contents/Developer/usr/bin/make -C catalog
> schemapg.h
> cd ../../../src/include/catalog && '/opt/local/bin/perl' ./duplicate_oids
> 3255
> make[3]: *** [postgres.bki] Error 1
> make[2]: *** [submake-schemapg] Error 2
> make[1]: *** [all-backend-recurse] Error 2
> make: *** [all-src-recurse] Error 2
New function is being added by following commit i.e.
commit b34e37bfefbed1bf9396dde18f308d8b96fd176c
> Author: Robert Haas
> Date: Thu Aug 14 12:09:52 2014 -0400
> Add sortsupport routines for text.
> This provides a small but worthwhile speedup when sorting text, at
> least
> in cases to which the sortsupport machinery applies.
> Robert Haas and Peter Geoghegan
It seems that you need to use another oid. Other than this patch looks good
to me, please share updated patch and feel free to assign it to committer.
Thanks.
Regards,
Muhammad Asif Naeem
On Tue, Aug 12, 2014 at 3:12 PM, Haribabu Kommi
wrote:
> On Mon, Aug 4, 2014 at 3:22 PM, Asif Naeem wrote:
> > Sorry for not being clear, above mentioned test is related to following
> doc (sgml) changes that seems not working as described i.e.
> >
> > Table 9-35. cidr and inet Functions
> >
> > FunctionReturn TypeDescriptionExampleResult
> >
> > max(inet, inet) inetlarger of two inet typesmax('192.168.1.5',
> '192.168.1.4')192.168.1.5
> > min(inet, inet) inetsmaller of two inet typesmin('192.168.1.5',
> '192.168.1.4')192.168.1.4
> >
> > Can you please elaborate these sgml change ?
>
> I thought of adding them for newly added "network" functions but
> mistakenly I kept the names as max and min.
> Anyway with your suggestion in the earlier mail, these changes are not
> required.
>
> I removed these changes in the attached patch.
> Thanks for reviewing the patch.
>
> Regards,
> Hari Babu
> Fujitsu Australia
>
Re: [HACKERS] [BUGS] BUG #9652: inet types don't support min/max
Thank you Haribabu. Please see my comments inlined below i.e.
On Sun, Jul 27, 2014 at 11:42 AM, Haribabu Kommi
wrote:
> On Thu, Jul 24, 2014 at 5:59 PM, Asif Naeem wrote:
> > Sorry for being late. Thank you for sharing updated patch, sgml changes
> > seems not working i.e.
> >
> >> postgres=# select max('192.168.1.5', '192.168.1.4');
> >> ERROR: function max(unknown, unknown) does not exist
> >> LINE 1: select max('192.168.1.5', '192.168.1.4');
> >>
> >>^
> >> HINT: No function matches the given name and argument types. You might
> >> need to add explicit type casts.
> >> postgres=# select min('192.168.1.5', '192.168.1.4');
> >> ERROR: function min(unknown, unknown) does not exist
> >> LINE 1: select min('192.168.1.5', '192.168.1.4');
> >>
> >>^
> >> HINT: No function matches the given name and argument types. You might
> >> need to add explicit type casts.
>
> I didn't get your point. I tested the patch, the sgml changes are working
> fine.
Sorry for not being clear, above mentioned test is related to following doc
(sgml) changes that seems not working as described i.e.
*Table 9-35. cidr and inet Functions*
FunctionReturn TypeDescriptionExampleResult
max(inet, inet)inetlarger of two inet typesmax('192.168.1.5', '192.168.1.4')
192.168.1.5min(inet, inet)inetsmaller of two inet typesmin('192.168.1.5',
'192.168.1.4')192.168.1.4
Can you please elaborate these sgml change ?
> > I would suggest the following or similar changes e.g.
> >
> >> doc/src/sgml/func.sgml
> >>
> >> max( >> class="parameter">expression)
> >>
> >> - any array, numeric, string, or date/time type
> >> + any inet, array, numeric, string, or date/time
> type
> >>same as argument type
> >>
> >> maximum value of >> @@ -12204,7 +12228,7 @@ NULL baz(3 rows)
> >>
> >> min( >> class="parameter">expression)
> >>
> >> - any array, numeric, string, or date/time type
> >> + any inet, array, numeric, string, or date/time
> type
> >>same as argument type
> >>
> >> minimum value of
> Thanks for reviewing the patch.
> I missed the above change. Corrected the same in the attached patch.
>
> Regards,
> Hari Babu
> Fujitsu Australia
>
Re: [HACKERS] [BUGS] BUG #9652: inet types don't support min/max
Hi Haribabu,
Sorry for being late. Thank you for sharing updated patch, sgml changes
seems not working i.e.
postgres=# select max('192.168.1.5', '192.168.1.4');
> ERROR: function max(unknown, unknown) does not exist
> LINE 1: select max('192.168.1.5', '192.168.1.4');
>^
> HINT: No function matches the given name and argument types. You might
> need to add explicit type casts.
> postgres=# select min('192.168.1.5', '192.168.1.4');
> ERROR: function min(unknown, unknown) does not exist
> LINE 1: select min('192.168.1.5', '192.168.1.4');
>^
> HINT: No function matches the given name and argument types. You might
> need to add explicit type casts.
I would suggest the following or similar changes e.g.
doc/src/sgml/func.sgml
>
> max( class="parameter">expression)
>
> - any array, numeric, string, or date/time type
> + any inet, array, numeric, string, or date/time type
>same as argument type
>
> maximum value of @@ -12204,7 +12228,7 @@ NULL baz(3 rows)
>
> min( class="parameter">expression)
>
> - any array, numeric, string, or date/time type
> + any inet, array, numeric, string, or date/time type
>same as argument type
>
> minimum value of
wrote:
> On Mon, Jul 7, 2014 at 6:59 PM, Asif Naeem wrote:
> > Hi Haribabu,
> >
> > Thank you for sharing the patch. I have spent some time to review the
> > changes. Overall patch looks good to me, make check and manual testing
> seems
> > run fine with it. There seems no related doc/sgml changes ?. Patch added
> > network_smaller() and network_greater() functions but in PG source code,
> > general practice seems to be to use “smaller" and “larger” as related
> > function name postfix e.g. timestamp_smaller()/timestamp_larger(),
> > interval_smaller/interval_larger(), cashsmaller()/cashlarger() etc.
> Thanks.
>
> Thanks for reviewing the patch.
>
> I corrected the function names as smaller and larger.
> and also added documentation changes.
>
> Updated patch attached in the mail.
>
> Regards,
> Hari Babu
> Fujitsu Australia
>
Re: [HACKERS] [bug fix or improvement?] Correctly place DLLs for ECPG apps in bin folder
Yes. Can you please take a look at Win_lib_bin.patch I shared earlier ?, I
think it is or similar approach will be appropriate. Thanks.
Regards,
Muhammad Asif Naeem
On Tue, Jul 8, 2014 at 5:53 PM, MauMau wrote:
> From: "Asif Naeem"
>
> Other than my pervious comments, patch looks good to me. Thanks.
>>
>
> Thanks for reviewing. I understood that your previous comment was to
> suggest copying the desired DLLs directly from Release/Debug folder to bin.
> But I'm afraid it cannot be done cleanly... CopySolutionOutput() copies
> all DLLS to lib folder with no exception as follows:
>
> if ($1 == 1)
> {
>$dir = "bin";
>$ext = "exe";
> }
> elsif ($1 == 2)
> {
>$dir = "lib";
>$ext = "dll";
> }
>
> It seems like I have to do something like this, listing the relevant DLL
> names anyway. I don't think this is not cleaner.
>
> if ($1 == 1)
> {
>$dir = "bin";
>$ext = "exe";
> }
> elsif ($1 == 2 && /* the project is libpq, libecpg, etc. */)
> {
>$dir = "bin";
>$ext = "dll";
> }
> elsif ($1 == 2)
> {
>$dir = "lib";
>$ext = "dll";
> }
>
> What do you think? Am I misunderstanding your suggestion?
>
> Regards
> MauMau
>
>
Re: [HACKERS] [bug fix or improvement?] Correctly place DLLs for ECPG apps in bin folder
Hi MauMau, Other than my pervious comments, patch looks good to me. Thanks. Regards, Muhammad Asif Naeem On Wed, Feb 26, 2014 at 2:14 AM, Peter Eisentraut wrote: > You should be able to do this without specifically referencing the names > "libpq" or "ecpg". Look into the Makefile, if it defines > SO_MAJOR_VERSION, then it's a library you are concerned with, and then > do the copying or moving. >
Re: [HACKERS] [BUGS] BUG #9652: inet types don't support min/max
Hi Haribabu, Thank you for sharing the patch. I have spent some time to review the changes. Overall patch looks good to me, make check and manual testing seems run fine with it. There seems no related doc/sgml changes ?. Patch added network_smaller() and network_greater() functions but in PG source code, general practice seems to be to use “smaller" and “larger” as related function name postfix e.g. timestamp_smaller()/timestamp_larger(), interval_smaller/interval_larger(), cashsmaller()/cashlarger() etc. Thanks. Regards, Muhammad Asif Naeem On Mon, Jul 7, 2014 at 1:56 PM, Asif Naeem wrote: > On Mon, Jun 30, 2014 at 4:45 PM, Abhijit Menon-Sen > wrote: > >> At 2014-06-30 16:35:45 +0500, anaeem...@gmail.com wrote: >> > >> > pc1dotnetpk:postgresql asif$ patch -p0 < >> > > ~/core/min_max_support_for_inet_datatypes/inet_agg_v4.patch >> > > can't find file to patch at input line 3 >> > > Perhaps you used the wrong -p or --strip option? >> > > The text leading up to this was: >> > > -- >> > > |*** a/src/backend/utils/adt/network.c >> > > |--- b/src/backend/utils/adt/network.c >> > > -- >> >> You need to use patch -p1, to strip off the "a"/"b" prefix. >> > > Thank you Abhijit, It worked. > > >> >> -- Abhijit >> > >
Re: [HACKERS] [BUGS] BUG #9652: inet types don't support min/max
On Mon, Jun 30, 2014 at 4:45 PM, Abhijit Menon-Sen wrote: > At 2014-06-30 16:35:45 +0500, anaeem...@gmail.com wrote: > > > > pc1dotnetpk:postgresql asif$ patch -p0 < > > > ~/core/min_max_support_for_inet_datatypes/inet_agg_v4.patch > > > can't find file to patch at input line 3 > > > Perhaps you used the wrong -p or --strip option? > > > The text leading up to this was: > > > -- > > > |*** a/src/backend/utils/adt/network.c > > > |--- b/src/backend/utils/adt/network.c > > > -- > > You need to use patch -p1, to strip off the "a"/"b" prefix. > Thank you Abhijit, It worked. > > -- Abhijit >
Re: [HACKERS] [BUGS] BUG #9652: inet types don't support min/max
Hi Haribabu, I am not able to apply latest patch on REL9_4_STABLE or master branch i.e. pc1dotnetpk:postgresql asif$ git apply > ~/core/min_max_support_for_inet_datatypes/inet_agg_v4.patch > fatal: unrecognized input pc1dotnetpk:postgresql asif$ patch -p0 < > ~/core/min_max_support_for_inet_datatypes/inet_agg_v4.patch > can't find file to patch at input line 3 > Perhaps you used the wrong -p or --strip option? > The text leading up to this was: > -- > |*** a/src/backend/utils/adt/network.c > |--- b/src/backend/utils/adt/network.c > -- > File to patch: Is there any other utility required to apply the patch, Can you please guide ?. Thanks. Regards, Muhammad Asif Naeem On Thu, Jun 5, 2014 at 6:28 AM, Haribabu Kommi wrote: > On Thu, Jun 5, 2014 at 9:12 AM, Andres Freund > wrote: > > Hi, > > > > On 2014-06-04 10:37:48 +1000, Haribabu Kommi wrote: > >> Thanks for the test. Patch is re-based to the latest head. > > > > Did you look at the source of the conflict? Did you intentionally mark > > the functions as leakproof and reviewed that that truly is the case? Or > > was that caused by copy & paste? > > Yes it is copy & paste mistake. I didn't know much about that parameter. > Thanks for the information. I changed it. > > Regards, > Hari Babu > Fujitsu Australia > > > -- > Sent via pgsql-bugs mailing list (pgsql-b...@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-bugs > >
Re: [HACKERS] Extending MSVC scripts to support --with-extra-version
Thank you for sharing updated patch. I have compared it with MSVC and configure generated build i.e. *MacOSX (*--with-extra-version "-30JUN"*)* pc1dotnetpk:inst asif$ ./bin/psql -d postgres > psql (9.5devel-30JUN) > Type "help" for help. > postgres=# select version(); > version > > > PostgreSQL 9.5devel-30JUN on x86_64-apple-darwin13.2.0, compiled by Apple > LLVM version 5.1 (clang-503.0.40) (based on LLVM 3.4svn), 64-bit > (1 row) > pc1dotnetpk:inst asif$ ./bin/initdb -V > initdb (PostgreSQL) 9.5devel-30JUN *Windows7 64bit (*extraver => '-30JUN'*)* C:\PG\postgresql\inst_withpatch_v2_extra-version>bin\psql -d postgres > psql (9.5devel-30JUN) > WARNING: Console code page (437) differs from Windows code page (1252) > 8-bit characters might not work correctly. See psql reference > page "Notes for Windows users" for details. > Type "help" for help. > postgres=# select version(); >version > -- > PostgreSQL 9.5devel-30JUN, compiled by Visual C++ build 1600, 64-bit > (1 row) > C:\PG\postgresql\inst_withpatch_v2_extra-version>bin\initdb.exe -V > initdb (PostgreSQL) 9.5devel-30JUN Patch looks good to me. I think it is ready for committer. Thanks. Regards, Muhammad Asif Naeem On Fri, Jun 27, 2014 at 5:00 AM, Michael Paquier wrote: > > > > On Fri, Jun 27, 2014 at 8:26 AM, Asif Naeem wrote: > >> I have spent some time reviewing the code. It applies well and PG master >> branch build fine with setting extraver or keep it undefined. >> > Thanks for reviewing that. > > >> I have observed the following output applying the patch i.e. >> > It seems that extraver information only appears when version function is >> being used. If we use -V (--version) with pg utilities/binaries, it does >> not include additional provided information. >> > You are right. The first version of this patch updates PG_VERSION_STR but > not PG_VERSION, which is the string used for all the binaries to report the > version. > > >> Can you please guide how can I perform similar functionality via >> configure script (that can be used on Unix like OS/MinGW) or It is intended >> for Window specific requirement ?. Thanks. >> > Sure, you can do the equivalent with plain configure like that: > ./configure --with-extra-version="-foo" --prefix=/to/path/ > And here is the output that I get with such options on OSX for example: > $ psql -c 'select substring(version(), 1, 52)' > substring > -- > PostgreSQL 9.5devel-foo on x86_64-apple-darwin13.2.0 > (1 row) > $ initdb --version > initdb (PostgreSQL) 9.5devel-foo > > With the new patch attached, the following output is generated for an MSVC > build: > $ psql -c 'select version()' > version > > PostgreSQL 9.5devel-foo, compiled by Visual C++ build 1600, 64-bit > (1 row) > $ initdb --version > initdb (PostgreSQL) 9.5devel-foo > > Regards, > -- > Michael >
Re: [HACKERS] Extending MSVC scripts to support --with-extra-version
Hi, I have spent some time reviewing the code. It applies well and PG master branch build fine with setting extraver or keep it undefined. I have observed the following output applying the patch i.e. *Keeping extraver undefined* : C:\PG\postgresql\inst_withpatch_no_extra-version>bin\psql.exe -d postgres > psql (9.5devel) > WARNING: Console code page (437) differs from Windows code page (1252) > 8-bit characters might not work correctly. See psql reference > page "Notes for Windows users" for details. > Type "help" for help. > postgres=# select version(); > version > > PostgreSQL 9.5devel, compiled by Visual C++ build 1600, 64-bit > (1 row) > C:\PG\postgresql\inst_withpatch_no_extra-version>bin\initdb.exe -V > initdb (PostgreSQL) 9.5devel *Setting extraver as ‘June27’* : C:\PG\postgresql\inst_withpatch_extra-version>bin\psql -d postgres > psql (9.5devel) > WARNING: Console code page (437) differs from Windows code page (1252) > 8-bit characters might not work correctly. See psql reference > page "Notes for Windows users" for details. > Type "help" for help. > postgres=# select version(); >version > -- > PostgreSQL 9.5develJune27, compiled by Visual C++ build 1600, 64-bit > (1 row) > > C:\PG\postgresql\inst_withpatch_extra-version>bin\initdb.exe -V > initdb (PostgreSQL) 9.5devel It seems that extraver information only appears when version function is being used. If we use -V (--version) with pg utilities/binaries, it does not include additional provided information. Can you please guide how can I perform similar functionality via configure script (that can be used on Unix like OS/MinGW) or It is intended for Window specific requirement ?. Thanks. Regards, Muhammad Asif Naeem On Tue, May 27, 2014 at 5:58 AM, Michael Paquier wrote: > Hi all, > > Please find attached a patch extending support of --with-extra-version > in the MSVC scripts. This is something I have been using internally, > and I believe that it is useful for Windows packagers. > Similarly to the other options, a new field called extraver is added > in config_default.pl and it can be overwritten in config.pl to have an > additional version string, similarly to what is currently doable with > ./configure. > > I'll add this patch to the next commit fest. > Regards, > -- > Michael > > > -- > 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] [bug fix or improvement?] Correctly place DLLs for ECPG apps in bin folder
Hi MauMau, I have't completed tested all the expects of submitted patch yet. I would like to share my findings so far. By looking at the patch I do feel that there is room for improvement in the patch, Instead of moving related dll's from lib directory to bin directory later in the installation process, copy these files directly from release/debug directory earlier (PFA patch). It seems unfortunate that Windows don't have RPATH or similar logic implemented in OS. Alternate methods seems not appropriate, Only feasible option seems to be placing related dependency dll in same executable directory. At first one may think an alternate to create symbolic link for relative path in bin directory e.g. libpq.dll -> ..\lib\libpq.dll but it is unfortunate that normal user do require special permissions to create symbolic link otherwise it could help. There is SetDllDirectory or AddDllDirectory function available that effects only subsequent calls to LoadLibrary and LoadLibraryEx. I will look into this patch further and let you know about my more findings. Thanks. Regards, Muhammad Asif Naeem On Wed, Dec 4, 2013 at 5:07 PM, MauMau wrote: > From: "MauMau" > > In addition, I'll remove libpq.dll from lib folder unless somebody >> objects. >> Currently, libpq.dll is placed in both bin and lib. I guess libpq.dll was >> left in lib because it was considered necessary for ECPG DLLs. >> > > The attached patch also removes libpq.dll from lib folder. I don't mind > whether this patch or yesterday's one will be adopted. I'll add this to > 2014-1 commitfest this weekend if the patch is not committed until then. > > Regards > MauMau > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > > Win_lib_bin.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] [bug fix] PostgreSQL fails to start on Windows if it crashes after tablespace creation
Hi MauMau,
Ah. Sorry, I missed that part. As NTFS junctions and symbolic links are
different (although they behave similarly), there seems only a minor
inconvenience related to misleading error message i.e.
+ #ifdef WIN32
> + if (rmdir(linkloc) < 0 && errno != ENOENT)
> + #else
>if (unlink(linkloc) < 0 && errno != ENOENT)
> + #endif
>ereport(ERROR,
>(errcode_for_file_access(),
> errmsg("could not remove symbolic link \"%s\": %m",
What is your opinion about it, Is it not worth changing ? . Thanks.
On Wed, Jan 15, 2014 at 7:42 PM, MauMau wrote:
> From: "Asif Naeem"
>
> As you have
>>
> followed destroy_tablespace_directories() function, Is there any specific
> reason not to use same logic to detect type of the file/link i.e.
> “(lstat(linkloc, &st) == 0 && S_ISDIR(st.st_mode))”, It also seems have
> more appropriate error message i.e.
>
> Thanks for reviewing and testing the patch. Yes, at first I did what you
> mentioned, but modified the patch according to some advice in the mail
> thread. During redo, create_tablespace_directories() needs to handle the
> case where the $PGDATA/pg_tblspc/xxx is not a symlink but a directory even
> on UNIX/Linux. Please see TablespaceCreateDbspace is().
> destroy_tablespace_directories()
> doesn't have to handle such situation.
>
> Regards
> MauMau
>
>
Re: [HACKERS] [bug fix] PostgreSQL fails to start on Windows if it crashes after tablespace creation
I did worked on testing the patch on Windows, test scenario mentioned above
thread is reproducible and the provided patch resolve the issue. In case of
junction or directory unlink function
(deprecated<http://msdn.microsoft.com/en-us/library/ms235350.aspx>)
returns -1 with errno “EACCES” (i.e. Permission denied). As you have
followed destroy_tablespace_directories() function, Is there any specific
reason not to use same logic to detect type of the file/link i.e.
“(lstat(linkloc, &st) == 0 && S_ISDIR(st.st_mode))”, It also seems have
more appropriate error message i.e.
src/backend/commands/tablespace.c
> ….
> 745 /*
> 746 * Try to remove the symlink. We must however deal with
> the possibility
> 747 * that it's a directory instead of a symlink --- this
> could happen during
> 748 * WAL replay (see TablespaceCreateDbspace), and it is
> also the case on
> 749 * Windows where junction points lstat() as directories.
> 750 *
> 751 * Note: in the redo case, we'll return true if this final
> step fails;
> 752 * there's no point in retrying it. Also, ENOENT should
> provoke no more
> 753 * than a warning.
> 754 */
> 755 remove_symlink:
> 756 linkloc = pstrdup(linkloc_with_version_dir);
> 757 get_parent_directory(linkloc);
> 758 if (lstat(linkloc, &st) == 0 && S_ISDIR(st.st_mode))
> 759 {
> 760 if (rmdir(linkloc) < 0)
> 761 ereport(redo ? LOG : ERROR,
> 762 (errcode_for_file_access(),
> 763 errmsg("could not remove
> directory \"%s\": %m",
> 764 linkloc)));
> 765 }
> 766 else
> 767 {
> 768 if (unlink(linkloc) < 0)
> 769 ereport(redo ? LOG : (errno == ENOENT ?
> WARNING : ERROR),
> 770 (errcode_for_file_access(),
> 771 errmsg("could not remove
> symbolic link \"%s\": %m",
> 772 linkloc)));
> 773 }
> ….
Other than this patch looks good to me.
Regards,
Muhammad Asif Naeem
On Thu, Oct 31, 2013 at 8:03 PM, MauMau wrote:
> From: "MauMau"
>
> I thought the same situation could happen as in
>> destroy_tablespace_directories(), but it doesn't seem to apply on second
>> thought. Revised patch attached, which is very simple based on compile
>> time
>> check.
>>
>
> Sorry, I was careless to leave an old comment. Please use this version.
>
> Regards
> MauMau
>
>
> --
> 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] PostgreSQL Service on Windows does not start. ~ "is not a valid Win32 application"
On Thu, Oct 31, 2013 at 10:17 AM, Amit Kapila wrote: > On Tue, Oct 29, 2013 at 12:46 PM, Naoya Anzai > wrote: > > Hi Sandeep > > > >> I think, you should change the subject line to "Unquoted service path > containing space is vulnerable and can be exploited on Windows" to get the > attention.. :) > > Thank you for advice! > > I'll try to post to pgsql-bugs again. > > I could also reproduce this issue. The situation is very rare such > that an "exe" with name same as first part of directory should exist > in installation path. > I believe it is a security risk with bigger impact as it is related to Windows environment and as installers rely on it. > I suggest you can post your patch in next commit fest. Yes. Are not vulnerabilities/security risk's taken care of more urgent bases ? > With Regards, > Amit Kapila. > EnterpriseDB: http://www.enterprisedb.com >
Re: [HACKERS] PostgreSQL Service on Windows does not start. ~ "is not a valid Win32 application"
Yes. It should not be installer issue as installer is using pg_ctl to register and run the service on Windows. Thanks. Best Regards, Muhammad Asif Naeem On Tue, Oct 29, 2013 at 9:57 AM, Sandeep Thakkar < sandeep.thak...@enterprisedb.com> wrote: > So, this is not an installer issue. Is this bug raised to the PostgreSQL > community? If yes, you should submit the patch there. > > > On Tue, Oct 29, 2013 at 6:23 AM, Naoya Anzai < > anzai-na...@mxu.nes.nec.co.jp> wrote: > >> Hi, Asif >> >> Thank you for providing my patch (pg_ctl.c.patch) to Sandeep on my behalf. >> >> > Good finding. I have attached another version of patch >> (pg_ctl.c_windows_vulnerability.patch) attached that has fewer lines of >> code changes, can you please take a look ?. Thanks. >> >> I think your patch is not sufficient to fix. >> Not only "pg_ctl.exe" but "postgres.exe" also have the same problem. >> Even if your patch is attached, >> A Path of "postgres.exe" passed to CreateRestrictedProcess is not >> enclosed in quotation.(See pgwin32_ServiceMain at pg_ctl.c) >> >> So, processing enclosed in quotation should do in both conditions. >> >> Regards, >> Naoya >> >> --- >> Naoya Anzai >> Engineering Department >> NEC Soft, Ltd. >> E-Mail: anzai-na...@mxu.nes.nec.co.jp >> --- >> >> >> > Hi Sandeep, >> > >> > PFA Naoya's patch (pg_ctl.c.patch). >> > >> > Hi Naoya, >> > >> > Good finding. I have attached another version of patch >> (pg_ctl.c_windows_vulnerability.patch) attached that has fewer lines of >> code changes, can you please take a look ?. Thanks. >> > >> > Best Regards, >> > Asif Naeem >> > >> > >> > On Mon, Oct 28, 2013 at 4:46 PM, Sandeep Thakkar < >> sandeep.thak...@enterprisedb.com> wrote: >> > >> > >> > Hi Dave >> > >> > We register the service using pg_ctl. When I manually executed >> the following on the command prompt, I saw that the service path of the >> registered service did not have the pg_ctl.exe path in quotes. May be it >> should be handled in the pg_ctl code. >> > >> > c:\Users\Sandeep Thakkar\Documents>"c:\Program >> Files\PostgreSQL\9.3\bin\pg_ctl.e >> > xe" register -N "pg-9.3" -U "NT AUTHORITY\NetworkService" -D >> "c:\Program Files\P >> > ostgreSQL\9.3\data" -w >> > >> > Naoya, I could not find your patch here. Can you please share it >> again? >> > >> > >> > >> > On Mon, Oct 28, 2013 at 2:53 PM, Dave Page >> wrote: >> > >> > >> > Sandeep, can you look at this please? Thanks. >> > >> > On Mon, Oct 28, 2013 at 8:18 AM, Asif Naeem < >> anaeem...@gmail.com> wrote: >> > > It is related to windows unquoted service path >> vulnerability in the the >> > > installer that creates service path without quotes that >> make service.exe to >> > > look for undesirable path for executable. >> > > >> > > postgresql-9.3 service path : >> C:/Users/asif/Desktop/Program >> > > files/9.3/bin/pg_ctl.exe runservice -N "postgresql-9.3" >> -D >> > > "C:/Users/asif/Desktop/Program files/9.3/data" -w >> > > >> > > service.exe >> > >> >> > >> C:\Users\asif\Desktop\Program NAME NOT FOUND >> > >> C:\Users\asif\Desktop\Program.exe NAME NOT FOUND >> > >> C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe >> ACCESS DENIED >> > >> C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe >> ACCESS DENIED >> > >> C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe >> runservice NAME >> > >> NOT FOUND >> > >> C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe >> runservice.exe >> > >> NAME NOT FOUND >> > >> C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe >> runservice -N >> > >> NAME NOT FOUND >> > >> C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe
Re: [HACKERS] PostgreSQL Service on Windows does not start. ~ "is not a valid Win32 application"
Hi Sandeep, PFA Naoya's patch (pg_ctl.c.patch). Hi Naoya, Good finding. I have attached another version of patch (pg_ctl.c_windows_vulnerability.patch) attached that has fewer lines of code changes, can you please take a look ?. Thanks. Best Regards, Asif Naeem On Mon, Oct 28, 2013 at 4:46 PM, Sandeep Thakkar < sandeep.thak...@enterprisedb.com> wrote: > Hi Dave > > We register the service using pg_ctl. When I manually executed the > following on the command prompt, I saw that the service path of the > registered service did not have the pg_ctl.exe path in quotes. May be it > should be handled in the pg_ctl code. > > *c:\Users\Sandeep Thakkar\Documents>*"c:\Program > Files\PostgreSQL\9.3\bin\pg_ctl.e > xe" register -N "pg-9.3" -U "NT AUTHORITY\NetworkService" -D "c:\Program > Files\P > ostgreSQL\9.3\data" -w > > Naoya, I could not find your patch here. Can you please share it again? > > > > On Mon, Oct 28, 2013 at 2:53 PM, Dave Page wrote: > >> Sandeep, can you look at this please? Thanks. >> >> On Mon, Oct 28, 2013 at 8:18 AM, Asif Naeem wrote: >> > It is related to windows unquoted service path vulnerability in the the >> > installer that creates service path without quotes that make >> service.exe to >> > look for undesirable path for executable. >> > >> > postgresql-9.3 service path : C:/Users/asif/Desktop/Program >> > files/9.3/bin/pg_ctl.exe runservice -N "postgresql-9.3" -D >> > "C:/Users/asif/Desktop/Program files/9.3/data" -w >> > >> > service.exe >> >> >> >> C:\Users\asif\Desktop\Program NAME NOT FOUND >> >> C:\Users\asif\Desktop\Program.exe NAME NOT FOUND >> >> C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe ACCESS >> DENIED >> >> C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe ACCESS >> DENIED >> >> C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice >> NAME >> >> NOT FOUND >> >> C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice.exe >> >> NAME NOT FOUND >> >> C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice -N >> >> NAME NOT FOUND >> >> C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice >> -N.exe >> >> NAME NOT FOUND >> >> C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice -N >> >> "postgresql-9.3" NAME INVALID >> >> C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice -N >> >> "postgresql-9.3".exe NAME INVALID >> >> C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice -N >> >> "postgresql-9.3" -D NAME INVALID >> >> C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice -N >> >> "postgresql-9.3" -D.exe NAME INVALID >> >> C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice -N >> >> "postgresql-9.3" -D "C:\Users\asif\Desktop\Program NAME INVALID >> >> C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice -N >> >> "postgresql-9.3" -D "C:\Users\asif\Desktop\Program.exe NAME INVALID >> >> C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice -N >> >> "postgresql-9.3" -D "C:\Users\asif\Desktop\Program files\9.3\data" >> NAME >> >> INVALID >> >> C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice -N >> >> "postgresql-9.3" -D "C:\Users\asif\Desktop\Program files\9.3\data".exe >> >> NAME INVALID >> >> C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice -N >> >> "postgresql-9.3" -D "C:\Users\asif\Desktop\Program files\9.3\data" -w >> >> NAME INVALID >> >> C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice -N >> >> "postgresql-9.3" -D "C:\Users\asif\Desktop\Program files\9.3\data" >> -w.exe >> >> NAME INVALID >> > >> > >> > Fix : >> > >> > postgresql-9.3 service path : "C:/Users/asif/Desktop/Program >> > files/9.3/bin/pg_ctl.exe" runservice -N "postgresql-9.3" -D >> > "C:/Users/asif/Desktop/Program files/9.3/data" -w >> > >> > It would be good if this is reported on pg installer forum or security >> > forum. Thanks. >> > >> > Regards, >> > Asif Naeem >>
Re: [HACKERS] PostgreSQL Service on Windows does not start. ~ "is not a valid Win32 application"
It is related to windows unquoted service path vulnerability in the the installer that creates service path without quotes that make service.exe to look for undesirable path for executable. postgresql-9.3 service path : C:/Users/asif/Desktop/Program files/9.3/bin/pg_ctl.exe runservice -N "postgresql-9.3" -D "C:/Users/asif/Desktop/Program files/9.3/data" -w service.exe > C:\Users\asif\Desktop\Program NAME NOT FOUND > C:\Users\asif\Desktop\Program.exe NAME NOT FOUND > C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe ACCESS DENIED > C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe ACCESS DENIED > > C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice NAME > NOT FOUND > C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice.exe > NAME NOT FOUND > C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice -N > NAME NOT FOUND > C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice -N.exe > NAME NOT FOUND > C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice -N > "postgresql-9.3" NAME INVALID > C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice -N > "postgresql-9.3".exe NAME INVALID > C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice -N > "postgresql-9.3" -D NAME INVALID > C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice -N > "postgresql-9.3" -D.exe NAME INVALID > C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice -N > "postgresql-9.3" -D "C:\Users\asif\Desktop\Program NAME INVALID > C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice -N > "postgresql-9.3" -D "C:\Users\asif\Desktop\Program.exe NAME INVALID > C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice -N > "postgresql-9.3" -D "C:\Users\asif\Desktop\Program files\9.3\data" NAME > INVALID > C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice -N > "postgresql-9.3" -D "C:\Users\asif\Desktop\Program files\9.3\data".exe > NAME INVALID > C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice -N > "postgresql-9.3" -D "C:\Users\asif\Desktop\Program files\9.3\data" -w > NAME INVALID > C:\Users\asif\Desktop\Program files\9.3\bin\pg_ctl.exe runservice -N > "postgresql-9.3" -D "C:\Users\asif\Desktop\Program files\9.3\data" -w.exe > NAME INVALID Fix : postgresql-9.3 service path : "C:/Users/asif/Desktop/Program files/9.3/bin/pg_ctl.exe" runservice -N "postgresql-9.3" -D "C:/Users/asif/Desktop/Program files/9.3/data" -w It would be good if this is reported on pg installer forum or security forum. Thanks. Regards, Asif Naeem On Mon, Oct 28, 2013 at 12:06 PM, Naoya Anzai wrote: > > Hi, Asif. > > Thank you for response. > > > > C:\Users\asif\Desktop\Program files\9.3>"bin\pg_ctl" -D "C:\Users\asif\Desktop\Program files\9.3\data1" -l logfile start > > server starting > > This failure does not occur by the command line. > PostgreSQL needs to start by Windows Service. > > Additionally,In this case, > A file "Program" needs to be exist at "C:\Users\asif\Desktop\", and > "postgres.exe" needs to be exist at "C:\Users\asif\Desktop\Program files\9.3\bin". > > C:\Users\asif\Desktop\Program files\9.3\bin>dir > ... > 4,435,456 postgres.exe >80,896 pg_ctl.exe > ... > > C:\Users\asif\Desktopp>dir > ... > 0 Program > Program files > ... > > > Regards, > Naoya > > > Hi Naoya, > > > > I am not able to reproduce the problem. Do you mean pg windows service installed by installer is not working or bin\pg_ctl binary is not accepting spaces in the patch ?. Following worked for me i.e. > > > > > > C:\Users\asif\Desktop\Program files\9.3>"bin\pg_ctl" -D "C:\Users\asif\Desktop\Program files\9.3\data1" -l logfile start > > server starting > > > > > > Can you please share the exact steps ?. Thanks. > > > > > > Regards, > > Muhammad Asif Naeem > > > > > > > > On Mon, Oct 28, 2013 at 10:26 AM, Naoya Anzai < anzai-na...@mxu.nes.nec.co.jp> wrote: > > > > > > Hi All, > > > > I have found a case that PostgreSQL Service does not start. > > When it happens, the following error appears. > > > >"is not a valid Win32 application" > > > > This failure occurs whe
Re: [HACKERS] PostgreSQL Service on Windows does not start. ~ "is not a valid Win32 application"
Hi Naoya, I am not able to reproduce the problem. Do you mean pg windows service installed by installer is not working or bin\pg_ctl binary is not accepting spaces in the patch ?. Following worked for me i.e. C:\Users\asif\Desktop\Program files\9.3>"bin\pg_ctl" -D > "C:\Users\asif\Desktop\Program files\9.3\data1" -l logfile start > server starting Can you please share the exact steps ?. Thanks. Regards, Muhammad Asif Naeem On Mon, Oct 28, 2013 at 10:26 AM, Naoya Anzai wrote: > Hi All, > > I have found a case that PostgreSQL Service does not start. > When it happens, the following error appears. > > "is not a valid Win32 application" > > This failure occurs when the following conditions are true. > > 1. There is "postgres.exe" in any directory that contains a space, >such as "Program Files". > >e.g.) >C:\Program Files\PostgreSQL\bin\postgres.exe > > 2. A file using the first white space-delimited >tokens of that directory as the file name exists, >and there is it in the same hierarchy. > >e.g.) >C:\Program //file > > "pg_ctl.exe" as PostgreSQL Service creates a postgres > process using an absolute path which indicates the > location of "postgres.exe",but the path is not enclosed > in quotation. > > Therefore,if the above-mentioned conditions are true, > CreateProcessAsUser(a Windows Function called by pg_ctl.exe) > tries to create a process using the other file such > as "Program", so the service fails to start. > > Accordingly, I think that the command path should be > enclosed in quotation. > > I created a patch to fix this failure, > So could anyone confirm? > > Regards, > > Naoya > > --- > Naoya Anzai > Engineering Department > NEC Soft, Ltd. > E-Mail: anzai-na...@mxu.nes.nec.co.jp > --- > > > -- > 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] [PATCH] Add use of asprintf()
On Tue, Oct 15, 2013 at 10:55 AM, David Rowley wrote: > Though this is not yet enough to get the windows build work with me... I'm > still getting link failures for isolationtester.c > > > "D:\Postgres\c\pgsql.sln" (default target) (1) -> > "D:\Postgres\c\isolationtester.vcxproj" (default target) (89) -> > (Link target) -> > isolationtester.obj : error LNK2019: unresolved external symbol > _pg_strdup referenced in function _try_complete_step > [D:\Postgres\c\isolationtester.vcxproj] > isolationtester.obj : error LNK2019: unresolved external symbol > _pg_asprintf referenced in function _try_complete_step > [D:\Postgres\c\isolationtester.vcxproj > ] > .\Release\isolationtester\isolationtester.exe : fatal error LNK1120: 2 > unresolved externals [D:\Postgres\c\isolationtester.vcxproj] > > 1 Warning(s) > > I guess this is down to a make file error somewhere. > Can you please try the attached patch ?. I hope it will solve the problem. > > David > > > > Win_isolationtester_fix.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] [PATCH] Add use of asprintf()
+1
I think you can safely use va_list without copy on Windows. va_copy is
available in Visual Studio 2013 as part of support for C99, previous
versions don't have it.
Regards,
Muhammad Asif Naeem
On Tue, Oct 15, 2013 at 10:33 AM, Amit Kapila wrote:
> On Tue, Oct 15, 2013 at 2:18 AM, Peter Eisentraut wrote:
> > On Mon, 2013-10-14 at 23:08 +1300, David Rowley wrote:
> >
> >>
> >> Looks like something like:
> >>
> >>
> >> #ifndef WIN32
> >> #define HAVE_VA_COPY 1
> >> #endif
> >>
> >>
> >> would need to be added to asprintf.c, but also some work needs to be
> >> done with mcxt.c as it uses va_copy unconditionally. Perhaps just
> >> defining a macro for va_copy would be better for windows. I was not
> >> quite sure the best header file for such a macro so I did not write a
> >> patch to fix it.
> >
> > Does Windows not have va_copy? What do they use instead?
>
> No, Windows doesn't have va_copy, instead they use something like below:
> #define va_copy(dest, src) (dest = src)
>
> Please refer below link for details of porting va_copy() on Windows:
> http://stackoverflow.com/questions/558223/va-copy-porting-to-visual-c
>
> I could see that there is similar handling in code of vasprintf(),
> such that if va_copy is not available then directly assign src to dst.
>
> #if defined(HAVE_VA_COPY)
> va_copy(ap2, ap);
> #define my_va_end(ap2) va_end(ap2)
> #elif defined(HAVE___BUILTIN_VA_COPY)
> __builtin_va_copy(ap2, ap);
> #define my_va_end(ap2) __builtin_va_end(ap2)
> #else
> ap2 = ap;
> #define my_va_end(ap2) do {} while (0)
> #endif
>
> I think rather than having writing code like above at places where
> va_copy is used, we can use something like:
> #ifdef WIN32
> #define va_copy(dest, src) (dest = src)
> #endif
>
> and define HAVE_VA_COPY to 1 for non-windows platform.
>
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>
Re: [HACKERS] [PATCH] Add use of asprintf()
You are right, wget worked. Latest patch looks good to me. make check run fine. Thank you Peter. On Wed, Oct 2, 2013 at 5:02 PM, Peter Eisentraut wrote: > On 10/2/13 5:12 AM, Asif Naeem wrote: > > Neither git nor patch command apply the patch successfully. Can you > > please guide ?. Thanks. > > Works for me. Check that your email client isn't mangling line endings. > >
Re: [HACKERS] [PATCH] Add use of asprintf()
Thank you Peter.
When I try to apply the patch on latest PG source code on master branch, it
gives the following error message i.e.
> asif$ git apply /Users/asif/core/Patch\:\ Add\ use\ of\
> asprintf\(\)/v2-0001-Add-use-of-asprintf.patch
> /Users/asif/core/Patch: Add use of
> asprintf()/v2-0001-Add-use-of-asprintf.patch:76: trailing whitespace.
> /Users/asif/core/Patch: Add use of
> asprintf()/v2-0001-Add-use-of-asprintf.patch:77: trailing whitespace.
> for ac_func in asprintf crypt fls getopt getrusage inet_aton random rint
> srandom strerror strlcat strlcpy
> /Users/asif/core/Patch: Add use of
> asprintf()/v2-0001-Add-use-of-asprintf.patch:90: trailing whitespace.
> AC_REPLACE_FUNCS([asprintf crypt fls getopt getrusage inet_aton random
> rint srandom strerror strlcat strlcpy])
> /Users/asif/core/Patch: Add use of
> asprintf()/v2-0001-Add-use-of-asprintf.patch:104: trailing whitespace.
> values[1] = psprintf("%s/%s", fctx->location, de->d_name);
> /Users/asif/core/Patch: Add use of
> asprintf()/v2-0001-Add-use-of-asprintf.patch:118: trailing whitespace.
> pg_asprintf(&todo,
> fatal: git apply: bad git-diff - expected /dev/null on line 1557
Neither git nor patch command apply the patch successfully. Can you please
guide ?. Thanks.
Best Regards,
Muhammad Asif Naeem
On Wed, Oct 2, 2013 at 7:29 AM, Peter Eisentraut wrote:
> On Tue, 2013-09-17 at 15:13 +0500, Asif Naeem wrote:
> > I did put some time review the patch, please see my findings below
> > i.e.
>
> Updated patch for this.
>
>
Re: [HACKERS] [PATCH] Add use of asprintf()
On Tue, Sep 17, 2013 at 3:13 PM, Asif Naeem wrote:
> Hi,
>
> I did put some time review the patch, please see my findings below i.e.
>
> 1. It seems that you have used strdup() on multiple places in the patch,
> e.g. in the below code snippet is it going to lead crash if newp->ident is
> NULL because of strdup() failure ?
>
> static EPlan *
>> find_plan(char *ident, EPlan **eplan, int *nplans)
>> {
>> ...
>> newp->ident = strdup(ident);
>> ...
>> }
>
>
> 2. Can we rely on return value of asprintf() instead of recompute length
> as strlen(cmdbuf) ?
>
> if (asprintf(&cmdbuf, "COMMENT ON LARGE OBJECT %u IS '", loid) <
>> 0)
>>return fail_lo_xact("\\lo_import", own_transaction);
>> bufptr = cmdbuf + strlen(cmdbuf);
>
>
> 3. There seems naming convention for functions like pfree (that seems
> similar to free()), pstrdup etc; but psprintf seems different than sprintf.
> Can't we use pg_asprintf instead in the patch, as it seems that both
> (psprintf and pg_asprintf) provide almost same functionality ?
>
> 4. It seems that "ret < 0" need to be changed to "rc < 0" in the following
> code i.e.
>
> int
>> pg_asprintf(char **ret, const char *format, ...)
>> {
>> va_list ap;
>> int rc;
>> va_start(ap, format);
>> rc = vasprintf(ret, format, ap);
>> va_end(ap);
>> if (ret < 0)
>> {
>> fprintf(stderr, _("out of memory\n"));
>> exit(EXIT_FAILURE);
>> }
>> return rc;
>> }
>
>
It seems this point is already mentioned by Alvaro. Thanks.
>
> 5. It seems that in the previously implementation, error handling was not
> there, is it appropriate here that if there is issue in duplicating
> environment, quit the application ? i.e.
>
> /*
>> * Handy subroutine for setting an environment variable "var" to "val"
>> */
>> static void
>> doputenv(const char *var, const char *val)
>> {
>> char*s;
>> pg_asprintf(&s, "%s=%s", var, val);
>> putenv(s);
>> }
>
>
>
> Please do let me know if I missed something or more info is required.
> Thank you.
>
> Regards,
> Muhammad Asif Naeem
>
>
> On Tue, Sep 17, 2013 at 1:31 AM, Alvaro Herrera
> wrote:
>
>> Peter Eisentraut wrote:
>>
>> > The attached patch should speak for itself.
>>
>> Yeah, it's a very nice cleanup.
>>
>> > I have supplied a few variants:
>> >
>> > - asprintf() is the standard function, supplied by libpgport if
>> > necessary.
>> >
>> > - pg_asprintf() is asprintf() with automatic error handling (like
>> > pg_malloc(), etc.)
>> >
>> > - psprintf() is the same idea but with palloc.
>>
>> Looks good to me, except that pg_asprintf seems to be checking ret
>> instead of rc.
>>
>> Is there a reason for the API discrepancy of pg_asprintf vs. psprintf?
>> I don't see that we use the integer return value anywhere. Callers
>> interested in the return value can use asprintf directly (and you have
>> already inserted callers that do nonstandard things using direct
>> asprintf).
>>
>> --
>> Álvaro Herrerahttp://www.2ndQuadrant.com/
>> PostgreSQL Development, 24x7 Support, Training & Services
>>
>>
>> --
>> 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] [PATCH] Add use of asprintf()
Hi,
I did put some time review the patch, please see my findings below i.e.
1. It seems that you have used strdup() on multiple places in the patch,
e.g. in the below code snippet is it going to lead crash if newp->ident is
NULL because of strdup() failure ?
static EPlan *
> find_plan(char *ident, EPlan **eplan, int *nplans)
> {
> ...
> newp->ident = strdup(ident);
> ...
> }
2. Can we rely on return value of asprintf() instead of recompute length as
strlen(cmdbuf) ?
if (asprintf(&cmdbuf, "COMMENT ON LARGE OBJECT %u IS '", loid) <
> 0)
>return fail_lo_xact("\\lo_import", own_transaction);
> bufptr = cmdbuf + strlen(cmdbuf);
3. There seems naming convention for functions like pfree (that seems
similar to free()), pstrdup etc; but psprintf seems different than sprintf.
Can't we use pg_asprintf instead in the patch, as it seems that both
(psprintf and pg_asprintf) provide almost same functionality ?
4. It seems that "ret < 0" need to be changed to "rc < 0" in the following
code i.e.
int
> pg_asprintf(char **ret, const char *format, ...)
> {
> va_list ap;
> int rc;
> va_start(ap, format);
> rc = vasprintf(ret, format, ap);
> va_end(ap);
> if (ret < 0)
> {
> fprintf(stderr, _("out of memory\n"));
> exit(EXIT_FAILURE);
> }
> return rc;
> }
5. It seems that in the previously implementation, error handling was not
there, is it appropriate here that if there is issue in duplicating
environment, quit the application ? i.e.
/*
> * Handy subroutine for setting an environment variable "var" to "val"
> */
> static void
> doputenv(const char *var, const char *val)
> {
> char *s;
> pg_asprintf(&s, "%s=%s", var, val);
> putenv(s);
> }
Please do let me know if I missed something or more info is required. Thank
you.
Regards,
Muhammad Asif Naeem
On Tue, Sep 17, 2013 at 1:31 AM, Alvaro Herrera wrote:
> Peter Eisentraut wrote:
>
> > The attached patch should speak for itself.
>
> Yeah, it's a very nice cleanup.
>
> > I have supplied a few variants:
> >
> > - asprintf() is the standard function, supplied by libpgport if
> > necessary.
> >
> > - pg_asprintf() is asprintf() with automatic error handling (like
> > pg_malloc(), etc.)
> >
> > - psprintf() is the same idea but with palloc.
>
> Looks good to me, except that pg_asprintf seems to be checking ret
> instead of rc.
>
> Is there a reason for the API discrepancy of pg_asprintf vs. psprintf?
> I don't see that we use the integer return value anywhere. Callers
> interested in the return value can use asprintf directly (and you have
> already inserted callers that do nonstandard things using direct
> asprintf).
>
> --
> Álvaro Herrerahttp://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>
>
> --
> 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] pg_ctl idempotent option
I am working on reviewing the patch. Patch apply without warning/error on master branch. My findings are as following i.e. 1. Behavior change in pg_ctl return value i.e. * * * Server already running* a. Without Patch inst asif$ ./bin/pg_ctl -D data_test/ -l data_test.log start pg_ctl: another server might be running; trying to start server anyway server starting inst asif$ echo $? 0 b. With Patch inst_pg_ctl_idempotent_option asif$ ./bin/pg_ctl -D data_test/ -l > data_test.log start > pg_ctl: another server might be running > inst_pg_ctl_idempotent_option asif$ echo $? > 1 2. -w option seems not working for start as per documentation, it should return 0. *Starting already running server with -I -w option* inst_pg_ctl_idempotent_option asif$ ./bin/pg_ctl -D data_test/ -l > data_test.log -I -w start pg_ctl: another server might be running; trying to start server anyway waiting for server to start pg_ctl: this data directory appears to be running a pre-existing postmaster stopped waiting pg_ctl: could not start server Examine the log output. inst_pg_ctl_idempotent_option asif$ echo $? 1 3. I believe postmaster (DAEMON="$prefix/bin/postmaster") is not going to accept "-I" option as mentioned in the patch i.e. contrib/start-scripts/linux > su - $PGUSER -c "$DAEMON -I -D '$PGDATA' &" >>$PGLOG 2>& Rest of the patch changes looks good to me. Thanks. Best Regards, Asif Naeem On Thu, Jan 24, 2013 at 6:06 PM, Bruce Momjian wrote: > On Thu, Jan 24, 2013 at 09:05:59AM +0530, Ashutosh Bapat wrote: > > I agree, answering the question, whether the particular attempt of > > starting a server succeeded or not, will need the current behaviour. > > Now, question is which of these behaviours should be default? > > That would work. pg_upgrade knows the cluster version at that point and > can use the proper flag. > > -- > Bruce Momjian http://momjian.us > EnterpriseDB http://enterprisedb.com > > + It's impossible for everything to be true. + > > > -- > 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] plpython issue with Win64 (PG 9.2)
> Patch attached. Asif, could you try a few things on a CP1252 database?
First verify if your original test case now works and then try this:
>
>
I have test the patch on Win64. postgres server is working fine now for
WIN1252. Thanks.
> create function enctest() returns text as $$
> return b'tr\xc3\xb3spido'.decode('**utf-8')
> $$ language plpython3u;
>
> select enctest(), encode(convert_to(enctest(), 'utf-8'), 'hex');
>
create function enctest() returns text as $$
return b'tr\xc3\xb3spido'.decode('utf-8')
$$ language plpython3u;
select enctest(), encode(convert_to(enctest(), 'utf-8'), 'hex');
enctest | encode
--+
tróspido | 7472c3b3737069646f
(1 row)
Please do let me know If you have any other query. Thanks.
Best Regards,
Muhammad Asif Naeem
Re: [HACKERS] plpython issue with Win64 (PG 9.2)
Thank you. Please do let me know once fix check-in. I will test it and share feedback with you. Thanks. Best Regards, Asif Naeem On Fri, Jun 29, 2012 at 3:36 AM, Jan Urbański wrote: > On 27/06/12 13:57, Jan Urbański wrote: > >> On 27/06/12 11:51, Asif Naeem wrote: >> >>> Hi, >>> >>> On Windows 7 64bit, plpython is causing server crash with the following >>> test case i.e. >>> >>> CREATE PROCEDURAL LANGUAGE 'plpython3u'; >>> >>>> CREATE OR REPLACE FUNCTION pymax (a integer, b integer) >>>> RETURNS integer >>>> AS $$ >>>> if a> b: >>>> return a >>>> return b >>>> $$ LANGUAGE plpython3u; >>>> SELECT pymax(1, 2); >>>> >>> >> >>> I think primary reason that trigger this issue is when Function >>> PLyUnicode_Bytes() calls "PyUnicode_AsEncodedString( ,WIN1252 /*Server >>> encoding*/, ) " it fails with null. I built latest pg 9.2 source code >>> with >>> python 3.2.2.3 by using Visual Studio 2010. Thanks. >>> >> >> I'll try to reproduce this on Linux, which should be possible given the >> results of your investigation. >> > > Your analysis is correct, I managed to reproduce this by injecting > > serverenc = "win1252"; > > into PLyUnicode_Bytes. The comment in that function says that Python > understands all PostgreSQL encoding names except for SQL_ASCII, but that's > not really true. In your case GetDatabaseEncodingName() returns "WIN1252" > and Python accepts "CP125". > > I'm wondering how this should be fixed. Just by adding more special cases > in PLyUnicode_Bytes? > > Even if we add a switch statement that would convert PG_WIN1250 into > "CP1250", Python can still raise an exception when encoding (for various > reasons). How about replacing the PLy_elog there with just an elog? This > loses traceback support and the Python exception message, which could be > helpful for debugging (something like "invalid character for encoding > cp1250"). OTOH, I'm uneasy about invoking the entire PLy_elog machinery > from a function that's as low-level as PLyUnicode_Bytes. > > Lastly, we map SQL_ASCII to "ascii" which is arguably wrong. The function > is supposed to return bytes in the server encoding, and under SQL_ASCII > that probably means we can return anything (ie. use any encoding we deem > useful). Using "ascii" as the Python codec name will raise an error on > anything that has the high bit set. > > So: I'd add code to translate WINxxx into CPxxx when choosing the Python > to use, change PLy_elog to elog in PLyUnicode_Bytes and leave the SQL_ASCII > case alone, as there were no complaints and people using SQL_ASCII are > asking for it anyway. > > Cheers, > Jan >
[HACKERS] plpython issue with Win64 (PG 9.2)
Hi, On Windows 7 64bit, plpython is causing server crash with the following test case i.e. CREATE PROCEDURAL LANGUAGE 'plpython3u'; > CREATE OR REPLACE FUNCTION pymax (a integer, b integer) > RETURNS integer > AS $$ > if a > b: > return a > return b > $$ LANGUAGE plpython3u; > SELECT pymax(1, 2); Server exit with the following exception i.e. Unhandled exception at 0x777a3483 in postgres.exe: 0xC0FD: Stack overflow. plpython3.dll!PLy_traceback(char * * xmsg, char * * tbmsg, int * tb_depth) Line 174 C plpython3.dll!PLy_elog(int elevel, const char * fmt, ...) Line 67 C plpython3.dll!PLyUnicode_AsString(_object * unicode) Line 96 C plpython3.dll!PLy_traceback(char * * xmsg, char * * tbmsg, int * tb_depth) Line 176 + 0x8 bytes C plpython3.dll!PLy_elog(int elevel, const char * fmt, ...) Line 67 C plpython3.dll!PLyUnicode_AsString(_object * unicode) Line 96 C ... ... plpython3.dll!PLy_traceback(char * * xmsg, char * * tbmsg, int * tb_depth) Line 176 + 0x8 bytes C plpython3.dll!PLy_elog(int elevel, const char * fmt, ...) Line 67 C plpython3.dll!PLyUnicode_AsString(_object * unicode) Line 96 C plpython3.dll!PLy_traceback(char * * xmsg, char * * tbmsg, int * tb_depth) Line 176 + 0x8 bytes C Dbserver get stuck in the following call loop i.e. ... PLy_elog() -> PLy_traceback() -> PLyUnicode_AsString() -> PLyUnicode_Bytes() -> PLy_elog() ... I think primary reason that trigger this issue is when Function PLyUnicode_Bytes() calls "PyUnicode_AsEncodedString( ,WIN1252 /*Server encoding*/, ) " it fails with null. I built latest pg 9.2 source code with python 3.2.2.3 by using Visual Studio 2010. Thanks. Best Regards, Muhammad Asif Naeem
[HACKERS] pgdump tar bug (PG 9.2)
Hi, With the following test case pgdump creates a corrupt tar file i.e. CREATE DATABASE dump_test; > \c dump_test > CREATE TABLE test_table1 (int1 int); > INSERT INTO test_table1 (SELECT * FROM generate_series(1, 1000)); > \! pg_dump -F t -f dump_test.tar dump_test Debugging shows that pg_dump tries to fopen tar file with "w" option that corrupts already opened archive file i.e. _CloseArchive() -> RestoreArchive() -> SetOutput() -> fopen(filename, > PG_BINARY_W); man fopen > ... > ... > w > Truncate file to zero length or create text file for writing. The > stream is positioned at the beginning of the file. This issue is caused by addition of the following code in function _CloseArchive() i.e. > memcpy(ropt, AH->ropt, sizeof(RestoreOptions)); It was intruduced by recent patch is as following i.e. > commit 4317e0246c645f60c39e6572644cff1cb03b4c65 > Author: Tom Lane > Date: Tue May 29 23:22:14 2012 -0400 > Rewrite --section option to decouple it from --schema-only/--data-only. PFA patch. Thanks. Best Regards, Muhammad Asif Naeem pg_backup_tar.c.patch.pg 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] plpython crash (PG 92)
FYI, I have observed this crash on Linux64. Thanks. Best Regards, Muhammad Asif Naeem On Thu, Apr 26, 2012 at 5:32 PM, Asif Naeem wrote: > Hi, > > PFA test case. It used simple select statement to retrieve data via > plpython. It crashes latest pg 9.2 with the following stack trace i.e. > > #0 0x0073021f in pfree () >> #1 0x7fa74b632f7a in PLy_result_dealloc () from >> /home/masif/work/postgresql/postgresql/inst/lib/plpython2.so >> #2 0x7fa74b2c710b in iter_iternext (iterator=0x1ad7150) at >> Objects/iterobject.c:74 >> #3 0x7fa74b2934db in PyIter_Next (iter=0x1b3c5f0) at >> Objects/abstract.c:3107 >> #4 0x7fa74b630245 in PLy_exec_function () from >> /home/masif/work/postgresql/postgresql/inst/lib/plpython2.so >> #5 0x7fa74b630c57 in plpython_call_handler () from >> /home/masif/work/postgresql/postgresql/inst/lib/plpython2.so >> #6 0x00583907 in ExecMakeFunctionResult () >> #7 0x0057f146 in ExecProject () >> #8 0x00596740 in ExecResult () >> #9 0x0057e708 in ExecProcNode () >> #10 0x0057d582 in standard_ExecutorRun () >> #11 0x0064f477 in PortalRunSelect () >> #12 0x00650778 in PortalRun () >> #13 0x0064ceca in exec_simple_query () >> #14 0x0064ddc7 in PostgresMain () >> #15 0x0060bdd9 in ServerLoop () >> #16 0x0060e9d7 in PostmasterMain () >> #17 0x005ad360 in main () > > > Apparently it is being crashed because of invalid related pointer value of > pfree() *header->context->methods->free_p. It is reproducible with latest > version of python i.e. Python-2.7.3 and Python-3.2.3. Thanks. > > Best Regards, > Muhammad Asif Naeem >
[HACKERS] plpython crash (PG 92)
Hi, PFA test case. It used simple select statement to retrieve data via plpython. It crashes latest pg 9.2 with the following stack trace i.e. #0 0x0073021f in pfree () > #1 0x7fa74b632f7a in PLy_result_dealloc () from > /home/masif/work/postgresql/postgresql/inst/lib/plpython2.so > #2 0x7fa74b2c710b in iter_iternext (iterator=0x1ad7150) at > Objects/iterobject.c:74 > #3 0x7fa74b2934db in PyIter_Next (iter=0x1b3c5f0) at > Objects/abstract.c:3107 > #4 0x7fa74b630245 in PLy_exec_function () from > /home/masif/work/postgresql/postgresql/inst/lib/plpython2.so > #5 0x7fa74b630c57 in plpython_call_handler () from > /home/masif/work/postgresql/postgresql/inst/lib/plpython2.so > #6 0x00583907 in ExecMakeFunctionResult () > #7 0x0057f146 in ExecProject () > #8 0x00596740 in ExecResult () > #9 0x0057e708 in ExecProcNode () > #10 0x0057d582 in standard_ExecutorRun () > #11 0x0064f477 in PortalRunSelect () > #12 0x00650778 in PortalRun () > #13 0x0064ceca in exec_simple_query () > #14 0x0064ddc7 in PostgresMain () > #15 0x0060bdd9 in ServerLoop () > #16 0x0060e9d7 in PostmasterMain () > #17 0x005ad360 in main () Apparently it is being crashed because of invalid related pointer value of pfree() *header->context->methods->free_p. It is reproducible with latest version of python i.e. Python-2.7.3 and Python-3.2.3. Thanks. Best Regards, Muhammad Asif Naeem plpython_crash.sql 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] Error trying to compile a simple C trigger
It seems that compiler is complain about "Relation" structure, can you please try adding the following in trigtest.c i.e. #include "utils/rel.h" Best Regards, Asif Naeem On Tue, Mar 20, 2012 at 3:53 PM, Marco Nenciarini < marco.nenciar...@2ndquadrant.it> wrote: > I was trying to compile orafce on the current master and it yield > an error at line > > tupdesc = trigdata->tg_relation->rd_att; > > alert.c: In function ‘dbms_alert_defered_signal’: > alert.c:839:33: error: dereferencing pointer to incomplete type > make: *** [alert.o] Error 1 > > I've also tried the example at > > http://www.postgresql.org/docs/devel/static/trigger-example.html > > and the result is exactly the same. > > trigtest.c: In function ‘trigf’: > trigtest.c:44:36: error: dereferencing pointer to incomplete type > make: *** [trigtest.o] Error 1 > > Regards, > Marco > > -- > Marco Nenciarini - 2ndQuadrant Italy > PostgreSQL Training, Services and Support > marco.nenciar...@2ndquadrant.it | www.2ndQuadrant.it > > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
