Thread: More problem with scripts
Look at this: $ dropdb lijasdf oiuwqe testDROP DATABASE The create/drop scripts only process the last arguments, ignoring earlier ones. I assume no one wants me to fix it now so I will add this to TODO: * Prevent create/drop scripts from allowing extra args (Bruce) -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
[2002-01-03 13:12] Bruce Momjian said:
| Look at this:
|
| $ dropdb lijasdf oiuwqe test
| DROP DATABASE
|
| The create/drop scripts only process the last arguments, ignoring
| earlier ones. I assume no one wants me to fix it now so I will add this
| to TODO:
something /simple/ might look like.
Index: dropdb
===================================================================
RCS file: /var/cvsup/pgsql/src/bin/scripts/dropdb,v
retrieving revision 1.13
diff -c -r1.13 dropdb
*** dropdb 30 Sep 2001 22:17:51 -0000 1.13
--- dropdb 3 Jan 2002 18:54:17 -0000
***************
*** 88,94 **** exit 1 ;; *)
! dbname="$1" ;; esac shift
--- 88,95 ---- exit 1 ;; *)
! [ ! -z "$dbname" ] && usage=1
! dbname=$1 ;; esac shift
***************
*** 132,137 ****
--- 133,141 ---- dbname=`echo $dbname | sed 's/\"/\\\"/g'`
+
+ echo $dbname
+ exit 0; ${PATHNAME}psql $PSQLOPT -d template1 -c "DROP DATABASE \"$dbname\"" if [ "$?" -ne 0 ]; then
--
"Develop your talent, man, and leave the world something. Records are
really gifts from people. To think that an artist would love you enough
to share his music with anyone is a beautiful thing." -- Duane Allman
Actually, we can just do:
> *)
> dbname=$1
> [ "$#" -ne 1 ] && usage=1
Meaning if they have anything after the dbname, it is an error. This
catches flags _after_ the dbname. Seems most of the script have this
problem. If people want it fixed, I can easily do it; just give me to
go-ahead.
---------------------------------------------------------------------------
Brent Verner wrote:
> [2002-01-03 13:12] Bruce Momjian said:
> | Look at this:
> |
> | $ dropdb lijasdf oiuwqe test
> | DROP DATABASE
> |
> | The create/drop scripts only process the last arguments, ignoring
> | earlier ones. I assume no one wants me to fix it now so I will add this
> | to TODO:
>
> something /simple/ might look like.
>
> Index: dropdb
> ===================================================================
> RCS file: /var/cvsup/pgsql/src/bin/scripts/dropdb,v
> retrieving revision 1.13
> diff -c -r1.13 dropdb
> *** dropdb 30 Sep 2001 22:17:51 -0000 1.13
> --- dropdb 3 Jan 2002 18:54:17 -0000
> ***************
> *** 88,94 ****
> exit 1
> ;;
> *)
> ! dbname="$1"
> ;;
> esac
> shift
> --- 88,95 ----
> exit 1
> ;;
> *)
> ! [ ! -z "$dbname" ] && usage=1
> ! dbname=$1
> ;;
> esac
> shift
> ***************
> *** 132,137 ****
> --- 133,141 ----
>
>
> dbname=`echo $dbname | sed 's/\"/\\\"/g'`
> +
> + echo $dbname
> + exit 0;
>
> ${PATHNAME}psql $PSQLOPT -d template1 -c "DROP DATABASE \"$dbname\""
> if [ "$?" -ne 0 ]; then
>
> --
> "Develop your talent, man, and leave the world something. Records are
> really gifts from people. To think that an artist would love you enough
> to share his music with anyone is a beautiful thing." -- Duane Allman
>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/users-lounge/docs/faq.html
>
-- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610)
853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill,
Pennsylvania19026
[2002-01-03 13:56] Brent Verner said: | [2002-01-03 13:12] Bruce Momjian said: | | Look at this: | | | | $ dropdb lijasdf oiuwqe test | | DROP DATABASE | | | | The create/drop scripts only process the last arguments, ignoring | | earlier ones. I assume no one wants me to fix it now so I will add this | | to TODO: | | something /simple/ might look like. | | Index: dropdb | =================================================================== | RCS file: /var/cvsup/pgsql/src/bin/scripts/dropdb,v[snip] | + | + echo $dbname | + exit 0; er, minus this debugging cruft :-) sorry, b -- "Develop your talent, man, and leave the world something. Records are really gifts from people. To think that an artist would love you enough to share his music with anyone is a beautiful thing." -- Duane Allman
[2002-01-03 14:19] Bruce Momjian said: | | Actually, we can just do: | | > *) | > dbname=$1 | > [ "$#" -ne 1 ] && usage=1 | | Meaning if they have anything after the dbname, it is an error. This | catches flags _after_ the dbname. Seems most of the script have this | problem. If people want it fixed, I can easily do it; just give me to | go-ahead. +1 I can't see a reason to /not/ fix something this simple for the 7.2 release. In general, I think it's best to fix things like this[1] "on sight" as opposed to queueing them in TODO where they /might/ sit untouched through another release cycle. [1] meaning problems that require little effort to fix, and whose solutions are /very/ localized. cheers. brent -- "Develop your talent, man, and leave the world something. Records are really gifts from people. To think that an artist would love you enough to share his music with anyone is a beautiful thing." -- Duane Allman
Brent Verner wrote: > [2002-01-03 14:19] Bruce Momjian said: > | > | Actually, we can just do: > | > | > *) > | > dbname=$1 > | > [ "$#" -ne 1 ] && usage=1 > | > | Meaning if they have anything after the dbname, it is an error. This > | catches flags _after_ the dbname. Seems most of the script have this > | problem. If people want it fixed, I can easily do it; just give me to > | go-ahead. > > +1 > > I can't see a reason to /not/ fix something this simple for the 7.2 > release. In general, I think it's best to fix things like this[1] > "on sight" as opposed to queueing them in TODO where they /might/ sit > untouched through another release cycle. > > [1] meaning problems that require little effort to fix, and whose > solutions are /very/ localized. OK, one more +1 and I will get to it. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
>> I can't see a reason to /not/ fix something this simple for the 7.2
>> release.
Seems like a safe fix to me too.
regards, tom lane
> Brent Verner wrote: > > [2002-01-03 14:19] Bruce Momjian said: > > | > > | Actually, we can just do: > > | > > | > *) > > | > dbname=$1 > > | > [ "$#" -ne 1 ] && usage=1 > > | > > | Meaning if they have anything after the dbname, it is an error. This > > | catches flags _after_ the dbname. Seems most of the script have this > > | problem. If people want it fixed, I can easily do it; just give me > to > > | go-ahead. > > > > +1 > > > > I can't see a reason to /not/ fix something this simple for the 7.2 > > release. In general, I think it's best to fix things like this[1] > > "on sight" as opposed to queueing them in TODO where they /might/ sit > > untouched through another release cycle. > > > > [1] meaning problems that require little effort to fix, and whose > > solutions are /very/ localized. > > OK, one more +1 and I will get to it. -4 1: It's not a regression from 7.1. Anything else is too late. 2: The issue does not cause problems if you stick to the documented syntax and it does not cause hazard if you don't. 3: The patch is wrong, because showing the usage screen in case of an error is inappropriate. 4: Even beginning to talk of "localized", "trivial", "little effort" should cause an automatic ban on programming for 1 month. -- GMX - Die Kommunikationsplattform im Internet. http://www.gmx.net
...
> 4: Even beginning to talk of "localized", "trivial", "little effort" should
> cause an automatic ban on programming for 1 month.
Hey, this a new year of optimism and hope. Save the realism until summer
or fall at least :)
- Thomas
[2002-01-03 23:33] Peter Eisentraut said: | > Brent Verner wrote: | > > [2002-01-03 14:19] Bruce Momjian said: | > > | | > > | Actually, we can just do: | > > +1 | > > | > > I can't see a reason to /not/ fix something this simple for the 7.2 | > > release. In general, I think it's best to fix things like this[1] | > > "on sight" as opposed to queueing them in TODO where they /might/ sit | > > untouched through another release cycle. | > > | > > [1] meaning problems that require little effort to fix, and whose | > > solutions are /very/ localized. | > | > OK, one more +1 and I will get to it. | | -4 Hey, no ballot stuffing ;-) | 1: It's not a regression from 7.1. Anything else is too late. IMO, this is not a valid reason to /not/ fix _this problem_. Taken to an extreme, this reasoning would allow any number of fatal bugs to remain in 7.2 because they also existed in 7.1. Given the imminent release, this position is valid even for a number of non-fatal bugs, but not this one. | 2: The issue does not cause problems if you stick to the documented syntax | and it | does not cause hazard if you don't. First half correct. It is arguable whether or not the following would be considered a hazard or not. Is unexpected behavior hazardous? $ createdb whatiwanted whatigot | 3: The patch is wrong, because showing the usage screen in case of an error | is | inappropriate. Perhaps a more suitable message similar to the "invalid option" error should be printed prior to exit, but this is not as important as enforcing proper/documented use of the script(s). cheers. brent -- "Develop your talent, man, and leave the world something. Records are really gifts from people. To think that an artist would love you enough to share his music with anyone is a beautiful thing." -- Duane Allman
Brent Verner wrote:
> [2002-01-03 23:33] Peter Eisentraut said:
> | > Brent Verner wrote:
> | > > [2002-01-03 14:19] Bruce Momjian said:
> | > > |
> | > > | Actually, we can just do:
>
> | > > +1
> | > >
> | > > I can't see a reason to /not/ fix something this simple for the 7.2
> | > > release. In general, I think it's best to fix things like this[1]
> | > > "on sight" as opposed to queueing them in TODO where they /might/ sit
> | > > untouched through another release cycle.
> | > >
> | > > [1] meaning problems that require little effort to fix, and whose
> | > > solutions are /very/ localized.
> | >
> | > OK, one more +1 and I will get to it.
> |
> | -4
OK, here's the patch. Seems createdb wasn't properly handling the db
comment (arg code now similar to createlang), createlang dbname being
optional wasn't documented in --help, and vacuumdb wasn't handlling an
optional dbname. I added the required checks so extra arguments report
a failure:
$ dropdb lkjas asdfljk test
dropdb: invalid option: asdfljk
Try 'dropdb --help' for more information.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Index: doc/src/sgml/ref/vacuumdb.sgml
===================================================================
RCS file: /cvsroot/pgsql/doc/src/sgml/ref/vacuumdb.sgml,v
retrieving revision 1.20
diff -c -r1.20 vacuumdb.sgml
*** doc/src/sgml/ref/vacuumdb.sgml 2001/12/08 03:24:40 1.20
--- doc/src/sgml/ref/vacuumdb.sgml 2002/01/04 05:23:04
***************
*** 23,33 ****
<cmdsynopsis>
<command>vacuumdb</command>
<arg rep="repeat"><replaceable>connection-options</replaceable></arg>
! <arg><arg>-d</arg> <replaceable>dbname</replaceable></arg>
<group><arg>--full</arg><arg>-f</arg></group>
<group><arg>--verbose</arg><arg>-v</arg></group>
<group><arg>--analyze</arg><arg>-z</arg></group>
! <arg>--table '<replaceable>table</replaceable>
<arg>( <replaceable class="parameter">column</replaceable> [,...] )</arg>'
</arg>
<sbr>
--- 23,33 ----
<cmdsynopsis>
<command>vacuumdb</command>
<arg rep="repeat"><replaceable>connection-options</replaceable></arg>
! <arg><replaceable>dbname</replaceable></arg>
<group><arg>--full</arg><arg>-f</arg></group>
<group><arg>--verbose</arg><arg>-v</arg></group>
<group><arg>--analyze</arg><arg>-z</arg></group>
! <arg>--table | -t '<replaceable>table</replaceable>
<arg>( <replaceable class="parameter">column</replaceable> [,...] )</arg>'
</arg>
<sbr>
Index: src/bin/scripts/createdb
===================================================================
RCS file: /cvsroot/pgsql/src/bin/scripts/createdb,v
retrieving revision 1.18
diff -c -r1.18 createdb
*** src/bin/scripts/createdb 2001/09/30 22:17:51 1.18
--- src/bin/scripts/createdb 2002/01/04 05:23:05
***************
*** 104,114 ****
exit 1
;;
*)
! if [ -z "$dbname" ]; then
! dbname="$1"
! else
dbcomment="$1"
fi
;;
esac
shift
--- 104,120 ----
exit 1
;;
*)
! dbname="$1"
! if [ "$2" ]
! then
! shift
dbcomment="$1"
fi
+ if [ "$#" -ne 1 ]; then
+ echo "$CMDNAME: invalid option: $2" 1>&2
+ echo "Try '$CMDNAME --help' for more information." 1>&2
+ exit 1
+ fi
;;
esac
shift
***************
*** 118,124 ****
echo "$CMDNAME creates a PostgreSQL database."
echo
echo "Usage:"
! echo " $CMDNAME [options] dbname [description]"
echo
echo "Options:"
echo " -D, --location=PATH Alternative place to store the database"
--- 124,130 ----
echo "$CMDNAME creates a PostgreSQL database."
echo
echo "Usage:"
! echo " $CMDNAME [options] [dbname] [description]"
echo
echo "Options:"
echo " -D, --location=PATH Alternative place to store the database"
Index: src/bin/scripts/createlang.sh
===================================================================
RCS file: /cvsroot/pgsql/src/bin/scripts/createlang.sh,v
retrieving revision 1.32
diff -c -r1.32 createlang.sh
*** src/bin/scripts/createlang.sh 2002/01/03 05:30:04 1.32
--- src/bin/scripts/createlang.sh 2002/01/04 05:23:05
***************
*** 116,121 ****
--- 116,126 ----
fi
else dbname="$1"
fi
+ if [ "$#" -ne 1 ]; then
+ echo "$CMDNAME: invalid option: $2" 1>&2
+ echo "Try '$CMDNAME --help' for more information." 1>&2
+ exit 1
+ fi
;;
esac
shift
Index: src/bin/scripts/createuser
===================================================================
RCS file: /cvsroot/pgsql/src/bin/scripts/createuser,v
retrieving revision 1.22
diff -c -r1.22 createuser
*** src/bin/scripts/createuser 2001/09/30 22:17:51 1.22
--- src/bin/scripts/createuser 2002/01/04 05:23:05
***************
*** 123,128 ****
--- 123,133 ----
;;
*)
NewUser="$1"
+ if [ "$#" -ne 1 ]; then
+ echo "$CMDNAME: invalid option: $2" 1>&2
+ echo "Try '$CMDNAME --help' for more information." 1>&2
+ exit 1
+ fi
;;
esac
shift;
Index: src/bin/scripts/dropdb
===================================================================
RCS file: /cvsroot/pgsql/src/bin/scripts/dropdb,v
retrieving revision 1.13
diff -c -r1.13 dropdb
*** src/bin/scripts/dropdb 2001/09/30 22:17:51 1.13
--- src/bin/scripts/dropdb 2002/01/04 05:23:05
***************
*** 89,94 ****
--- 89,99 ----
;;
*)
dbname="$1"
+ if [ "$#" -ne 1 ]; then
+ echo "$CMDNAME: invalid option: $2" 1>&2
+ echo "Try '$CMDNAME --help' for more information." 1>&2
+ exit 1
+ fi
;;
esac
shift
Index: src/bin/scripts/droplang
===================================================================
RCS file: /cvsroot/pgsql/src/bin/scripts/droplang,v
retrieving revision 1.20
diff -c -r1.20 droplang
*** src/bin/scripts/droplang 2002/01/03 08:53:00 1.20
--- src/bin/scripts/droplang 2002/01/04 05:23:05
***************
*** 105,110 ****
--- 105,115 ----
fi
else dbname="$1"
fi
+ if [ "$#" -ne 1 ]; then
+ echo "$CMDNAME: invalid option: $2" 1>&2
+ echo "Try '$CMDNAME --help' for more information." 1>&2
+ exit 1
+ fi
;;
esac
shift
Index: src/bin/scripts/dropuser
===================================================================
RCS file: /cvsroot/pgsql/src/bin/scripts/dropuser,v
retrieving revision 1.14
diff -c -r1.14 dropuser
*** src/bin/scripts/dropuser 2001/09/30 22:17:51 1.14
--- src/bin/scripts/dropuser 2002/01/04 05:23:05
***************
*** 91,96 ****
--- 91,101 ----
;;
*)
DelUser="$1"
+ if [ "$#" -ne 1 ]; then
+ echo "$CMDNAME: invalid option: $2" 1>&2
+ echo "Try '$CMDNAME --help' for more information." 1>&2
+ exit 1
+ fi
;;
esac
shift;
Index: src/bin/scripts/vacuumdb
===================================================================
RCS file: /cvsroot/pgsql/src/bin/scripts/vacuumdb,v
retrieving revision 1.19
diff -c -r1.19 vacuumdb
*** src/bin/scripts/vacuumdb 2001/09/30 22:17:51 1.19
--- src/bin/scripts/vacuumdb 2002/01/04 05:23:05
***************
*** 112,117 ****
--- 112,122 ----
;;
*)
dbname="$1"
+ if [ "$#" -ne 1 ]; then
+ echo "$CMDNAME: invalid option: $2" 1>&2
+ echo "Try '$CMDNAME --help' for more information." 1>&2
+ exit 1
+ fi
;;
esac
shift
***************
*** 151,159 ****
dbname=`${PATHNAME}psql $PSQLOPT -q -t -A -d template1 -c 'SELECT datname FROM pg_database WHERE datallowconn'`
elif [ -z "$dbname" ]; then
! echo "$CMDNAME: missing required argument: database name" 1>&2
! echo "Try '$CMDNAME -?' for help." 1>&2
! exit 1
fi
for db in $dbname
--- 156,167 ----
dbname=`${PATHNAME}psql $PSQLOPT -q -t -A -d template1 -c 'SELECT datname FROM pg_database WHERE datallowconn'`
elif [ -z "$dbname" ]; then
! if [ "$PGUSER" ]; then
! dbname="$PGUSER"
! else
! dbname=`${PATHNAME}pg_id -u -n`
! fi
! [ "$?" -ne 0 ] && exit 1
fi
for db in $dbname
[2002-01-04 00:34] Bruce Momjian said: | Brent Verner wrote: | > [2002-01-03 23:33] Peter Eisentraut said: | > | > Brent Verner wrote: | > | > > [2002-01-03 14:19] Bruce Momjian said: | > | > > | | > | > > | Actually, we can just do: | > | > | > > +1 | > | > > | > | > > I can't see a reason to /not/ fix something this simple for the 7.2 | > | > > release. In general, I think it's best to fix things like this[1] | > | > > "on sight" as opposed to queueing them in TODO where they /might/ sit | > | > > untouched through another release cycle. | > | > > | > | > > [1] meaning problems that require little effort to fix, and whose | > | > > solutions are /very/ localized. | > | > | > | > OK, one more +1 and I will get to it. | > | | > | -4 | | OK, here's the patch. Seems createdb wasn't properly handling the db | comment (arg code now similar to createlang), createlang dbname being | optional wasn't documented in --help, and vacuumdb wasn't handlling an | optional dbname. I added the required checks so extra arguments report | a failure: | | $ dropdb lkjas asdfljk test | dropdb: invalid option: asdfljk | Try 'dropdb --help' for more information. This looks good to me. thanks. brent -- "Develop your talent, man, and leave the world something. Records are really gifts from people. To think that an artist would love you enough to share his music with anyone is a beautiful thing." -- Duane Allman
Peter is against this patch, and he has done lots of work on the scripts, so I will keep my patch for 7.3. --------------------------------------------------------------------------- Brent Verner wrote: > [2002-01-03 23:33] Peter Eisentraut said: > | > Brent Verner wrote: > | > > [2002-01-03 14:19] Bruce Momjian said: > | > > | > | > > | Actually, we can just do: > > | > > +1 > | > > > | > > I can't see a reason to /not/ fix something this simple for the 7.2 > | > > release. In general, I think it's best to fix things like this[1] > | > > "on sight" as opposed to queueing them in TODO where they /might/ sit > | > > untouched through another release cycle. > | > > > | > > [1] meaning problems that require little effort to fix, and whose > | > > solutions are /very/ localized. > | > > | > OK, one more +1 and I will get to it. > | > | -4 > > Hey, no ballot stuffing ;-) > > | 1: It's not a regression from 7.1. Anything else is too late. > > IMO, this is not a valid reason to /not/ fix _this problem_. Taken > to an extreme, this reasoning would allow any number of fatal bugs to > remain in 7.2 because they also existed in 7.1. Given the imminent > release, this position is valid even for a number of non-fatal bugs, > but not this one. > > | 2: The issue does not cause problems if you stick to the documented syntax > | and it > | does not cause hazard if you don't. > > First half correct. It is arguable whether or not the following > would be considered a hazard or not. Is unexpected behavior > hazardous? > > $ createdb whatiwanted whatigot > > | 3: The patch is wrong, because showing the usage screen in case of an error > | is > | inappropriate. > > Perhaps a more suitable message similar to the "invalid option" error > should be printed prior to exit, but this is not as important as > enforcing proper/documented use of the script(s). > > cheers. > brent > > -- > "Develop your talent, man, and leave the world something. Records are > really gifts from people. To think that an artist would love you enough > to share his music with anyone is a beautiful thing." -- Duane Allman > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026