Re: [HACKERS] Initial refactoring of plperl.c - updated
On Sun, Jan 10, 2010 at 01:16:13AM -0500, Tom Lane wrote: Tim Bunce tim.bu...@pobox.com writes: On Sat, Jan 09, 2010 at 11:16:18PM +0200, Peter Eisentraut wrote: What's the reason for the temp file here? Defensive. If the text2macro.pl program fails/dies then you'd be left with a broken output file with a newer timestamp, so the next make wouldn't rerun text2macro.pl. Doesn't make forcibly remove the target file if the command fails? [ rummages in manual... ] It does if you specify .DELETE_ON_ERROR, which we do (in Makefile.global); so this bit of complication is a waste. Okay. Andrew, perhaps you could apply the attached to fix that. (Or I could bundle it into one of the split out plperl feature patches.) Tim. diff --git a/src/pl/plperl/GNUmakefile b/src/pl/plperl/GNUmakefile index 43b0fd0..3733da7 100644 *** a/src/pl/plperl/GNUmakefile --- b/src/pl/plperl/GNUmakefile *** include $(top_srcdir)/src/Makefile.shlib *** 57,64 plperl.o: perlchunks.h perlchunks.h: $(PERLCHUNKS) ! $(PERL) $(srcdir)/text2macro.pl --strip='^(\#.*|\s*)$$' $^ perlchunks.htmp ! mv perlchunks.htmp perlchunks.h all: all-lib --- 57,63 plperl.o: perlchunks.h perlchunks.h: $(PERLCHUNKS) ! $(PERL) $(srcdir)/text2macro.pl --strip='^(\#.*|\s*)$$' $^ perlchunks.h all: all-lib *** submake: *** 79,85 $(MAKE) -C $(top_builddir)/src/test/regress pg_regress$(X) clean distclean maintainer-clean: clean-lib ! rm -f SPI.c $(OBJS) perlchunks.htmp perlchunks.h rm -rf results rm -f regression.diffs regression.out --- 78,84 $(MAKE) -C $(top_builddir)/src/test/regress pg_regress$(X) clean distclean maintainer-clean: clean-lib ! rm -f SPI.c $(OBJS) perlchunks.h rm -rf results rm -f regression.diffs regression.out -- 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] Initial refactoring of plperl.c - updated
Tim Bunce tim.bu...@pobox.com writes: On Sun, Jan 10, 2010 at 01:16:13AM -0500, Tom Lane wrote: Doesn't make forcibly remove the target file if the command fails? Andrew, perhaps you could apply the attached to fix that. (Or I could bundle it into one of the split out plperl feature patches.) Applied. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Initial refactoring of plperl.c - updated
On fre, 2010-01-08 at 12:46 +, Tim Bunce wrote: *** 45,50 --- 45,55 include $(top_srcdir)/src/Makefile.shlib + plperl.o: perlchunks.h + + perlchunks.h: plc_*.pl + $(PERL) text2macro.pl --strip='^(\#.*|\s*)$$' plc_*.pl perlchunks.htmp + mv perlchunks.htmp perlchunks.h all: all-lib What's the reason for the temp file here? -- 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] Initial refactoring of plperl.c - updated
On Fri, Jan 08, 2010 at 09:47:01PM -0500, Andrew Dunstan wrote: Tim Bunce wrote: I see you've not commited it yet, so to help out I've attached a new diff, over the current CVS head, with two minor changes: - Removed the test, as noted above. - Optimized pg_verifymbstr calls to avoid unneeded strlen()s. I have committed this with minor edits. That should give us a clean base for the features patch(es). Wonderful. Many thanks Andrew. Tim. -- 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] Initial refactoring of plperl.c - updated
On Sat, Jan 09, 2010 at 11:16:18PM +0200, Peter Eisentraut wrote: On fre, 2010-01-08 at 12:46 +, Tim Bunce wrote: *** 45,50 --- 45,55 include $(top_srcdir)/src/Makefile.shlib + plperl.o: perlchunks.h + + perlchunks.h: plc_*.pl + $(PERL) text2macro.pl --strip='^(\#.*|\s*)$$' plc_*.pl perlchunks.htmp + mv perlchunks.htmp perlchunks.h all: all-lib What's the reason for the temp file here? Defensive. If the text2macro.pl program fails/dies then you'd be left with a broken output file with a newer timestamp, so the next make wouldn't rerun text2macro.pl. Tim. p.s. In the makefile for perl we use a little utility called mv_if_diff instead of a plain mv so that any downstream dependencies only get rebuilt if the contents have changed. -- 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] Initial refactoring of plperl.c - updated
On Sat, Jan 09, 2010 at 11:49:22PM +, Tim Bunce wrote: On Sat, Jan 09, 2010 at 11:16:18PM +0200, Peter Eisentraut wrote: On fre, 2010-01-08 at 12:46 +, Tim Bunce wrote: *** 45,50 --- 45,55 include $(top_srcdir)/src/Makefile.shlib + plperl.o: perlchunks.h + + perlchunks.h: plc_*.pl + $(PERL) text2macro.pl --strip='^(\#.*|\s*)$$' plc_*.pl perlchunks.htmp + mv perlchunks.htmp perlchunks.h all: all-lib What's the reason for the temp file here? Defensive. If the text2macro.pl program fails/dies then you'd be left with a broken output file with a newer timestamp, so the next make wouldn't rerun text2macro.pl. An alternative would be for text2macro.pl to write to a temp file and rename at the end. Tim. -- 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] Initial refactoring of plperl.c - updated
On sön, 2010-01-10 at 00:03 +, Tim Bunce wrote: On Sat, Jan 09, 2010 at 11:49:22PM +, Tim Bunce wrote: On Sat, Jan 09, 2010 at 11:16:18PM +0200, Peter Eisentraut wrote: On fre, 2010-01-08 at 12:46 +, Tim Bunce wrote: *** 45,50 --- 45,55 include $(top_srcdir)/src/Makefile.shlib + plperl.o: perlchunks.h + + perlchunks.h: plc_*.pl + $(PERL) text2macro.pl --strip='^(\#.*|\s*)$$' plc_*.pl perlchunks.htmp + mv perlchunks.htmp perlchunks.h all: all-lib What's the reason for the temp file here? Defensive. If the text2macro.pl program fails/dies then you'd be left with a broken output file with a newer timestamp, so the next make wouldn't rerun text2macro.pl. An alternative would be for text2macro.pl to write to a temp file and rename at the end. Sounds better. I think any program should be written such that it doesn't produce an output file at all if it cannot produce a correct output file. So use a temp file or a trap or something like that. The makefile should not have to clean up after everyone. -- 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] Initial refactoring of plperl.c - updated
Tim Bunce tim.bu...@pobox.com writes: On Sat, Jan 09, 2010 at 11:16:18PM +0200, Peter Eisentraut wrote: What's the reason for the temp file here? Defensive. If the text2macro.pl program fails/dies then you'd be left with a broken output file with a newer timestamp, so the next make wouldn't rerun text2macro.pl. Doesn't make forcibly remove the target file if the command fails? [ rummages in manual... ] It does if you specify .DELETE_ON_ERROR, which we do (in Makefile.global); so this bit of complication is a waste. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Initial refactoring of plperl.c - updated
Tim Bunce wrote: I see you've not commited it yet, so to help out I've attached a new diff, over the current CVS head, with two minor changes: - Removed the test, as noted above. - Optimized pg_verifymbstr calls to avoid unneeded strlen()s. I have committed this with minor edits. That should give us a clean base for the features patch(es). cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
