Thread: Re: [COMMITTERS] pgsql: StrNCpy -> strlcpy (not complete)
Woh. Peter, you realize one of the reasons we use StrNCpy as a macro is for performance. I don't see strlcpy as a macro. Are you going to change all call locations to strlcpy? If so, have you measured the performance impact? --------------------------------------------------------------------------- Peter Eisentraut wrote: > Log Message: > ----------- > StrNCpy -> strlcpy (not complete) > > Modified Files: > -------------- > pgsql/src/backend/bootstrap: > bootstrap.c (r1.229 -> r1.230) > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/bootstrap/bootstrap.c.diff?r1=1.229&r2=1.230) > pgsql/src/backend/libpq: > crypt.c (r1.72 -> r1.73) > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/libpq/crypt.c.diff?r1=1.72&r2=1.73) > hba.c (r1.159 -> r1.160) > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/libpq/hba.c.diff?r1=1.159&r2=1.160) > ip.c (r1.39 -> r1.40) > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/libpq/ip.c.diff?r1=1.39&r2=1.40) > pgsql/src/backend/nodes: > print.c (r1.83 -> r1.84) > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/nodes/print.c.diff?r1=1.83&r2=1.84) > pgsql/src/backend/postmaster: > pgarch.c (r1.28 -> r1.29) > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/postmaster/pgarch.c.diff?r1=1.28&r2=1.29) > postmaster.c (r1.518 -> r1.519) > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/postmaster/postmaster.c.diff?r1=1.518&r2=1.519) > pgsql/src/backend/tcop: > postgres.c (r1.521 -> r1.522) > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/tcop/postgres.c.diff?r1=1.521&r2=1.522) > pgsql/src/backend/utils/misc: > guc-file.l (r1.46 -> r1.47) > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/misc/guc-file.l.diff?r1=1.46&r2=1.47) > pgsql/src/bin/initdb: > initdb.c (r1.131 -> r1.132) > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/bin/initdb/initdb.c.diff?r1=1.131&r2=1.132) > pgsql/src/bin/pg_ctl: > pg_ctl.c (r1.77 -> r1.78) > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/bin/pg_ctl/pg_ctl.c.diff?r1=1.77&r2=1.78) > pgsql/src/bin/pg_dump: > pg_dumpall.c (r1.89 -> r1.90) > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/bin/pg_dump/pg_dumpall.c.diff?r1=1.89&r2=1.90) > pgsql/src/bin/pg_resetxlog: > pg_resetxlog.c (r1.56 -> r1.57) > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/bin/pg_resetxlog/pg_resetxlog.c.diff?r1=1.56&r2=1.57) > pgsql/src/interfaces/libpq: > fe-auth.c (r1.122 -> r1.123) > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/interfaces/libpq/fe-auth.c.diff?r1=1.122&r2=1.123) > fe-connect.c (r1.342 -> r1.343) > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/interfaces/libpq/fe-connect.c.diff?r1=1.342&r2=1.343) > pgsql/src/timezone: > pgtz.c (r1.49 -> r1.50) > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/timezone/pgtz.c.diff?r1=1.49&r2=1.50) > > ---------------------------(end of broadcast)--------------------------- > TIP 6: explain analyze is your friend -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Sat, Feb 10, 2007 at 09:21:04PM -0500, Bruce Momjian wrote: > Woh. Peter, you realize one of the reasons we use StrNCpy as a macro is > for performance. I don't see strlcpy as a macro. Are you going to > change all call locations to strlcpy? If so, have you measured the > performance impact? I think we had this discussion already. strncpy() copies N bytes, whereas strlcpy() copies only as many bytes as necessary. For short strings with larger buffers, strlcpy() wins. It's understood that in many cases in PostgreSQL, the expectation is for short strings, and it is not required for the later bytes to be '\0'. I assume Peter is only changing the provably good uses? :-) Cheers, mark > Peter Eisentraut wrote: > > Log Message: > > ----------- > > StrNCpy -> strlcpy (not complete) > > > > Modified Files: > > -------------- > > pgsql/src/backend/bootstrap: > > bootstrap.c (r1.229 -> r1.230) > > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/bootstrap/bootstrap.c.diff?r1=1.229&r2=1.230) > > pgsql/src/backend/libpq: > > crypt.c (r1.72 -> r1.73) > > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/libpq/crypt.c.diff?r1=1.72&r2=1.73) > > hba.c (r1.159 -> r1.160) > > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/libpq/hba.c.diff?r1=1.159&r2=1.160) > > ip.c (r1.39 -> r1.40) > > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/libpq/ip.c.diff?r1=1.39&r2=1.40) > > pgsql/src/backend/nodes: > > print.c (r1.83 -> r1.84) > > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/nodes/print.c.diff?r1=1.83&r2=1.84) > > pgsql/src/backend/postmaster: > > pgarch.c (r1.28 -> r1.29) > > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/postmaster/pgarch.c.diff?r1=1.28&r2=1.29) > > postmaster.c (r1.518 -> r1.519) > > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/postmaster/postmaster.c.diff?r1=1.518&r2=1.519) > > pgsql/src/backend/tcop: > > postgres.c (r1.521 -> r1.522) > > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/tcop/postgres.c.diff?r1=1.521&r2=1.522) > > pgsql/src/backend/utils/misc: > > guc-file.l (r1.46 -> r1.47) > > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/misc/guc-file.l.diff?r1=1.46&r2=1.47) > > pgsql/src/bin/initdb: > > initdb.c (r1.131 -> r1.132) > > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/bin/initdb/initdb.c.diff?r1=1.131&r2=1.132) > > pgsql/src/bin/pg_ctl: > > pg_ctl.c (r1.77 -> r1.78) > > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/bin/pg_ctl/pg_ctl.c.diff?r1=1.77&r2=1.78) > > pgsql/src/bin/pg_dump: > > pg_dumpall.c (r1.89 -> r1.90) > > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/bin/pg_dump/pg_dumpall.c.diff?r1=1.89&r2=1.90) > > pgsql/src/bin/pg_resetxlog: > > pg_resetxlog.c (r1.56 -> r1.57) > > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/bin/pg_resetxlog/pg_resetxlog.c.diff?r1=1.56&r2=1.57) > > pgsql/src/interfaces/libpq: > > fe-auth.c (r1.122 -> r1.123) > > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/interfaces/libpq/fe-auth.c.diff?r1=1.122&r2=1.123) > > fe-connect.c (r1.342 -> r1.343) > > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/interfaces/libpq/fe-connect.c.diff?r1=1.342&r2=1.343) > > pgsql/src/timezone: > > pgtz.c (r1.49 -> r1.50) > > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/timezone/pgtz.c.diff?r1=1.49&r2=1.50) > > > > ---------------------------(end of broadcast)--------------------------- > > TIP 6: explain analyze is your friend > > -- > Bruce Momjian <bruce@momjian.us> http://momjian.us > EnterpriseDB http://www.enterprisedb.com > > + If your life is a hard drive, Christ can be your backup. + > > ---------------------------(end of broadcast)--------------------------- > TIP 3: Have you checked our extensive FAQ? > > http://www.postgresql.org/docs/faq -- mark@mielke.cc / markm@ncf.ca / markm@nortel.com __________________________ . . _ ._ . . .__ . . ._. .__ . . . .__ | Neighbourhood Coder |\/| |_| |_| |/ |_ |\/| | |_ | |/ |_ | | | | | | \ | \ |__ . | | .|. |__ |__ | \ |__ | Ottawa, Ontario, Canada One ring to rule them all, one ring to find them, one ring to bring them all and in the darkness bindthem... http://mark.mielke.cc/
Bruce Momjian <bruce@momjian.us> writes:
> Woh. Peter, you realize one of the reasons we use StrNCpy as a macro is
> for performance. I don't see strlcpy as a macro.
Huh? StrNCpy is a wrapper around strncpy(). Do you have reason to
think that strncpy() is especially tightly implemented on most
platforms?
regards, tom lane
mark@mark.mielke.cc wrote: > I think we had this discussion already. strncpy() copies N bytes, > whereas strlcpy() copies only as many bytes as necessary. For short > strings with larger buffers, strlcpy() wins. It's understood that > in many cases in PostgreSQL, the expectation is for short strings, > and it is not required for the later bytes to be '\0'. You may also speculate that strncpy() is more optimized in some C libraries than strlcpy(). However, the changed cases are all uninteresting in terms of performance or fall under the short strings in long buffers case. The remaining uses of StrNCpy() are either inner loops which need to be investigated, or it's not clear whether the zero-filling of strncpy() is required, or it's in a library were the libpgport linkages needs to be added. The main idea here is to get this programming style out because it's become clear that people are very confused about how to use some of the other functions correctly. -- Peter Eisentraut http://developer.postgresql.org/~petere/
Peter Eisentraut wrote: > mark@mark.mielke.cc wrote: > > I think we had this discussion already. strncpy() copies N bytes, > > whereas strlcpy() copies only as many bytes as necessary. For short > > strings with larger buffers, strlcpy() wins. It's understood that > > in many cases in PostgreSQL, the expectation is for short strings, > > and it is not required for the later bytes to be '\0'. > > You may also speculate that strncpy() is more optimized in some C > libraries than strlcpy(). However, the changed cases are all > uninteresting in terms of performance or fall under the short strings > in long buffers case. > > The remaining uses of StrNCpy() are either inner loops which need to be > investigated, or it's not clear whether the zero-filling of strncpy() > is required, or it's in a library were the libpgport linkages needs to > be added. > > The main idea here is to get this programming style out because it's > become clear that people are very confused about how to use some of the > other functions correctly. Sorry, I was confusing this with MemSet. Thanks for the clarification. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +