More problem with scripts

Started by Bruce Momjianabout 24 years ago13 messages

pgman@candle.pha.pa.us

about 24 years ago

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:

* 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, Pennsylvania 19026

Brent Verner

brent@rcfile.org

about 24 years ago

In reply to: Bruce Momjian (#1)

Re: More problem with scripts

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

Brent Verner

brent@rcfile.org

about 24 years ago

In reply to: Brent Verner (#2)

Re: More problem with scripts

er, minus this debugging cruft :-)

sorry,
b

Bruce Momjian

pgman@candle.pha.pa.us

about 24 years ago

In reply to: Brent Verner (#2)

Re: More problem with scripts

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, Pennsylvania 19026

Brent Verner

brent@rcfile.org

about 24 years ago

In reply to: Bruce Momjian (#4)

Re: More problem with scripts

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]meaning problems that require little effort to fix, and whose solutions are /very/ localized.
"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.
solutions are /very/ localized.

cheers.
brent

Bruce Momjian

pgman@candle.pha.pa.us

about 24 years ago

In reply to: Brent Verner (#5)

Re: More problem with scripts

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, Pennsylvania 19026

Tom Lane

tgl@sss.pgh.pa.us

about 24 years ago

In reply to: Bruce Momjian (#6)

Re: More problem with scripts

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

Peter Eisentraut

peter_e@gmx.net

about 24 years ago

In reply to: Bruce Momjian (#6)

1 attachment(s)

Re: More problem with scripts

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

Thomas Lockhart

lockhart@fourpalms.org

about 24 years ago

In reply to: Bruce Momjian (#6)

Re: More problem with scripts

...

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

#10

Brent Verner

brent@rcfile.org

about 24 years ago

In reply to: Peter Eisentraut (#8)

Re: More problem with scripts

| > > +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

#11

Bruce Momjian

pgman@candle.pha.pa.us

about 24 years ago

In reply to: Brent Verner (#10)

1 attachment(s)

Re: More problem with scripts

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

Attachments:

/bjm/difftext/plainDownload

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

#12

Brent Verner

brent@rcfile.org

about 24 years ago

In reply to: Bruce Momjian (#11)

Re: More problem with scripts

[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

#13

Bruce Momjian

pgman@candle.pha.pa.us

almost 24 years ago

In reply to: Brent Verner (#10)

Re: More problem with scripts

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, Pennsylvania 19026