Re: [HACKERS] Small patch for pg_basebackup argument parsing

[Ryan Murphy]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:not tested

I applied this patch via patch -p1.  (Had an issue using git apply, but 
apparently that's common?)

All tests passed normally.  Ran make check, make installcheck, and make 
installcheck-world.

Looked thru the diffs and it looks fine to me.
Changing a lot of a integer/long arguments that were being read directly via 
atoi / atol
to be read as strings first, to give better error handling.

This looks good to go to me.  Reviewing this as "Ready for Committer".
Thanks for the patch, Pierre!

Ryan

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] Small patch for pg_basebackup argument parsing

[Ryan Murphy]

Great, thanks Pierre!

I don't have a chance to try the patch tonight, but I will on the weekend
if no one else beats me to it.

On Wed, Sep 13, 2017 at 12:53 PM Pierre Ducroquet <p.p...@pinaraf.info>
wrote:

> On Wednesday, September 13, 2017 2:06:50 AM CEST Daniel Gustafsson wrote:
> > > On 05 Jul 2017, at 08:32, Michael Paquier <michael.paqu...@gmail.com>
> > > wrote:>
> > > On Wed, Jul 5, 2017 at 2:57 PM, Ryan Murphy <ryanfmur...@gmail.com>
> wrote:
> > >> I tried to apply your patch to test it (though reading Robert's last
> > >> comment it seems we wish to have it adjusted before committing)... but
> > >> in any case I was not able to apply your patch to the tip of the
> master
> > >> branch (my git apply failed).  I'm setting this to Waiting On Author
> for
> > >> a new patch, and I also agree with Robert that the test can be simpler
> > >> and can go in the other order.  If you don't have time to make another
> > >> patch, let me know, I may be able to make one.>
> > > git is unhappy even if forcibly applied with patch -p1. You should
> > > check for whitespaces at the same time:
> > > $ git diff --check
> > > src/bin/pg_basebackup/pg_receivewal.c:483: indent with spaces.
> > > +char   *strtol_endptr = NULL
> > > And there are more than this one.
> >
> > Like Michael said above, this patch no longer applies and have some
> > whitespace issues.  The conflicts in applying are quite trivial though,
> so
> > it should be easy to create a new version.  Do you plan to work on this
> > during this Commitfest so we can move this patch forward?
>
> Yes, my bad. Attached to this email is the new version, that now applies on
> master.
>
> Sorry for the delay :(
>

Re: [HACKERS] postgres_fdw bug in 9.6

[Ryan Murphy]

> I tested that with HEAD, but couldn't reproduce this problem.  Didn't you
test that with HEAD?

Yeah, I know it's not an issue other people are having - I think it's
something specific about how my environment / postgres build is set up.  In
any case I knew that it wasn't caused by your patch.

Thanks,
Ryan

Re: [HACKERS] postgres_fdw bug in 9.6

[Ryan Murphy]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   not tested
Spec compliant:   not tested
Documentation:not tested

This feature is obviously a pretty deep cut, and I don't understand the JOIN 
system enough to comment on whether the code is correct.  I did not see any 
issues when glancing through the patch.

I ran "make check", "make installcheck" and "make installcheck-world" and had 
ALMOST no problems:  I marked it Tested and Passed because the only issue I had 
was a recurring issue I've had with "make installcheck-world" which I think is 
unrelated:

*** path/to/postgres/src/interfaces/ecpg/test/expected/connect-test5.stderr 
2017-02-14 09:22:25.0 -0600
--- path/to/postgres/src/interfaces/ecpg/test/results/connect-test5.stderr  
2017-09-04 23:31:50.0 -0500
***
*** 36,42 
  [NO_PID]: sqlca: code: 0, state: 0
  [NO_PID]: ECPGconnect: opening database  on  port  
 for user regress_ecpg_user2
  [NO_PID]: sqlca: code: 0, state: 0
! [NO_PID]: ECPGconnect: could not open database: FATAL:  database 
"regress_ecpg_user2" does not exist
...

But this still Needs Review by someone who knows the JOIN system better.

Best,
Ryan
-- 
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] Useless code in ExecInitModifyTable

[Ryan Murphy]

> FWIW, I find that good ol' patch is much more reliable about applying
> patches that didn't come from git itself.  In this case
>
>
Thanks, I knew I was missing something!
Ryan

Re: [HACKERS] Useless code in ExecInitModifyTable

[Ryan Murphy]

Hello!

I downloaded the latest patch "clean-nodeModifyTable-v2.patch"
and tried applying it to HEAD using "git apply ".

The only response from git was "fatal: unrecognized input".
I tried this with git 2.5.4 stable that comes native on my mac, and git 2.12 
built from source.
In both cases I got the same error.

I know you recently rebased this patch already, but could you please 
double-check it?
Of course it's possible I'm doing something wrong.

Thanks,
Ryan

The new status of this patch is: Waiting on Author

-- 
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] assorted code cleanup

[Ryan Murphy]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:tested, passed

I've reviewed the code changes, and it's pretty clear to me that they clean 
things up a bit while not changing any behavior.  They simplify things in a way 
that make the code more comprehensible.  I've run all the tests and they behave 
the same way as they did before the patch.  I also trying manually playing 
around the the function in question, `metaphone`, and it seems to behave the 
same as before.

I think it's ready to commit!

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] 1 test fails in make installcheck-world - database "regress_ecpg_user2" does not exist

[Ryan Murphy]

> Thanks for reporting it!
>

My pleasure!

So the initial issue didn't happen the 2nd time.  So if misc_sanity was the
only test
failing then I guess my tests are working fine other than that.  Sweet!

When I get a break from work I'll review some patches!

Best,
Ryan

Re: [HACKERS] 1 test fails in make installcheck-world - database "regress_ecpg_user2" does not exist

[Ryan Murphy]

> No, you're reading it backwards: the error is expected, but it's not
> appearing in your results.  I can duplicate this if I manually create
> database "regress_ecpg_user2" before running ecpg's installcheck,
> so I guess that's what you did.  I can find no evidence that any
> part of the PG regression tests creates such a database.
>

Thanks, that makes sense.  However, when I go into my database
with psql and type `drop database regress_ecpg_user2;`, the
response is "ERROR:  database "regress_ecpg_user2" does not exist".
So it seems either the tests are creating it somehow and then cleaning
it up (though I agree that I found no obvious evidence of that in the
codebase), or could I be looking in the wrong postgres install?

I think it's the same install though.  I have my postgres installing into an
install_dir/ directory.  Here's how I run configure:

#!/bin/sh
CFLAGS='-O0' ./configure \
--prefix=path/to/postgres/install_dir/ \
--with-uuid=e2fs \
--enable-debug \
--with-perl

I did notice that the test seems to create a ROLE called regress_ecpg_user2:

$ git grep -n 'regress_ecpg_user2'
src/interfaces/ecpg/test/Makefile:78:REGRESS_OPTS =
--dbname=ecpg1_regression,ecpg2_regression
--create-role=regress_ecpg_user1,regress_ecpg_user2 $(EXTRA_REGRESS_OPTS)
...

Could that implicitly create a database too?  I know that I somehow have a
database named after my username / postgres role "murftown".

I ran "make installcheck-world" again, and, the result is different this
time -
a test called "misc_sanity" failed early on in the tests:

$ make installcheck-world
make -C src/test installcheck
make -C perl installcheck
make[2]: Nothing to be done for `installcheck'.
make -C regress installcheck
...
== dropping database "regression" ==
DROP DATABASE
== creating database "regression" ==
CREATE DATABASE
ALTER DATABASE
== running regression test queries==
test tablespace   ... ok
test boolean  ... ok
test char ... ok
...
test regex... ok
test oidjoins ... ok
test type_sanity  ... ok
test opr_sanity   ... ok
test misc_sanity  ... FAILED
test comments ... ok
test expressions  ... ok
test insert   ... ok
test insert_conflict  ... ok
test create_function_1... ok
...

The old failure with the missing error message is gone from
regression.diffs this time.
I've attached the new regression.diffs.

Best,
Ryan


regression2.diffs
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] 1 test fails in make installcheck-world - database "regress_ecpg_user2" does not exist

[Ryan Murphy]

Hello Postgres Hackers,

I want to become more helpful to the project, reviewing more patches and
starting to write more of my own - and one of the first steps is to get all
the tests passing so I can confidently review patches.  It almost worked...

I typed "make check" and all the tests passed.
Then I typed "make installcheck" and all the tests passed again.

But when I typed "make installcheck-world", I got this:

===
 1 of 55 tests failed.
===

The differences that caused some tests to fail can be viewed in the
file "path/to/postgres/src/interfaces/ecpg/test/regression.diffs".  A copy
of the test summary that you see
above is saved in the file
"path/to/postgres/src/interfaces/ecpg/test/regression.out".

And when I look at that diffs file, this is what I see:

- [NO_PID]: ECPGconnect: could not open database: FATAL:  database
"regress_ecpg_user2" does not exist

Why would this database not be getting created?  The full diffs file is
attached.

Thanks for your help!
Ryan


regression.diffs
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 in Physical Replication Slots (at least 9.5)?

[Ryan Murphy]

Poking this.  Looking back through the discussion, this issue has been 
reproduced by multiple people.  The patch still applies to HEAD without issues. 
 I have no experience with PostgreSQL replication, so I'm not qualified to 
really review this.  From what I can see with the patch, it's just a small 
block of code added to /src/backend/replication/walreceiver.c to handle some 
edge case where the WAL file no longer exists or something.

I think one thing that would help move this forward is if we edited the patch 
to include a comment explaining why this new code is necessary.  There's lots 
of great discussion on this issue in the email list, so if a summary of that 
gets into the code I think it would make the patch easier to understand and 
make the new walreceiver.c less confusing.
-- 
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: Error-like LOG when connecting with SSL for password authentication

[Ryan Murphy]

Hi Michael,

I tried to apply your patch to HEAD and it failed.  But I think that's because 
some version of it has already been committed, correct?  I see some of your 
ECONNRESET and "SSL connection has been closed unexpectedly" code already in 
HEAD.

Best,
Ryan

The new status of this patch is: Waiting on Author

-- 
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] CommitFest 2017-09 - How do I know what commit to apply patches to

[Ryan Murphy]

Thanks guys!

The expectation is that the patch will be applied to HEAD, so it should
> apply to HEAD. If not, the custom is to ask the author for a rebase.
>
>
Makes sense!  I'll do that.

(I've taken to running a cronjob that tells me when patches I've
> posted no longer apply, so I can rebase and repost ASAP.)
>
>
Smart idea!  I may try this in the future.

Best,
Ryan

Re: [HACKERS] Small patch for pg_basebackup argument parsing

[Ryan Murphy]

Hi Pierre,

I tried to apply your patch to test it (though reading Robert's last comment it 
seems we wish to have it adjusted before committing)... but in any case I was 
not able to apply your patch to the tip of the master branch (my git apply 
failed).  I'm setting this to Waiting On Author for a new patch, and I also 
agree with Robert that the test can be simpler and can go in the other order.  
If you don't have time to make another patch, let me know, I may be able to 
make one.

Thanks!
Ryan

The new status of this patch is: Waiting on Author

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] CommitFest 2017-09 - How do I know what commit to apply patches to

[Ryan Murphy]

Hello PostgreSQL hackers,

I was diving into CommitFest 2017-09 
to help review some patches, but I was not sure which version / git commit
/ git tag of the PostgreSQL repo I should be checked out to in order to
correctly apply / test these patches.  Is there a place within the
CommitFest where it says what version to have checked out?

Thank you,
Ryan

Re: [HACKERS] Incorrect mentions to pg_xlog in walmethods.c/h

[Ryan Murphy]

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:   not tested
Spec compliant:   not tested
Documentation:not tested

This commit only affects comments, so I'm confident it doesn't break code, 
though I did not run the tests.

I understand that the pg_xlog directory was renamed to pg_wal (confirmed that 
this is in the changelog too), so the comment changes seem correct.

I am marking this Ready for Committer.

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] Does having a NULL column automatically exclude the table from the tupleDesc cache?

[Ryan Murphy]

> > You'll probably want to do those at a C level, bypassing the executor. I
> > would guess that executor overhead will completely swamp the effect of
> the
> > cache in most cases.
>
> That seems like it's kind of missing the point.  If the tupleDesc
> cache saves so little that it's irrelevant when tested through the
> executor, it's not a very useful cache.  I bet that's not the case,
> though.
>

Thank you both for your insight.  I'll probably hold off on the benchmarks
for right now.  I didn't have a production reason to worry about the cache,
just getting more familiar with the codebase.  Thanks again!

Re: [HACKERS] Does having a NULL column automatically exclude the table from the tupleDesc cache?

[Ryan Murphy]

> attcacheoff can only be set positive for fields preceding any varlena
> (typlen<0, but including the first such) or nullable values.  I don't
> know how much faster it is with the cache; you can measure it if your
> curiosity is strong enough -- just set the first column to nullable.
>
>
Thanks!  Maybe I'll do some benchmarks.

Re: [HACKERS] Does having a NULL column automatically exclude the table from the tupleDesc cache?

[Ryan Murphy]

> No, that tests whether the particular tuple contains any null fields, not
> whether the whole table is declared NOT NULL.
>
> regards, tom lane
>

Ok, thanks, that makes sense.

My question kind of remains though - does that mean that having any nulls
in the tuple loses the ability to use the tupleDesc cache? and how much of
a performance impact is this?  I'm sure there's a good reason why you can't
really use the cache if you have a null column, just was curious of the
implications.  Thanks again!

[HACKERS] Does having a NULL column automatically exclude the table from the tupleDesc cache?

[Ryan Murphy]

Hi all,

I was looking through some of the implementation details of the
heap/tuples, specifically src/include/access/htup_details.h - and I came
across the big macro fastgetattr, and had a question about it.  I've
included the code here for clarity and convenience:

#define fastgetattr(tup, attnum, tupleDesc, isnull)\
(\
AssertMacro((attnum) > 0),\
(*(isnull) = false),\
HeapTupleNoNulls(tup) ?\
(\
(tupleDesc)->attrs[(attnum)-1]->attcacheoff >= 0 ?\
(\
fetchatt((tupleDesc)->attrs[(attnum)-1],\
(char *) (tup)->t_data + (tup)->t_data->t_hoff +\
(tupleDesc)->attrs[(attnum)-1]->attcacheoff)\
)\
:\
nocachegetattr((tup), (attnum), (tupleDesc))\
)\
:\
(\
att_isnull((attnum)-1, (tup)->t_data->t_bits) ?\
(\
(*(isnull) = true),\
(Datum)NULL\
)\
:\
(\
nocachegetattr((tup), (attnum), (tupleDesc))\
)\
)\
)

My question is this:  HeapTupleNoNulls() is run first to see if there are
any columns that can be NULL.  It looks like the fetchatt() that uses the
cache in the tupleDesc can only be used if there are no NULLable columns in
the table.  Is my understanding correct?  Does this mean that there is
significant performance gain by never allowing any column to be null in
your table?

Thanks!
Ryan

Re: [HACKERS] Access inside pg_node_tree from query?

[Ryan Murphy]

>
> There are no in-core operators or functions to manipulate pg_node_tree.
>

Thanks Michael, just checking!

[HACKERS] Access inside pg_node_tree from query?

[Ryan Murphy]

Hi Postgressers,

Quick question, just curious - is there a way to access the members of a
`pg_node_tree` value within a Postgres query?  By pg_node_tree I mean for
example, the `ev_qual` field in the `pg_rewrite` table.  By "access the
members" I mean in the same way that you can access the members of a json
or jsonp value (using -> or ->>).  Is there anything analogous for
pg_node_tree?

Thanks!
Ryan

Re: [HACKERS] Re: Clarifying "server starting" messaging in pg_ctl start without --wait

[Ryan Murphy]

> The on-screen output isn't all that helpful for diagnosing what went
> wrong.  You might learn more by looking at the regression.diffs files.
> Remember that errors tend to cascade, so the first one(s) in any
> particular test suite are the most important --- the rest might just
> be fallout.
>
>
Thanks Tom, I'll check out the diffs.

Re: [HACKERS] Re: Clarifying "server starting" messaging in pg_ctl start without --wait

[Ryan Murphy]

>
> Jim Nasby said I shouldn't necessarily need to build the docs / the whole
> world in order to review patches.  But the Review form needs a `make
> installworld-check`.  Do I need to install the whole world in order to meet
> this requirement?  Happy to do so if required, but in that case, I wonder
> why 'osx' is having so much trouble parsing the SGML into XML?
>

Aha, I was able to run a "make -i install-world", which ignored the SGML
errors and built the rest of the world.  Now trying "make -i
installcheck-world".

Re: [HACKERS] Re: Clarifying "server starting" messaging in pg_ctl start without --wait

[Ryan Murphy]

>
>
> Ryan try to run 'make install-world' then 'make -i installcheck-world', -i
> option will ignore the error and proceed. You can check if any other tests
> fails. This is a separate issue, unrelated to this patch. I do not think
> we should stop from changing the status because of this.
>
>
Beena, when I try to run 'make install-world' I get a lot of errors from
the 'osx' executable which I think has to do with building the SGML
documentation:

make -C doc install
make -C src install
make -C sgml install
{ \
  echo ""; \
  echo ""; \
} > version.sgml
'/opt/local/bin/perl' ./mk_feature_tables.pl YES
../../../src/backend/catalog/sql_feature_packages.txt
../../../src/backend/catalog/sql_features.txt > features-supported.sgml
'/opt/local/bin/perl' ./mk_feature_tables.pl NO
../../../src/backend/catalog/sql_feature_packages.txt
../../../src/backend/catalog/sql_features.txt > features-unsupported.sgml
'/opt/local/bin/perl' ./generate-errcodes-table.pl
../../../src/backend/utils/errcodes.txt > errcodes-table.sgml
/usr/local/bin/osx -D. -x lower -i include-xslt-index postgres.sgml
>postgres.xmltmp
/usr/local/bin/osx:postgres.sgml:3:55:W: cannot generate system identifier
for public text "-//OASIS//DTD DocBook V4.2//EN"
/usr/local/bin/osx:postgres.sgml:12:0:E: reference to entity "BOOK" for
which no system identifier could be generated
/usr/local/bin/osx:postgres.sgml:3:0: entity was defined here
/usr/local/bin/osx:postgres.sgml:12:0:E: DTD did not contain element
declaration for document type name
/usr/local/bin/osx:postgres.sgml:14:9:E: there is no attribute "ID"
/usr/local/bin/osx:postgres.sgml:14:19:E: element "BOOK" undefined
/usr/local/bin/osx:postgres.sgml:15:7:E: element "TITLE" undefined
/usr/local/bin/osx:postgres.sgml:17:10:E: element "BOOKINFO" undefined
/usr/local/bin/osx:postgres.sgml:18:13:E: element "CORPAUTHOR" undefined
/usr/local/bin/osx:postgres.sgml:19:14:E: element "PRODUCTNAME" undefined
/usr/local/bin/osx:postgres.sgml:20:16:E: element "PRODUCTNUMBER" undefined
/usr/local/bin/osx:legal.sgml:3:5:E: element "DATE" undefined
/usr/local/bin/osx:legal.sgml:5:10:E: element "COPYRIGHT" undefined
/usr/local/bin/osx:legal.sgml:6:6:E: element "YEAR" undefined
...
/usr/local/bin/osx:history.sgml:173:13:E: element "LISTITEM" undefined
/usr/local/bin/osx:history.sgml:174:10:E: element "PARA" undefined
/usr/local/bin/osx:history.sgml:175:14:E: element "ACRONYM" undefined
/usr/local/bin/osx:I: maximum number of errors (200) reached; change with
-E option
make[3]: *** [postgres.xml] Error 1
make[2]: *** [install] Error 2
make[1]: *** [install] Error 2
make: *** [install-world-doc-recurse] Error 2


Jim Nasby said I shouldn't necessarily need to build the docs / the whole
world in order to review patches.  But the Review form needs a `make
installworld-check`.  Do I need to install the whole world in order to meet
this requirement?  Happy to do so if required, but in that case, I wonder
why 'osx' is having so much trouble parsing the SGML into XML?

Thanks for the help,
Ryan

Re: [HACKERS] Re: Clarifying "server starting" messaging in pg_ctl start without --wait

[Ryan Murphy]

Thanks for the review Beena, I'm glad the patch is ready to go!

I think because of my environment/setup, I get errors when I try "make
install-world", but I'm at work now, when I have time I will go back and
try again and figure out what is wrong.  I'll let you guys know if I have
any questions.

Take care,
Ryan

Re: [HACKERS] Adding type info etc for inheritance errmsg: "child table is missing column ..."

[Ryan Murphy]

> AFAICS, what Ryan is after would be better served by a separate tool that
> would produce a "diff" of the two tables' schemas.

Related to the other idea of seeing the problems that exist in all the
> columns (instead of one column at a time), I think it'd be reasonable to
> have a SRF that spit out everything you'd need to fix to allow inheritance
> to be added. A schema diff won't know what specifically has to match, but
> our code does.
>
>
Sure, I think that's totally true that I could just make a
Set-Returning-Function (that's what SRF stands for right?) that would
accomplish this.  I'll try that path instead for now.

Thanks guys!
Ryan

Re: [HACKERS] Adding type info etc for inheritance errmsg: "child table is missing column ..."

[Ryan Murphy]

> Perhaps the ERROR message should remain as is, and a DETAIL or HINT line
> could be emitted with the entire column definition (or close to it)?
>
>
I'm open to this option as well.  The only parts I really care about are
the type and the NOT NULL, since those are the ones that will give you an
error if it doesn't line up with the parent.  Putting it in a DETAIL or
HINT is fine with me.

Re: [HACKERS] Adding type info etc for inheritance errmsg: "child table is missing column ..."

[Ryan Murphy]

> No, and TBH I would vote strongly against including that much detail in
> this error message anyway.  That info could be indefinitely long, and it's
> not especially relevant to the stated error condition --- for example, the
> presence of a default is *not* relevant to whether the column matches the
> parent.  I'm okay with shoehorning column type into this message, but not
> much more than that.
>
> regards, tom lane
>

Ok, that makes sense.  How about things like NOT NULL? you get an error if
your column doesn't have that.

Re: [HACKERS] Adding type info etc for inheritance errmsg: "child table is missing column ..."

[Ryan Murphy]

> Thanks Tom, I'll redo the patch using one of those, and get back to you
> guys.
>
>
So I tried using format_type_with_typemod() thinking that the "typemod
info" meant things like NOT NULL, DEFAULT etc.  It builds and includes the
plain type but not all that stuff.  E.g.

user=# alter table temp inherit entity;
ERROR:  child table is missing column "id" uuid

when I was hoping for

user=# alter table temp inherit entity;
ERROR:  child table is missing column "id" uuid default uuid_generate_v1mc()

Is there an easy way to get the string that includes all those additional
constraints/defaults etc?  I noticed that defaults seem to be stored in the
Form_pg_attrdef struct defined in src/include/catalog/pg_attrdef.h, and
haven't yet seen how constraints like NOT NULL are handled.

Thanks,
Ryan

Re: [HACKERS] Adding type info etc for inheritance errmsg: "child table is missing column ..."

[Ryan Murphy]

> The approved way to do this is with format_type_be(), or
> format_type_with_typemod() if you want to include typmod info, which
> I think you probably do for the intended use-case.
>
> regards, tom lane
>

Thanks Tom, I'll redo the patch using one of those, and get back to you
guys.

[HACKERS] Adding type info etc for inheritance errmsg: "child table is missing column ..."

[Ryan Murphy]

Hey Postgres team!

I've been gleefully using the inheritance feature of Postgres for the last
6 months or so, and it's really great!  I especially like how easily you
can ALTER TABLE foo INHERIT bar, and get helpful error messages about what
columns need to be there before the inherit can take place.

One thing that seemed helpful to me is if it not only told you that you're
missing the attribute, but also told you the type, and perhaps even the
constraints, so you can easily make the column match up with the one you're
inheriting.  Otherwise you may add the wrong type of column and end up with
a "column is the wrong type" error.

The attached patch is my initial attempt at adding the type, making the
error message read e.g.:

ERROR: child table is missing column "name" text

I'm sure it needs work (in particular I borrowed a lot of the get-type-name
logic from getTypeOutputInfo() so probably needs a factor), and I'd
ultimately love for it to list NOT NULL, DEFAULTs and other constraints to
make it easier to prepare a table to inherit from another.

Thoughts / suggestions?  Does this seem useful to you guys?

Best,
Ryan


0001-child-table-is-missing-attribute-show-type.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

[HACKERS] Re: Clarifying "server starting" messaging in pg_ctl start without --wait

[Ryan Murphy]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, failed
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:tested, passed

(Though I could not check "make installcheck-world" as passed because it failed 
1 test, I think it basically SHOULD pass - see my comment below.)

Patch looks good to me and does what we talked about, and Docs seem clear and 
correct.

I was able to build Postgres and run pg_ctl and observe that it waited by 
default for the 'start' action, which addresses my original concern.

`make` and `make install` went fine, and `make check` did as well, but `make 
installcheck-world` said (after a while):

===
 1 of 55 tests failed. 
===

The diff summary is here:

*** 
/home/my_secret_local_username/my/secret/path/to/postgres/src/interfaces/ecpg/test/expected/connect-test5.stderr
2016-08-23 10:00:53.0 -0500
--- 
/home/my_secret_local_username/my/secret/path/to/postgres/src/interfaces/ecpg/test/results/connect-test5.stderr
 2017-01-06 00:08:40.0 -0600
***
*** 36,42 
  [NO_PID]: sqlca: code: 0, state: 0
  [NO_PID]: ECPGconnect: opening database  on  port  
 for user regress_ecpg_user2
  [NO_PID]: sqlca: code: 0, state: 0
! [NO_PID]: ECPGconnect: could not open database: FATAL:  database 
"regress_ecpg_user2" does not exist
  
  [NO_PID]: sqlca: code: 0, state: 0
  [NO_PID]: ecpg_finish: connection main closed
--- 36,42 
  [NO_PID]: sqlca: code: 0, state: 0
  [NO_PID]: ECPGconnect: opening database  on  port  
 for user regress_ecpg_user2
  [NO_PID]: sqlca: code: 0, state: 0
! [NO_PID]: ECPGconnect: could not open database: FATAL:  database 
"my_secret_local_username" does not exist
  
  [NO_PID]: sqlca: code: 0, state: 0
  [NO_PID]: ecpg_finish: connection main closed
***
*** 73,79 
  [NO_PID]: sqlca: code: -220, state: 08003
  [NO_PID]: ECPGconnect: opening database  on  port  
 for user regress_ecpg_user2
  [NO_PID]: sqlca: code: 0, state: 0
! [NO_PID]: ECPGconnect: could not open database: FATAL:  database 
"regress_ecpg_user2" does not exist
  
  [NO_PID]: sqlca: code: 0, state: 0
  [NO_PID]: ecpg_finish: connection main closed
--- 73,79 
  [NO_PID]: sqlca: code: -220, state: 08003
  [NO_PID]: ECPGconnect: opening database  on  port  
 for user regress_ecpg_user2
  [NO_PID]: sqlca: code: 0, state: 0
! [NO_PID]: ECPGconnect: could not open database: FATAL:  database 
"my_secret_local_username" does not exist
  
  [NO_PID]: sqlca: code: 0, state: 0
  [NO_PID]: ecpg_finish: connection main closed

==


I don't actually believe this to indicate a problem though - I think perhaps 
there's a problem with this test, or with how I am running it.  The only diff 
was that when it (correctly) complained of a nonexistent database, it referred 
to my username that I was logged in as, instead of the test database name 
"regress_ecpg_user2".  I don't think this has anything to do with the changes 
to pg_ctl.

I could be wrong though!  I am going to leave this as "Needs review" until 
someone more familiar with the project double-checks this.
-- 
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] Clarifying "server starting" messaging in pg_ctl start without --wait

[Ryan Murphy]

> Making --wait the default may or may not be sensible -- I'm not sure
> -- but removing --no-wait is clearly a bad idea, and we shouldn't do
> it.  The fact that the problems created by removing it might be
> solvable doesn't mean that it's a good idea to create them in the
> first place.
>
>
>
I agree with Robert - pg_ctl is no doubt used in all kinds of scripts that
would then have to change.
It may make sense to have --wait be the default though - certainly less
confusing to new users!

[HACKERS] Clarifying "server starting" messaging in pg_ctl start without --wait

[Ryan Murphy]

Hi Postgres Devs,

I had a suggestion regarding the output pg_ctl gives when you use it to
start the postgres server.  At first I was going to write a patch, but then
I decided to just ask you guys first to see what you think.

I had an issue earlier where I was trying to upgrade my postgres database
to a new major version and incidentally a new pg_catalog version, and
therefore the new code could no longer run the existing data directory
without pg_upgrade or pg_dump (I ended up needing pg_dump).  Initially I
was very confused because I tried running "pg_ctl -D datadir -l logfile
start" like normal, and it just said "server starting", yet the server was
not starting.  It took me a while to realize that I needed to use the
"--wait" / "-w" option to actually wait and test whether the server was
really starting, at which point it told me there was a problem and to check
the log.

I'm concerned some new users may not understand this behavior of pg_ctl, so
I wanted to suggest that we add some additional messaging after "server
starting" - something like:

$ pg_ctl -D datadir -l logfile start
server starting
(to wait for confirmation that server actually started, try pg_ctl again
with --wait)

What do you guys think?  Is it important to keep pg_ctl output more terse
than this?  I do think something like this could help new users avoid
frustration.

I'm happy to write a patch for this if it's helpful, though it's such a
simple change that if one of the core devs wants this s/he can probably
more easily just add it themselves.

Cheers,
Ryan

Re: [HACKERS] Trouble building uuid-ossp extension in new versions of Postgres

[Ryan Murphy]

>
> On Mac, the recommended thing is to forget about the ossp code and
>> use "configure --with-uuid=e2fs".  Sorry if that wasn't clear enough.
>>
>>
> Ok thanks, I'll try this.
>

Thanks Tom, "uuid-ossp" built perfectly with "--with--uuid=e2fs".

Cheers and Happy Holidays!
Ryan

Re: [HACKERS] Trouble building uuid-ossp extension in new versions of Postgres

[Ryan Murphy]

> On Mac, the recommended thing is to forget about the ossp code and
> use "configure --with-uuid=e2fs".  Sorry if that wasn't clear enough.
>
>
Ok thanks, I'll try this.


> Reading over your post again, it sounds like you're trying to force-build
> contrib/uuid-ossp without having used any of the configure options that
> are supposed to enable it.  I'm not sure that would ever have worked very
> reliably.
>
>
Ok, that makes sense.  I had never learned the proper procedure for
building extensions, but have had success with going into e.g.
contrib/pgcrypto and typing make to build the pgcrypto extension.  Since
things like that have generally worked for me I didn't learn about the
proper flags etc.

I'll try some more things and get back to you, thanks for the help.

Best,
Ryan

[HACKERS] Trouble building uuid-ossp extension in new versions of Postgres

[Ryan Murphy]

Hello,

I have been trying to build Postgres and migrate my data to the newest
version.  Postgres builds just fine, but I also need the uuid-ossp module,
which used to build fine for me and now does not...

I am currently "git pull"ed to commit b645a05fc6112a4857ceac574d4aa2
4174a70417.

I cd into /contrib/uuid-ossp, and type "make" and this happens
(on Mac, see compiler version below):

$ make
gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
-Wformat-security -fno-strict-aliasing -fwrapv
-Wno-unused-command-line-argument
-O2  -I../../contrib/pgcrypto -I. -I. -I../../src/include   -c -o
uuid-ossp.o uuid-ossp.c

uuid-ossp.c:282:23: error: use of undeclared identifier 'uuid_s_ok'
uint32_tstatus = uuid_s_ok;
 ^
uuid-ossp.c:285:5: warning: implicit declaration of function 'uuid_create'
is invalid in C99 [-Wimplicit-function-declaration]
uuid_create(, );
^
uuid-ossp.c:287:19: error: use of undeclared identifier 'uuid_s_ok'
if (status == uuid_s_ok)
  ^
uuid-ossp.c:289:6: warning: implicit declaration of function
'uuid_to_string' is invalid in C99 [-Wimplicit-function-declaration]
uuid_to_string(, , );
^
uuid-ossp.c:290:20: error: use of undeclared identifier 'uuid_s_ok'
if (status == uuid_s_ok)
  ^
uuid-ossp.c:306:19: error: use of undeclared identifier 'uuid_s_ok'
if (status != uuid_s_ok)

...


I can post the rest of the errors if needed but it just keeps going,
eventually erroring out with "Too many errors".  Indeed I do not see the
identifier 'uuid_s_ok' defined anywhere within the contrib/uuid-ossp tree.

The code that is failing to build dates to Tom Lane's commit
b8cc8f94730610c0189aa82dfec4ae6ce9b13e34 in which he is apparently creating
an abstraction layer for uuid-ossp to be built with any of 3 different
backends.  I was looking for documentation about how to choose a backend /
more details on how to build this extension now, but drawing a blank.

Again I am on a Mac, this is my compiler info:

$ clang -v
Apple LLVM version 7.0.2 (clang-700.1.79)
Target: x86_64-apple-darwin14.0.0
Thread model: posix

Thanks much!
Ryan

Re: [HACKERS] proposal: psql \setfileref

[Ryan Murphy]

>
> please, can you check attached patch? It worked in my laptop.
>
> Regards
>
> Pavel
>
>
Yes, that one applied for me without any problems.

Thanks,
Ryan

Re: [HACKERS] proposal: psql \setfileref

[Ryan Murphy]

Hi Pavel,

I just tried to apply your patch psql-setfileref-initial.patch (using git 
apply) to the newest revision of postgres at the time (da6c4f6ca88) and it 
failed to patch startup.c.  Thinking that the patch was for some previous 
revision, I then checked out d062245b5, which was from 2016-08-31, and tried 
applying the patch from there.  I had the same problem:

$ git apply psql-setfileref-initial.patch
error: patch failed: src/bin/psql/startup.c:106
error: src/bin/psql/startup.c: patch does not apply

However, when I applied the changes to startup.c manually and removed them from 
the patch, the rest of the patch applied without a problem.  I don't know if I 
may have done something wrong in trying to apply the patch, but you may want to 
double check if you need to regenerate your patch from the latest revision so 
it will apply smoothly for reviewers.

In the meantime, I haven't had a chance to try out the fileref feature yet but 
I'll give it a go when I get a chance!

Thanks!
Ryan
-- 
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: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

Thanks for committing it Tom!  Thank you all for your help.

I really like the Postgres project!  If there's anything that especially
needs worked on let me know, I'd love to help.

Best,
Ryan

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

>
> I looked into this and soon found that fe_utils/string_utils.o has
> dependencies on libpq that are much wider than just pqexpbuffer :-(.
> It might be a project to think about sorting that out sometime, but it
> looks like it would be an awful lot of work just to avoid having initdb
> depend on libpq.so.  So I now think this objection is impractical.
> I'll just annotate the makefile entry to say that initdb itself doesn't
> use libpq.


Sounds good.


>
> > Another perhaps-only-cosmetic issue is that now initdb prints quotes
> > whether they are needed or not.
>
> I still think this is worth fixing, but it's a simple modification.
> Will take care of it.
>
> Great!  Seems like a good solution, I agree it looks better without quotes
if they're not needed.


> This item is listed in the commitfest as a bug fix, but given the lack of
> previous complaints, and the fact that the printed command isn't really
> meant to be blindly copied-and-pasted anyway (you're at least meant to
> replace "logfile" with something), I do not think it needs back-patching.
>
>
Agreed, not a "bug" in that sense.
Thanks Tom.


> regards, tom lane
>

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

On Sat, Aug 20, 2016 at 8:26 AM, Michael Paquier <michael.paqu...@gmail.com>
wrote:

> On Sat, Aug 20, 2016 at 4:39 AM, Ryan Murphy <ryanfmur...@gmail.com>
> wrote:
> > Here is another version of my initdb shell quoting patch.  I have removed
> > the unnecessary {} block.  I also ran pgindent on the code prior to
> creating
> > the patch.
>
> Could you please *not* top-post? This breaks the logic of the thread,
> this is the third time that I mention it, and that's not the style of
> this mailing list.
>
>
Sure, sorry about that.  Am not used to this style of mailing list.  I had
been avoiding top posting since you first mentioned it but then with the
new patch I didn't know if that was still supposed to go at the bottom.


> Regarding your patch, with a bit of clean up it gives the attached.
> You should declare variables at the beginning of a code block or
> function. One call to appendPQExpBufferStr can as well be avoided, and
> you added too many newlines. I have switched that as ready for
> committer.
> --
> Michael
>

Thanks Michael, I've looked at your changes and everything looks good to me.

I did notice the commit message is gone etc., is that not part of the
patch?  Perhaps the committer prefers to write their own message, that's
fine too.

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

Here is another version of my initdb shell quoting patch.  I have removed
the unnecessary {} block.  I also ran pgindent on the code prior to
creating the patch.

On Thu, Aug 18, 2016 at 3:50 PM, Ryan Murphy <ryanfmur...@gmail.com> wrote:

>
>
> On Thu, Aug 18, 2016 at 3:44 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>
>> Ryan Murphy <ryanfmur...@gmail.com> writes:
>> > On Thu, Aug 18, 2016 at 3:22 PM, Robert Haas <robertmh...@gmail.com>
>> wrote:
>> >>>> +{ /* pg_ctl command w path, properly quoted */
>> >>>> +PQExpBuffer pg_ctl_path = createPQExpBuffer();
>> >>>> +printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl",
>>
>> >> I think it's worth reducing the scope of variables when that's as
>> >> simple as putting them inside a block that you have to create anyway,
>> >> but I'm skeptical about the idea that one would create a block just to
>> >> reduce the scope of the variables.  I don't think that's our usual
>> >> practice, and I would expect the compiler to detect where the variable
>> >> is referenced first and last anyway.
>>
>> > I enjoy adding the blocks for explicit variable scoping and for quick
>> > navigation in vim (the % key navigates between matching {}'s).  But I
>> want
>> > to fit in with the style conventions of the project.
>>
>> Another point here is that code like this will look quite a bit different
>> after pgindent gets done with it --- that comment will not stay where
>> you put it, for example.  Some of our worst formatting messes come from
>> code wherein somebody adhered to their own favorite layout style without
>> any thought for how it would play with pgindent.
>>
>> regards, tom lane
>>
>
> Ahh, I didn't know about pgindent, but now I see it in /src/tools.  I can
> run that on my code before submitting.
>
> I found these
> <https://www.postgresql.org/message-id/1221125165.5637.12.camel@abbas-laptop>
> links <https://www.postgresql.org/docs/devel/static/source-format.html>
> about the style convention and will make sure my patch fits the conventions
> before submitting it.
>
>


0001-initdb-quote-shell-args-in-final-pg_ctl-command.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: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

On Thu, Aug 18, 2016 at 3:44 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Ryan Murphy <ryanfmur...@gmail.com> writes:
> > On Thu, Aug 18, 2016 at 3:22 PM, Robert Haas <robertmh...@gmail.com>
> wrote:
> >>>> +{ /* pg_ctl command w path, properly quoted */
> >>>> +PQExpBuffer pg_ctl_path = createPQExpBuffer();
> >>>> +printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl",
>
> >> I think it's worth reducing the scope of variables when that's as
> >> simple as putting them inside a block that you have to create anyway,
> >> but I'm skeptical about the idea that one would create a block just to
> >> reduce the scope of the variables.  I don't think that's our usual
> >> practice, and I would expect the compiler to detect where the variable
> >> is referenced first and last anyway.
>
> > I enjoy adding the blocks for explicit variable scoping and for quick
> > navigation in vim (the % key navigates between matching {}'s).  But I
> want
> > to fit in with the style conventions of the project.
>
> Another point here is that code like this will look quite a bit different
> after pgindent gets done with it --- that comment will not stay where
> you put it, for example.  Some of our worst formatting messes come from
> code wherein somebody adhered to their own favorite layout style without
> any thought for how it would play with pgindent.
>
> regards, tom lane
>

Ahh, I didn't know about pgindent, but now I see it in /src/tools.  I can
run that on my code before submitting.

I found these
<https://www.postgresql.org/message-id/1221125165.5637.12.camel@abbas-laptop>
links <https://www.postgresql.org/docs/devel/static/source-format.html>
about the style convention and will make sure my patch fits the conventions
before submitting it.

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

On Thu, Aug 18, 2016 at 3:22 PM, Robert Haas  wrote:

> On Thu, Aug 18, 2016 at 4:15 PM, Andres Freund  wrote:
> > On 2016-08-18 16:11:27 -0400, Robert Haas wrote:
> >> On Wed, Aug 17, 2016 at 11:18 PM, Andres Freund 
> wrote:
> >> > On August 17, 2016 8:15:56 PM PDT, Michael Paquier <
> michael.paqu...@gmail.com> wrote:
> >> >
> >> >>+{ /* pg_ctl command w path, properly quoted */
> >> >>+PQExpBuffer pg_ctl_path = createPQExpBuffer();
> >> >>+printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl",
> >> >>+bin_dir,
> >> >>+(strlen(bin_dir) > 0) ? DIR_SEP : ""
> >> >>+);
> >> >>+appendShellString(start_db_cmd, pg_ctl_path->data);
> >> >>+destroyPQExpBuffer(pg_ctl_path);
> >> >>+}
> >> >>
> >> >>This is not really project-style to have an independent block. Usually
> >> >>those are controlled by for, while or if.
> >> >
> >> > Besides the comment positioning I'd not say that that is against the
> usual style, there's a number of such blocks already.  Don't think it's
> necessarily needed here though...
> >>
> >> Really?  I'd remove such blocks before committing anything, or ask for
> >> them to be removed, unless there were some special reason for having
> >> them.
> >
> > Well, reducing the scope of variables *can* be such a reason, no? As I
> > said, I don't see any reason here, but in general, it's imo a reasonable
> > tool on one's belt.
>
> I think it's worth reducing the scope of variables when that's as
> simple as putting them inside a block that you have to create anyway,
> but I'm skeptical about the idea that one would create a block just to
> reduce the scope of the variables.  I don't think that's our usual
> practice, and I would expect the compiler to detect where the variable
> is referenced first and last anyway.
>
>
I'm can change my patch to take out that block.

I enjoy adding the blocks for explicit variable scoping and for quick
navigation in vim (the % key navigates between matching {}'s).  But I want
to fit in with the style conventions of the project.

Before I change and resubmit my patch, are there any other changes, style
or otherwise, that you all would recommend?


> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

> I think that's actually a good thing to forbid.

I think I agree Andres, there are already comments in the appendShellString
function to this effect - they say that CR/LF chars in a file name are
mostly used for malicious hacking attempts anyways - I know I've hardly
ever needed a newline in a file name.

Did you see anything else in my code that you have recommendations about?
I made sure to free the PQExpBufferStr vars that I allocated.

Best,
Ryan

On Wed, Aug 17, 2016 at 7:41 PM, Andres Freund <and...@anarazel.de> wrote:

> On 2016-08-18 09:14:44 +0900, Michael Paquier wrote:
> > On Thu, Aug 18, 2016 at 12:21 AM, Ryan Murphy <ryanfmur...@gmail.com>
> wrote:
> > > I have created a better patch (attached) that correctly escapes the
> shell
> > > arguments using PQExpBufferStr and the appendShellString function, as
> per
> > > Michael and Andres' suggestions.
> > >
> > > Further suggestions welcome of course.
> >
> > As far as I know, it is perfectly possible to have LF/CR in a path
> > name (that's bad practice btw...), and your patch would make initdb
> > fail in such cases. Do we want to authorize that?
>
> I think that's actually a good thing to forbid.
>

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

That's a fair point Michael.  I would be willing to make such a change, but
since c doesn't have optional function arguments I'm not sure the least
intrusive way to do that.  Do you have a suggestion?

On Wednesday, August 17, 2016, Michael Paquier <michael.paqu...@gmail.com>
wrote:

> On Thu, Aug 18, 2016 at 12:21 AM, Ryan Murphy <ryanfmur...@gmail.com
> <javascript:;>> wrote:
> > I have created a better patch (attached) that correctly escapes the shell
> > arguments using PQExpBufferStr and the appendShellString function, as per
> > Michael and Andres' suggestions.
> >
> > Further suggestions welcome of course.
>
> As far as I know, it is perfectly possible to have LF/CR in a path
> name (that's bad practice btw...), and your patch would make initdb
> fail in such cases. Do we want to authorize that? If we bypass the
> error checks in appendShellString with an extra option, and have
> initdb use that, the generated command would be actually correct.
> --
> Michael
>

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

I have created a better patch (attached) that correctly escapes the shell
arguments using PQExpBufferStr and the appendShellString function, as per
Michael and Andres' suggestions.

Further suggestions welcome of course.

Ryan

On Wed, Aug 17, 2016 at 8:28 AM, Ryan Murphy <ryanfmur...@gmail.com> wrote:

> That makes sense, Michael and Andres.
>
> I started to make a solution that uses a PQExpBuffer, appendShellString,
> etc.  I think it will work just fine, but I think I need to alter the
> Makefile as well, to get initdb.c to be compiled using
> -L../../../src/fe_utils -lpgfeutils.  Otherwise I am having issues linking:
>
> gcc -Wall -Wmissing-prototypes -Wpointer-arith
> -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
> -Wformat-security -fno-strict-aliasing -fwrapv 
> -Wno-unused-command-line-argument
> -O2 initdb.o findtimezone.o localtime.o encnames.o  -L../../../src/port
> -L../../../src/common -Wl,-dead_strip_dylibs   -lpgcommon -lpgport -lz
> -lreadline -lm  -o initdb
> Undefined symbols for architecture x86_64:
>   "_appendPQExpBufferStr", referenced from:
>   _main in initdb.o
>   "_appendShellString", referenced from:
>   _main in initdb.o
>   "_createPQExpBuffer", referenced from:
>   _main in initdb.o
>   "_destroyPQExpBuffer", referenced from:
>   _main in initdb.o
> ld: symbol(s) not found for architecture x86_64
> clang: error: linker command failed with exit code 1 (use -v to see
> invocation)
>
> On Tue, Aug 16, 2016 at 10:00 PM, Michael Paquier <
> michael.paqu...@gmail.com> wrote:
>
>> On Wed, Aug 17, 2016 at 8:05 AM, Andres Freund <and...@anarazel.de>
>> wrote:
>> > ISTM that the correct fix would be to actually introduce something like
>> > quote_path_for_shell() which either adds proper quotes, or fails if
>> > that'd be hard (e.g. if the path contains quotes, and we're on
>> > windows).
>>
>> You are looking for appendShellString in fe_utils/string_utils.c.
>> --
>> Michael
>>
>
>


0001-initdb-quote-shell-args-in-final-pg_ctl-command.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: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

That makes sense, Michael and Andres.

I started to make a solution that uses a PQExpBuffer, appendShellString,
etc.  I think it will work just fine, but I think I need to alter the
Makefile as well, to get initdb.c to be compiled using
-L../../../src/fe_utils -lpgfeutils.  Otherwise I am having issues linking:

gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
-Wformat-security -fno-strict-aliasing -fwrapv
-Wno-unused-command-line-argument -O2 initdb.o findtimezone.o localtime.o
encnames.o  -L../../../src/port -L../../../src/common
-Wl,-dead_strip_dylibs   -lpgcommon -lpgport -lz -lreadline -lm  -o initdb
Undefined symbols for architecture x86_64:
  "_appendPQExpBufferStr", referenced from:
  _main in initdb.o
  "_appendShellString", referenced from:
  _main in initdb.o
  "_createPQExpBuffer", referenced from:
  _main in initdb.o
  "_destroyPQExpBuffer", referenced from:
  _main in initdb.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see
invocation)

On Tue, Aug 16, 2016 at 10:00 PM, Michael Paquier  wrote:

> On Wed, Aug 17, 2016 at 8:05 AM, Andres Freund  wrote:
> > ISTM that the correct fix would be to actually introduce something like
> > quote_path_for_shell() which either adds proper quotes, or fails if
> > that'd be hard (e.g. if the path contains quotes, and we're on
> > windows).
>
> You are looking for appendShellString in fe_utils/string_utils.c.
> --
> Michael
>

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

I've submitted my patch to Commitfest 2016-09.

https://commitfest.postgresql.org/10/718/

My username on postgresql.org is murftown

On Tue, Aug 16, 2016 at 1:02 AM, Ryan Murphy <ryanfmur...@gmail.com> wrote:

> Ok, I'll do that!
> Thanks Michael!
> Ryan
>
>
> On Monday, August 15, 2016, Michael Paquier <michael.paqu...@gmail.com>
> wrote:
>
>> On Mon, Aug 15, 2016 at 12:12 AM, Ryan Murphy <ryanfmur...@gmail.com>
>> wrote:
>> > This is to fix an issue that came up for me when running initdb.
>> >
>> > At the end of a successful initdb it says:
>> >
>> > Success. You can now start the database server using:
>> > pg_ctl -D /some/path/to/data -l logfile start
>> >
>> > but this command did not work for me because my data directory
>> > contained a space.  The src/bin/initdb/initdb.c source code
>> > did already have a QUOTE_PATH constant, but it was the empty
>> > string for non-windows cases.
>> >
>> > Therefore, added quotes via existing QUOTE_PATH constant:
>> >
>> > pg_ctl -D '/some/path/to/data' -l logfile start
>>
>> You may want to register this patch to the next commit fest:
>> https://commitfest.postgresql.org/10/
>> --
>> Michael
>>
>

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

Ok, I'll do that!
Thanks Michael!
Ryan

On Monday, August 15, 2016, Michael Paquier <michael.paqu...@gmail.com>
wrote:

> On Mon, Aug 15, 2016 at 12:12 AM, Ryan Murphy <ryanfmur...@gmail.com
> <javascript:;>> wrote:
> > This is to fix an issue that came up for me when running initdb.
> >
> > At the end of a successful initdb it says:
> >
> > Success. You can now start the database server using:
> > pg_ctl -D /some/path/to/data -l logfile start
> >
> > but this command did not work for me because my data directory
> > contained a space.  The src/bin/initdb/initdb.c source code
> > did already have a QUOTE_PATH constant, but it was the empty
> > string for non-windows cases.
> >
> > Therefore, added quotes via existing QUOTE_PATH constant:
> >
> > pg_ctl -D '/some/path/to/data' -l logfile start
>
> You may want to register this patch to the next commit fest:
> https://commitfest.postgresql.org/10/
> --
> Michael
>

[HACKERS] Small patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

Hello Postgres Hackers!

I sent this already a few hours ago but it got blocked because I had not
yet joined the mailing list.  Trying again, sorry for any redundancy or
confusion!

This is to fix an issue that came up for me when running initdb.

At the end of a successful initdb it says:

Success. You can now start the database server using:
pg_ctl -D /some/path/to/data -l logfile start

but this command did not work for me because my data directory
contained a space.  The src/bin/initdb/initdb.c source code
did already have a QUOTE_PATH constant, but it was the empty
string for non-windows cases.

Therefore, added quotes via existing QUOTE_PATH constant:

pg_ctl -D '/some/path/to/data' -l logfile start

Best,
Ryan


0001-initdb-for-QUOTE_PATH-non-windows.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

[HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

Hello Postgres Team!

This is to fix an issue that came up for me when running initdb.

At the end of a successful initdb it says:

Success. You can now start the database server using:
pg_ctl -D /some/path/to/data -l logfile start

but this command did not work for me because my data directory
contained a space.  The src/bin/initdb/initdb.c source code
did already have a QUOTE_PATH constant, but it was the empty
string for non-windows cases.

Therefore, added quotes via existing QUOTE_PATH constant:

pg_ctl -D '/some/path/to/data' -l logfile start


0001-initdb-for-QUOTE_PATH-non-windows.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