Re: [HACKERS] Review: create extension default_full_version
Now it works in most of the cases, here is one more point about the patch. * In case we have hstore--1.3.sql file and want to install that file, but failed because of default_full_version. No default_full_version specified --- postgres=# CREATE EXTENSION hstore version '1.3'; CREATE EXTENSION default_full_version = 1.2 postgres=# CREATE EXTENSION hstore version '1.3'; ERROR: could not stat file /usr/local/pgsql/share/extension/hstore--1.2.sql: No such file or directory If we don't want to address this issue at least we should give some meaningful error message. On Tue, Dec 4, 2012 at 4:46 PM, Ibrar Ahmed ibrar.ah...@gmail.com wrote: On Tue, Dec 4, 2012 at 7:54 PM, Dimitri Fontaine dimi...@2ndquadrant.frwrote: Ibrar Ahmed ibrar.ah...@gmail.com writes: I am still getting the same error message. With the attached patch (v2), it works well: create extension hstore version '1.2' from 'unpackaged'; DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--unpackaged--1.0.sql' DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.0--1.1.sql' DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.1--1.2.sql' CREATE EXTENSION You have to remember that the spelling FROM 'unpackaged version' really means that we have previously installed a loose version of the extension (just \i hstore.sql) and want to apply the upgrade path from there. We can't have FROM meaning the same thing as default_full_version. I know. With default_full_version = '1.1' postgres=# CREATE EXTENSION hstore version '1.3' from '1.1'; DEBUG: execute_extension_script: '/usr/local/pgsql/share/extension/hstore--1.1.sql' DEBUG: execute_extension_script: '/usr/local/pgsql/share/extension/hstore--1.1--1.3.sql' *ERROR: could not stat file /usr/local/pgsql/share/extension/hstore--1.1--1.3.sql: No such file or directory* That's nonetheless a bug and is fixed now: - if (pcontrol-default_full_version) + if (pcontrol-default_full_version !unpackaged) Thanks, I will look at this again in detail. See attached. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Ibrar Ahmed EnterpriseDB http://www.enterprisedb.com -- Ibrar Ahmed EnterpriseDB http://www.enterprisedb.com
Re: [HACKERS] Review: create extension default_full_version
On Mon, Dec 3, 2012 at 11:05 PM, Dimitri Fontaine dimi...@2ndquadrant.frwrote:
Hi,
Thanks for your very good review!
Ibrar Ahmed ibrar.ah...@gmail.com writes:
I looked at the discussion for this patch and the patch itself. Here
are my comments and observations about the patch.
What I got from the discussion is that the patch tries to implement a
mechanism to install extension from series of SQL scripts from
base/full version e.g. if a user wants to create an extension 1.1,
system should run v1.0 script followed by 1.0--1.1 script. In that
case we need to know about the base or full version which in the above
case is v1.0. So the patch added a defualt_full_version option in
extension control file.
Exactly, that was an idea from Robert and I implemented it quite
quickly. Too quickly as we can see from your testing report.
Here are my comments about the patch
* Note: Patch does not apply cleanly on latest code base. You probably
need to re-base the code
Done. The thing is that meanwhile another solution to the main problem
has been found: drop support for installing hstore 1.0. Attached patch
fixes the problem by reinstalling hstore--1.0.sql and re-enabling this
version, and removing the hstore--1.1.sql file now that it's enough to
just have hstore--1.0--1.1.sql to install directly (and by default) the
newer version.
I think we will have to decide about taking only the mechanism or both
the mechanism and the actual change for the hstore contrib.
* This is a user visible change so documentation change is required here.
Added coverage of the new parameter.
* Also, You need to update the comment, because this code is now
handling default_full_version as well.
/*
* Determine the (unpackaged) version to update from, if any, and
then
* figure out what sequence of update scripts we need to apply.
*/
if ((d_old_version d_old_version-arg) ||
pcontrol-default_full_version)
Done. I also fixed the bugs you reported here. Here's an edited version
of the new (fixed) output:
dim=# set client_min_messages to debug1;
dim=# create extension hstore version '1.0';
DEBUG: execute_extension_script:
'/Users/dim/pgsql/ddl/share/extension/hstore--1.0.sql'
WARNING: = is deprecated as an operator name
CREATE EXTENSION
dim=# create extension hstore version '1.1';
DEBUG: execute_extension_script:
'/Users/dim/pgsql/ddl/share/extension/hstore--1.0.sql'
WARNING: = is deprecated as an operator name
DEBUG: execute_extension_script:
'/Users/dim/pgsql/ddl/share/extension/hstore--1.0--1.1.sql'
CREATE EXTENSION
dim=# create extension hstore;
DEBUG: execute_extension_script:
'/Users/dim/pgsql/ddl/share/extension/hstore--1.0.sql'
WARNING: = is deprecated as an operator name
DETAIL: This name may be disallowed altogether in future versions of
PostgreSQL.
DEBUG: execute_extension_script:
'/Users/dim/pgsql/ddl/share/extension/hstore--1.0--1.1.sql'
CREATE EXTENSION
postgres=# CREATE EXTENSION hstore version '1.3' from '1.0';
WARNING: /usr/local/pgsql/share/extension/hstore--1.0--1.1.sql
WARNING: /usr/local/pgsql/share/extension/hstore--1.1--1.2.sql
WARNING: /usr/local/pgsql/share/extension/hstore--1.2--1.3.sql
CREATE EXTENSION
I liked your idea of extending the reporting about what files are used,
but of course we can't keep that at the WARNING level, so I made that
logging DEBUG1 in the attached patch.
postgres=# CREATE EXTENSION hstore version '1.3' from '1.0';
Please try that case again, I believe it's fixed in the attached.
I am still getting the same error message.
Without default_full_version
postgres=# CREATE EXTENSION hstore version '1.3' from '1.1';
DEBUG: execute_extension_script:
'/usr/local/pgsql/share/extension/hstore--1.1--1.2.sql'
DEBUG: execute_extension_script:
'/usr/local/pgsql/share/extension/hstore--1.2--1.3.sql'
CREATE EXTENSION
With default_full_version = '1.1'
postgres=# CREATE EXTENSION hstore version '1.3' from '1.1';
DEBUG: execute_extension_script:
'/usr/local/pgsql/share/extension/hstore--1.1.sql'
DEBUG: execute_extension_script:
'/usr/local/pgsql/share/extension/hstore--1.1--1.3.sql'
*ERROR: could not stat file
/usr/local/pgsql/share/extension/hstore--1.1--1.3.sql: No such file or
directory*
I think there is an issue in this area of the code
if (pcontrol-default_full_version)
{
execute_extension_script(extensionOid, control,
-- 1.1.sql
NULL, oldVersionName,
requiredSchemas,
schemaName, schemaOid);
ApplyExtensionUpdates(extensionOid, pcontrol,
-- 1.1--1.3.sql (wrong)
oldVersionName, updateVersions);
The first statement is executing 1.1.sql because oldVersionName = 1.1.
Keep in mind that versionName = 1.2 and updateVersions list contain only
version name 1.3. So in case of default_full_version you are ignoring *
versionName* which is *1.2
Re: [HACKERS] Review: create extension default_full_version
On Tue, Dec 4, 2012 at 7:54 PM, Dimitri Fontaine dimi...@2ndquadrant.frwrote: Ibrar Ahmed ibrar.ah...@gmail.com writes: I am still getting the same error message. With the attached patch (v2), it works well: create extension hstore version '1.2' from 'unpackaged'; DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--unpackaged--1.0.sql' DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.0--1.1.sql' DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.1--1.2.sql' CREATE EXTENSION You have to remember that the spelling FROM 'unpackaged version' really means that we have previously installed a loose version of the extension (just \i hstore.sql) and want to apply the upgrade path from there. We can't have FROM meaning the same thing as default_full_version. I know. With default_full_version = '1.1' postgres=# CREATE EXTENSION hstore version '1.3' from '1.1'; DEBUG: execute_extension_script: '/usr/local/pgsql/share/extension/hstore--1.1.sql' DEBUG: execute_extension_script: '/usr/local/pgsql/share/extension/hstore--1.1--1.3.sql' *ERROR: could not stat file /usr/local/pgsql/share/extension/hstore--1.1--1.3.sql: No such file or directory* That's nonetheless a bug and is fixed now: - if (pcontrol-default_full_version) + if (pcontrol-default_full_version !unpackaged) Thanks, I will look at this again in detail. See attached. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Ibrar Ahmed EnterpriseDB http://www.enterprisedb.com
[HACKERS] Review: create extension default_full_version
Hi, I looked at the discussion for this patch and the patch itself. Here are my comments and observations about the patch. What I got from the discussion is that the patch tries to implement a mechanism to install extension from series of SQL scripts from base/full version e.g. if a user wants to create an extension 1.1, system should run v1.0 script followed by 1.0--1.1 script. In that case we need to know about the base or full version which in the above case is v1.0. So the patch added a defualt_full_version option in extension control file. Here are my comments about the patch * Note: Patch does not apply cleanly on latest code base. You probably need to re-base the code ibrar@ibrar-laptop:~/work/postgresql$ patch -p1 extension-default-full-version.v0.patch patching file contrib/hstore/Makefile Hunk #1 FAILED at 5. 1 out of 1 hunk FAILED -- saving rejects to file contrib/hstore/Makefile.rej patching file contrib/hstore/hstore--1.1.sql patching file contrib/hstore/hstore.control patching file src/backend/commands/extension.c Hunk #1 succeeded at 68 (offset 2 lines). Hunk #2 succeeded at 508 (offset 2 lines). Hunk #3 succeeded at 1295 (offset 2 lines). Hunk #4 succeeded at 1316 (offset 2 lines). Hunk #5 succeeded at 1473 (offset 3 lines). * This is a user visible change so documentation change is required here. * Also, You need to update the comment, because this code is now handling default_full_version as well. /* * Determine the (unpackaged) version to update from, if any, and then * figure out what sequence of update scripts we need to apply. */ if ((d_old_version d_old_version-arg) || pcontrol-default_full_version) * In case the default_full_version and VERSION in SQL are same then we are getting a misleading error message i.e. comment = 'data type for storing sets of (key, value) pairs' default_version = '1.1' default_full_version = '1.0' module_pathname = '$libdir/hstore' relocatable = true postgres=# create EXTENSION hstore version '1.0'; ERROR: FROM version must be different from installation target version 1.0 Error message is complaining about FROM clause which is not used in the query. I think EXTENSION creation should not be blocked in case default_full_version and VERSION are same. But if we want to block that; error message should be meaningful. * I noticed another issue with the patch. In case we do not specify the default_full_version in control file then this is the sequence of sql scripts. postgres=# CREATE EXTENSION hstore version '1.3' from '1.0'; WARNING: /usr/local/pgsql/share/extension/hstore--1.0--1.1.sql WARNING: /usr/local/pgsql/share/extension/hstore--1.1--1.2.sql WARNING: /usr/local/pgsql/share/extension/hstore--1.2--1.3.sql CREATE EXTENSION But in case default_full_version = 1.0, then we are getting ERROR: could not stat file... error message. postgres=# CREATE EXTENSION hstore version '1.3' from '1.0'; WARNING: SCRIPT = /usr/local/pgsql/share/extension/hstore--1.0.sql WARNING: SCRIPT = /usr/local/pgsql/share/extension/hstore--1.0--1.2.sql --- Why not 1.0--1.1 ERROR: could not stat file /usr/local/pgsql/share/extension/hstore--1.0--1.2.sql: No such file or directory This is because of missing version number i.e. first we're executing 1.0 followed by 1.0--1.2 not 1.0--1.1 but I think following should be the right sequence postgres=# CREATE EXTENSION hstore version '1.3' from '1.0'; WARNING: /usr/local/pgsql/share/extension/hstore--1.0.sql WARNING: /usr/local/pgsql/share/extension/hstore--1.0--1.1.sql WARNING: /usr/local/pgsql/share/extension/hstore--1.1--1.2.sql WARNING: /usr/local/pgsql/share/extension/hstore--1.2--1.3.sql CREATE EXTENSION PS: I modified the code to get this result. - IMHO there should be an SQL option along with the default_full_version; like. postgres=# create EXTENSION hstore VERSION '1.1' FULL_VERSION '1.0'; - hstore regression is also failing. --- Ibrar Ahmed
Re: [HACKERS] errno not set in case of libm functions (HPUX)
On Fri, May 27, 2011 at 2:31 AM, Tom Lane t...@sss.pgh.pa.us wrote: Peter Eisentraut pete...@gmx.net writes: On tor, 2011-05-26 at 12:14 -0400, Tom Lane wrote: I tried this on my HP-UX 10.20 box, and it didn't work very nicely: configure decided that the compiler accepted +Olibmerrno, so I got a compile full of cc: warning 450: Unrecognized option +Olibmerrno. warnings. The reason is that PGAC_PROG_CC_CFLAGS_OPT does not pay any attention to whether the proposed flag generates a warning. That seems like a bug --- is there any situation where we'd want to accept a flag that does generate a warning? I'm thinking that macro should set ac_c_werror_flag=yes, the same way PGAC_C_INLINE does. I think so. OK, committed with that addition. Thanks, Is it worth to backport this? We could also do that globally, but that would probably be something for the next release. Hmm. I'm a bit scared of how much might break. I don't think the autoconf tests are generally designed to guarantee no warnings. regards, tom lane -- Ibrar Ahmed
Re: [HACKERS] errno not set in case of libm functions (HPUX)
Please find the updated patch. I have added this +Olibmerrno compile flag
check in configure/configure.in file.
OS
HP-UX B.11.31 U ia64
without patch
---
postgres=# select acos(2);
acos
--
NaN
(1 row)
with patch
---
postgres=# select acos(2);
ERROR: input is out of range
On Tue, May 24, 2011 at 11:13 PM, Tom Lane t...@sss.pgh.pa.us wrote:
Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes:
So the default is indeed non-standard. But I wonder if we should use -Aa
instead?
Probably not; at least on older HPUX versions, -Aa turns off access to
assorted stuff that we do want, eg long long. man cc on my box
saith
-Amode Specify the compilation standard to be used by the
compiler. mode can be one of the following letters:
c(Default) Compile in a mode compatible with
HP-UX releases prior to 7.0. (See The C
Programming Language, First Edition by
Kernighan and Ritchie). This option also
defines the symbol _HPUX_SOURCE and allows the
user to access macros and typedefs provided by
the HPUX Operating System. The default
compilation mode may change in future releases.
aCompile under ANSI mode (ANSI programming
language C standard ISO 9899:1990). When
compiling under ANSI mode, the header files
would define only those names (macros and
typedefs) specified by the Standard. To access
macros and typedefs that are not defined by the
ANSI Standard but are provided by the HPUX
Operating System, define the symbol
_HPUX_SOURCE; or use the extension option
described below.
eExtended ANSI mode. Same as -Aa -D_HPUX_SOURCE
+e. This would define the names (macros and
typedefs) provided by the HPUX Operating System
and, in addition, allow the following
extensions: $ characters in identifier names,
sized enums, sized bit-fields, and 64-bit
integral type long long. Additional extensions
may be added to this option in the future.
The +e option is elsewhere stated to mean
+eEnables HP value-added features while compiling
in ANSI C mode, -Aa. This option is ignored
with -Ac because these features are already
provided. Features enabled:
o Long pointers
o Integral type specifiers can appear in
enum declarations.
o The $ character can appear in
identifier names.
o Missing parameters on intrinsic calls
which isn't 100% consistent with what it says under -Ae, so maybe some
additional experimentation is called for. But anyway, autoconf appears
to think that -Ae is preferable to the combination -Aa -D_HPUX_SOURCE
(that choice is coming from autoconf not our own code); so I'm not
optimistic that we can get more-standard behavior by overriding that.
regards, tom lane
--
Ibrar Ahmed
diff --git a/configure b/configure
index 3b23c46..d448534 100755
--- a/configure
+++ b/configure
@@ -4468,6 +4468,66 @@ if test x$pgac_cv_prog_cc_cflags__qnoansialias = xyes; then
CFLAGS=$CFLAGS -qnoansialias
fi
+elif test $PORTNAME = hpux; then
+ # HP-UX libm functions on 'Integrity server' do not set errno by default,
+ # for errno setting, compile with the +Olibmerrno option.
+ { $as_echo $as_me:$LINENO: checking whether $CC supports +Olibmerrno 5
+$as_echo_n checking whether $CC supports +Olibmerrno... 6; }
+if test ${pgac_cv_prog_cc_cflags_pOlibmerrno+set} = set; then
+ $as_echo_n (cached) 6
+else
+ pgac_save_CFLAGS=$CFLAGS
+CFLAGS=$pgac_save_CFLAGS +Olibmerrno
+cat conftest.$ac_ext _ACEOF
+/* confdefs.h. */
+_ACEOF
+cat confdefs.h conftest.$ac_ext
+cat conftest.$ac_ext _ACEOF
+/* end confdefs.h. */
+
+int
+main ()
+{
+
+ ;
+ return 0;
+}
+_ACEOF
+rm -f conftest.$ac_objext
+if { (ac_try=$ac_compile
+case (($ac_try in
+ *\* | *\`* | *\\*) ac_try_echo=\$ac_try;;
+ *) ac_try_echo=$ac_try;;
+esac
+eval ac_try_echo=\\$as_me:$LINENO: $ac_try_echo\
+$as_echo $ac_try_echo) 5
+ (eval $ac_compile) 2conftest.er1
+ ac_status=$?
+ grep -v '^ *+' conftest.er1 conftest.err
+ rm -f
[HACKERS] errno not set in case of libm functions (HPUX)
I have found a problem which is specifically related to HP-UX compiler.
All 'libm' functions on HP-UX Integrity server do not set errno by
default. For 'errno' setting we should compile the code using +Olibmerrno
option. So we should add this option in /src/makefiles/Makefile.hpux.
Otherwise we cannot expect this code to work properly
[float.c]
Datum
dacos(PG_FUNCTION_ARGS)
{
...
errno = 0;
result = acos(arg1);
if (errno != 0)
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg(input is out of range)));
...
}
Because acos function will not set the errono in case of invalid input, so
check will not trigger the error message. I have attached a patch to add
this option in HPUX makefile.
BTW I have found same kind of discussion without any conclusion here
http://archives.postgresql.org/pgsql-hackers/2011-05/msg00046.php
--
Ibrar Ahmed
diff --git a/src/makefiles/Makefile.hpux b/src/makefiles/Makefile.hpux
index 1917d61..f2a8f19 100644
--- a/src/makefiles/Makefile.hpux
+++ b/src/makefiles/Makefile.hpux
@@ -43,6 +43,12 @@ else
CFLAGS_SL = +Z
endif
+
+# HP-UX libm functions on 'Integrity server' do not set errno by default,
+# for errno setting, compile with the +Olibmerrno option.
+
+CFLAGS := +Olibmerrno $(CFLAGS)
+
# Rule for building a shared library from a single .o file
%$(DLSUFFIX): %.o
ifeq ($(GCC), yes)
--
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] Fwd: psql include file using relative path
Gurjeet! What about tab completion, like in \i command? On Fri, Feb 25, 2011 at 5:07 AM, Robert Haas robertmh...@gmail.com wrote: On Thu, Feb 24, 2011 at 6:21 PM, Gurjeet Singh singh.gurj...@gmail.com wrote: psql has the ability to execute commands from a file, but if one wishes to develop and provide a modularized set of sql files, then psql is not very helpful because the \i command can open file paths either if they are absolute paths or if they are palced correctly relative to psql's current working directory. Attached patch adds a new meta-command to psql, '\ir' that allows the user to process files relative to currently processing file. Please add this patch to the currently open CommitFest at: https://commitfest.postgresql.org/action/commitfest_view/open -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Ibrar Ahmed -- 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] error order when debug postgresql step by step?
- export CFLAGS='-O0' may work for you. On Tue, Mar 1, 2011 at 8:21 PM, Kevin Grittner kevin.gritt...@wicourts.gov wrote: hom obsidian...@gmail.com wrote: How can I do to make sure the right excute order when I debug step by step ? You may need to reduce the optimization level of your compile. -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Ibrar Ahmed -- 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] REVIEW: Determining client_encoding from client locale
Stephen Frost!
I have modified the code to use ADD_STARTUP_OPTION instead of writing code
again.
And tried the patch on Windows and Linux and it works for me.
On Sun, Feb 6, 2011 at 10:19 AM, Stephen Frost sfr...@snowman.net wrote:
Ibrar,
* Ibrar Ahmed (ibrar.ah...@gmail.com) wrote:
I have reviewed/tested this patch.
Great, thanks for that!
In my point code should be like this
*if (conn-client_encoding_initial
conn-client_encoding_initial[0])
{
if (packet)
{
strcpy(packet + packet_len, client_encoding);
packet_len += strlen(client_encoding) + 1;
strcpy(packet + packet_len,
conn-client_encoding_initial);
packet_len +=
strlen(conn-client_encoding_initial) +
1;
}
}*
Makes sense to me, just reading through this email. Have you tested
this change..? Could you provide it as an additional patch or a new
patch including the change against head, assuming it still works well in
your testing?
I will test this patch on Windows and will send results.
That would be great, it's not easy for me to test under Windows.
Thanks!
Stephen
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.10 (GNU/Linux)
iEYEARECAAYFAk1O5joACgkQrzgMPqB3kijODgCeN1/PVKf/qzeuWOz82FwpR/B0
2rMAnR+4tCxNp9eZn7qIOTXqCv70H2oC
=vYXv
-END PGP SIGNATURE-
--
Ibrar Ahmed
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 7131fb4..9223b79 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -259,6 +259,21 @@ PGconn *PQconnectdbParams(const char **keywords, const char **values, int expand
/listitem
/varlistentry
+varlistentry id=libpq-connect-client-encoding xreflabel=client_encoding
+ termliteralclient_encoding/literal/term
+ listitem
+ para
+ This sets the varnameclient_encoding/varname
+ configuration parameter for this connection. In addition to
+ the values accepted by the corresponding server option, you
+ can use literalauto/literal to determine the right
+ encoding from the current locale in the client
+ (envarLC_CTYPE/envar environment variable on Unix
+ systems).
+ /para
+ /listitem
+/varlistentry
+
varlistentry id=libpq-connect-options xreflabel=options
termliteraloptions/literal/term
listitem
@@ -6355,6 +6370,16 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough)
linkend=libpq-connect-connect-timeout connection parameter.
/para
/listitem
+
+listitem
+ para
+ indexterm
+ primaryenvarPGCLIENTENCODING/envar/primary
+ /indexterm
+ envarPGCLIENTENCODING/envar behaves the same as xref
+ linkend=libpq-connect-client-encoding connection parameter.
+ /para
+/listitem
/itemizedlist
/para
@@ -6391,17 +6416,6 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough)
listitem
para
indexterm
- primaryenvarPGCLIENTENCODING/envar/primary
- /indexterm
- envarPGCLIENTENCODING/envar sets the default client character
- set encoding. (Equivalent to literalSET client_encoding TO
- .../literal.)
- /para
-/listitem
-
-listitem
- para
- indexterm
primaryenvarPGGEQO/envar/primary
/indexterm
envarPGGEQO/envar sets the default mode for the genetic query
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index eacae71..0bf05de 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -593,6 +593,17 @@ $ userinputpsql service=myservice sslmode=require/userinput
privileges, server is not running on the targeted host, etc.),
applicationpsql/application will return an error and terminate.
/para
+
+para
+ If at least one of standard input or standard output are a
+ terminal, then applicationpsql/application sets the client
+ encoding to quoteauto/quote, which will detect the
+ appropriate client encoding from the locale settings
+ (envarLC_CTYPE/envar environment variable on Unix systems).
+ If this doesn't work out as expected, the client encoding can be
+ overridden using the environment
+ variable envarPGCLIENTENCODING/envar.
+/para
/refsect2
refsect2 id=R2-APP-PSQL-4
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 301dc11..08c979a 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -1478,7 +1478,7 @@ do_connect(char *dbname, char *user, char *host, char *port)
while (true)
{
-#define PARAMS_ARRAY_SIZE 7
+#define PARAMS_ARRAY_SIZE 8
const char **keywords = pg_malloc(PARAMS_ARRAY_SIZE * sizeof(*keywords));
const char **values = pg_malloc(PARAMS_ARRAY_SIZE * sizeof(*values
Re: [HACKERS] REVIEW: Determining client_encoding from client locale
Hi!,
I have reviewed/tested this patch.
OS = Linux ubuntu 2.6.35-22-generic #33-Ubuntu SMP Sun Sep 19 20:34:50 UTC
2010 i686 GNU/Linux
PostgreSQL Version = Head (9.1devel)
Patch gives the desired results(still testing), but couple of questions with
this portion of the code.
*
if (conn-client_encoding_initial
conn-client_encoding_initial[0])
{
if (packet)
strcpy(packet + packet_len, client_encoding);
packet_len += strlen(client_encoding) + 1;
if (packet)
strcpy(packet + packet_len,
conn-client_encoding_initial);
packet_len += strlen(conn-client_encoding_initial) + 1;
}*
Is there any reason to check packet twice?.
And suppose packet is null then we will not append client_encoding into
the string but will increase the length(looks intentional! like in
ADD_STARTUP_OPTION).
In my point code should be like this
*if (conn-client_encoding_initial conn-client_encoding_initial[0])
{
if (packet)
{
strcpy(packet + packet_len, client_encoding);
packet_len += strlen(client_encoding) + 1;
strcpy(packet + packet_len,
conn-client_encoding_initial);
packet_len += strlen(conn-client_encoding_initial) +
1;
}
}*
*
*
*
*
BTW why you have not used ADD_STARTUP_OPTION?
I will test this patch on Windows and will send results.
--
Ibrar Ahmed
On Sun, Jan 30, 2011 at 10:56 AM, Heikki Linnakangas
heikki.linnakan...@enterprisedb.com wrote:
On 29.01.2011 19:23, Peter Eisentraut wrote:
Also, do we really need a new set of states for this..? I would have
thought, just reading through the patch, that we could use the
existing
OPTION_SEND/OPTION_WAIT states..
Don't know. Maybe Heikki could comment; he wrote that initially.
The other options send by the OPTION_SEND/WAIT machinery come from
environment variables, there's a getenv() call where the
SETENV_STATE_OPTION_SEND state is handled. It would be a bit awkward to
shoehorn client_encoding into that.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
Ibrar Ahmed
Re: [HACKERS] REVIEW: Determining client_encoding from client locale
On Wed, Feb 2, 2011 at 5:22 AM, Ibrar Ahmed ibrar.ah...@gmail.com wrote:
Hi!,
I have reviewed/tested this patch.
OS = Linux ubuntu 2.6.35-22-generic #33-Ubuntu SMP Sun Sep 19 20:34:50 UTC
2010 i686 GNU/Linux
PostgreSQL Version = Head (9.1devel)
Patch gives the desired results(still testing), but couple of questions
with this portion of the code.
*
if (conn-client_encoding_initial
conn-client_encoding_initial[0])
{
if (packet)
strcpy(packet + packet_len, client_encoding);
packet_len += strlen(client_encoding) + 1;
if (packet)
strcpy(packet + packet_len,
conn-client_encoding_initial);
packet_len += strlen(conn-client_encoding_initial) + 1;
}*
Is there any reason to check packet twice?.
And suppose packet is null then we will not append client_encoding into
the string but will increase the length(looks intentional! like in
ADD_STARTUP_OPTION).
I got the point, why we are doing this, just to calculate the length. Sorry
for this point.
In my point code should be like this
*if (conn-client_encoding_initial
conn-client_encoding_initial[0])
{
if (packet)
{
strcpy(packet + packet_len, client_encoding);
packet_len += strlen(client_encoding) + 1;
strcpy(packet + packet_len,
conn-client_encoding_initial);
packet_len += strlen(conn-client_encoding_initial)
+ 1;
}
}*
*
*
*
*
BTW why you have not used ADD_STARTUP_OPTION?
I will test this patch on Windows and will send results.
--
Ibrar Ahmed
On Sun, Jan 30, 2011 at 10:56 AM, Heikki Linnakangas
heikki.linnakan...@enterprisedb.com wrote:
On 29.01.2011 19:23, Peter Eisentraut wrote:
Also, do we really need a new set of states for this..? I would have
thought, just reading through the patch, that we could use the
existing
OPTION_SEND/OPTION_WAIT states..
Don't know. Maybe Heikki could comment; he wrote that initially.
The other options send by the OPTION_SEND/WAIT machinery come from
environment variables, there's a getenv() call where the
SETENV_STATE_OPTION_SEND state is handled. It would be a bit awkward to
shoehorn client_encoding into that.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
Ibrar Ahmed
--
Ibrar Ahmed
Re: [HACKERS] Review: B-Tree emulation for GIN
Thanks, On Fri, Dec 19, 2008 at 3:26 PM, Teodor Sigaev teo...@sigaev.ru wrote: Updated patch. Ibrar Ahmed wrote: Hi Teodor Sigaev, I am getting server crash in contrib regression. May be I am doing something wrong here. Regression diff is attached. BTW these queries work fine outside the regression. -- Ibrar Ahmed EnterpriseDB http://www.enterprisedb.com -- Teodor Sigaev E-mail: teo...@sigaev.ru WWW: http://www.sigaev.ru/ -- Ibrar Ahmed EnterpriseDB http://www.enterprisedb.com
[HACKERS] Review: B-Tree emulation for GIN
Hi Teodor Sigaev, I am getting server crash in contrib regression. May be I am doing something wrong here. Regression diff is attached. BTW these queries work fine outside the regression. -- Ibrar Ahmed EnterpriseDB http://www.enterprisedb.com 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] COCOMO Indians
Do we really need this kind of discussion here? On Thu, Dec 11, 2008 at 4:43 PM, Sreejesh O S [EMAIL PROTECTED] wrote: On Thu, Dec 11, 2008 at 3:13 PM, Dmitry Turin [EMAIL PROTECTED] wrote: Hi, Pgsql-hackers. We would like to obtain your opinion on these two questions: 1) We wanna append possibilities into Postgres engine, and wanna get top estimation for size of code, cost and time of implementation. 1.1) We divide possibilities to elementary features, find analogues in already written code, and suppose e.g., that quantity of lines for 'create timer' will be similar to 'create function', and that implementation for 'create timer' is easy than implementation of 'create function' (because it already has prototype in 'create function', and coping source code is possiblle) 1.2) We calculate cost and time by COCOMO http://en.wikipedia.org/wiki/Cocomo How relevant is this estimation ? 2) We are captivated by price of Indians, we listened much about low quality of code, written by Indians, How do you identify that the code written by Indians is low in quality ? Do you subcontracted to an Indian company or so ? we are fearing, that American company will resale implementation to Indian subcontractor (i.e. real developers will be Indians anyway). What requirements should satisfy code, written by Indians, to be in next version of Postgres ? Dmitry (SQL50, HTML60) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Ibrar Ahmed EnterpriseDB http://www.enterprisedb.com -- 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] pg_stop_backup wait bug fix
Hi, I have looked at the patch and it looks OK to me. BTW I am not too much familiar with this area of code, so I am not at the position to argue that patch -:) . I haven't found an easy way to test the patch. On Wed, Dec 3, 2008 at 1:24 PM, Heikki Linnakangas [EMAIL PROTECTED] wrote: Fujii Masao wrote: On Wed, Dec 3, 2008 at 5:13 AM, Heikki Linnakangas [EMAIL PROTECTED] wrote: Agreed, should use XLByteToPrevSeg. But I wonder if we can just replace the current XLByteToSeg call with XLByteToPrevSeg? That would offset the return value of the function by one byte as well, as well as the value printed to the backup history file. In fact, I think the original patch got that wrong; it would return the location of the *beginning* of the last xlog file. You're right. As you say, the value (stopxlogfilename) printed to the backup history file is wrong. But, since the value is not used fortunately, any troubles have not come up. So, I think that we can just replace them. Changing the return value doesn't seem like a good idea. If nothing else, it would be complicated to explain what it returns. I committed a patch that changes the waiting behavior, but not the return value or what's written into the backup label file, I also noticed that the 2nd BackupHistoryFileName call in that function is useless; histfilepath variable is already filled in earlier. Somewhat confusingly, BackupHistoryFileName is called only once. Isn't 1st (which probably you thought) BackupHistoryFilePath? Ouch, you're right. That's subtle. In order to prevent confusion, we should add new local variable (histfilename) for the backup history file name? Agreed. I included that in the patch. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Ibrar Ahmed EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [Review] Grouping Sets patch
((country),(male),(female)); SELECT country,male,female FROM population_tbl GROUP BY ALL ROLLUP((country,male),(female)); SELECT country,male,female FROM population_tbl GROUP BY DISTINCT ROLLUP(country,male,female); SELECT country,male,female FROM population_tbl GROUP BY DISTINCT ROLLUP(country,(male,female)); SELECT country,male,female FROM population_tbl GROUP BY DISTINCT ROLLUP(country,(male),(female)); SELECT country,male,female FROM population_tbl GROUP BY DISTINCT ROLLUP((country),(male),(female)); SELECT country,male,female FROM population_tbl GROUP BY DISTINCT ROLLUP((country,male),(female)); --CUBE SELECT country,male,female FROM population_tbl GROUP BY CUBE(country,male,female); SELECT country,male,female FROM population_tbl GROUP BY CUBE(country,(male,female)); SELECT country,male,female FROM population_tbl GROUP BY CUBE(country,(male),(female)); SELECT country,male,female FROM population_tbl GROUP BY CUBE((country),(male),(female)); SELECT country,male,female FROM population_tbl GROUP BY CUBE((country,male),(female)); SELECT country,male,female FROM population_tbl GROUP BY ALL CUBE(country,male,female); SELECT country,male,female FROM population_tbl GROUP BY ALL CUBE(country,(male,female)); SELECT country,male,female FROM population_tbl GROUP BY ALL CUBE(country,(male),(female)); SELECT country,male,female FROM population_tbl GROUP BY ALL CUBE((country),(male),(female)); SELECT country,male,female FROM population_tbl GROUP BY ALL CUBE((country,male),(female)); SELECT country,male,female FROM population_tbl GROUP BY DISTINCT CUBE(country,male,female); SELECT country,male,female FROM population_tbl GROUP BY DISTINCT CUBE(country,(male,female)); SELECT country,male,female FROM population_tbl GROUP BY DISTINCT CUBE(country,(male),(female)); SELECT country,male,female FROM population_tbl GROUP BY DISTINCT CUBE((country),(male),(female)); SELECT country,male,female FROM population_tbl GROUP BY DISTINCT CUBE((country,male),(female)); --Problems 1 - ORDER BY CLAUSE is not working after the patch --After Patch postgres=# SELECT country,male,female FROM population_tbl order by male DESC; country| male | female ---+--+ Australia |1 |100 Denmark |2 |200 Germany |3 |300 Netherlands |4 |400 United States |5 |500 Pakistan |6 |600 (6 rows) --Before patch --- postgres=# SELECT country,male,female FROM population_tbl order by male DESC; country| male | female ---+--+ Pakistan |6 |600 United States |5 |500 Netherlands |4 |400 Germany |3 |300 Denmark |2 |200 Australia |1 |100 (6 rows) Some Minor code observations -- 1 - IMHO we should use enum instead of #define i.e #define CUBE_OP 1 #define ROLLUP_OP 2 #define FUNCCALL_OP 3 -- Ibrar Ahmed EnterpriseDB http://www.enterprisedb.com -- 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] New bug
Hi, It works fine on my machine [EMAIL PROTECTED]:/usr/local/pgsql/bin$ ./psql postgres psql (8.4devel) Type help for help. postgres=# SELECT res, * FROM ( SELECT 'foo'||i AS test, i AS res FROM (VALUES (1)) AS x(i) UNION ALL SELECT 'foo'||i, i FROM (VALUES (2)) AS x(i) ) AS x; res | test | res -+--+- 1 | foo1 | 1 2 | foo2 | 2 (2 rows) postgres=# On Wed, Nov 19, 2008 at 8:38 PM, Gregory Stark [EMAIL PROTECTED] wrote: I haven't looked into what's causing it yet but... postgres=# SELECT res, * FROM ( SELECT 'foo'||i AS test, i AS res FROM (VALUES (1)) AS x(i) UNION ALL SELECT 'foo'||i, i FROM (VALUES (2)) AS x(i) ) AS x; server closed the connection unexpectedly This probably means the server terminated abnormally before or while processing the request. The connection to the server was lost. Attempting reset: Failed. TRAP: FailedAssertion(!(child_attr = new_max_attr), File: prepunion.c, Line: 1726) Works fine on 8.3 so even though that section of code in prepunion.c hasn't changed: postgres=# SELECT res, * FROM (SELECT 'foo'||i AS test, i AS res FROM (VALUES (1)) AS x(i) UNION ALL SELECT 'foo'||i, i FROM (VALUES (2)) AS x(i)) AS x ; res | test | res -+--+- 1 | foo1 | 1 2 | foo2 | 2 (2 rows) -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's PostGIS support! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Ibrar Ahmed EnterpriseDB http://www.enterprisedb.com -- 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] quick question about WIP: grouping sets support
Hi, I am able to apply your patch successfully but I am still getting compilation error ./configure --enable-depend --enable-cassert make gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing -fwrapv -I. -I../../../src/include -D_GNU_SOURCE -c -o scansup.o scansup.c -MMD -MP -MF .deps/scansup.Po gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing -fwrapv -I. -I../../../src/include -D_GNU_SOURCE -c -o parse_gsets.o parse_gsets.c -MMD -MP -MF .deps/parse_gsets.Po parse_gsets.c:846: error: conflicting types for 'transform_ungroup_cols_context' parse_gsets.c:48: error: previous declaration of 'transform_ungroup_cols_context' was here parse_gsets.c:850: error: conflicting types for 'set_multiplication' parse_gsets.c:548: error: previous definition of 'set_multiplication' was here parse_gsets.c:861: error: redefinition of 'expandGSOperators' parse_gsets.c:63: error: previous definition of 'expandGSOperators' was here parse_gsets.c:948: error: redefinition of 'adjustFields' parse_gsets.c:173: error: previous definition of 'adjustFields' was here parse_gsets.c:1000: error: redefinition of 'expandGSOperator' parse_gsets.c:225: error: previous definition of 'expandGSOperator' was here parse_gsets.c:1181: error: redefinition of 'add_unique_gsets' parse_gsets.c:406: error: previous definition of 'add_unique_gsets' was here parse_gsets.c:1236: error: redefinition of 'multiple' parse_gsets.c:461: error: previous definition of 'multiple' was here parse_gsets.c:1388: error: conflicting types for 'transform_ungroup_cols_mutator' parse_gsets.c:624: error: previous definition of 'transform_ungroup_cols_mutator' was here parse_gsets.c:1449: error: redefinition of 'transformGroupingSetsSpec' parse_gsets.c:689: error: previous definition of 'transformGroupingSetsSpec' was here parse_gsets.c:1607: error: conflicting types for 'transform_ungroup_cols_context' parse_gsets.c:846: error: previous declaration of 'transform_ungroup_cols_context' was here parse_gsets.c:1622: error: redefinition of 'expandGSOperators' parse_gsets.c:63: error: previous definition of 'expandGSOperators' was here parse_gsets.c:1709: error: redefinition of 'adjustFields' parse_gsets.c:948: error: previous definition of 'adjustFields' was here parse_gsets.c:1761: error: redefinition of 'expandGSOperator' parse_gsets.c:1000: error: previous definition of 'expandGSOperator' was here parse_gsets.c:1942: error: redefinition of 'add_unique_gsets' parse_gsets.c:1181: error: previous definition of 'add_unique_gsets' was here parse_gsets.c:1997: error: redefinition of 'multiple' parse_gsets.c:1236: error: previous definition of 'multiple' was here parse_gsets.c:2084: error: redefinition of 'set_multiplication' parse_gsets.c:1323: error: previous definition of 'set_multiplication' was here parse_gsets.c:2149: error: conflicting types for 'transform_ungroup_cols_mutator' parse_gsets.c:1388: error: previous definition of 'transform_ungroup_cols_mutator' was here parse_gsets.c:2210: error: redefinition of 'transformGroupingSetsSpec' parse_gsets.c:689: error: previous definition of 'transformGroupingSetsSpec' was here make[3]: *** [parse_gsets.o] Error 1 make[3]: Leaving directory `/home/ibrar/edb-work/PostgreSQL/postgresql/src/backend/parser' make[2]: *** [parser-recursive] Error 2 make[2]: Leaving directory `/home/ibrar/edb-work/PostgreSQL/postgresql/src/backend' make[1]: *** [all] Error 2 make[1]: Leaving directory `/home/ibrar/edb-work/PostgreSQL/postgresql/src' make: *** [all] Error 2 On Tue, Nov 11, 2008 at 9:52 PM, Pavel Stehule [EMAIL PROTECTED] wrote: Hello I synced grouping sets with current CVS HEAD. Please, try: http://www.pgsql.cz/patches/gsets.diff.gz Thank you Pavel Stehule 2008/11/10 Ibrar Ahmed [EMAIL PROTECTED]: Hi , While I am looking at your patch I am getting compilation error. BTW I have downloaded your patch from this link. http://archives.postgresql.org/message-id/[EMAIL PROTECTED] gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing -fwrapv -I. -I../../../src/include -D_GNU_SOURCE -c -o parse_gsets.o parse_gsets.c -MMD -MP -MF .deps/parse_gsets.Po parse_gsets.c:809: error: conflicting types for 'transform_ungroup_cols_context' parse_gsets.c:48: error: previous declaration of 'transform_ungroup_cols_context' was here parse_gsets.c:824: error: redefinition of 'expandGSOperators' parse_gsets.c:63: error: previous definition of 'expandGSOperators' was here parse_gsets.c:911: error: redefinition of 'adjustFields' parse_gsets.c:150: error: previous definition of 'adjustFields' was here parse_gsets.c:963: error: redefinition of 'expandGSOperator' parse_gsets.c:202: error: previous definition of 'expandGSOperator' was here parse_gsets.c:1144: error: redefinition of 'add_unique_gsets' parse_gsets.c:383: error
[HACKERS] server crash in to_timestamp function
Hi, While looking at the code base I have encountered a server crash in to_timestamp function. select TO_TIMESTAMP ( '2006 1', ' Q' ); server closed the connection unexpectedly This probably means the server terminated abnormally before or while processing the request. The connection to the server was lost. Attempting reset: Failed. I further debugged the issue and here are my thoughts [function DCH_from_char] ... case DCH_Q: /* * We ignore Q when converting to date because it is not * normative. * * We still parse the source string for an integer, but it * isn't stored anywhere in 'out'. */ from_char_parse_int((int *) NULL, s, n); s += SKIP_THth(n-suffix); ... This piece of code is calling function from_char_parse_int with first argument NULL. The function from_char_parse_int in turn calls from_char_parse_int_len which in turn calls from_char_set_int. In the function from_char_set_int the first argument dest is being derefernced without the null check. (if (*dest != 0 *dest != value) -- Ibrar Ahmed EnterpriseDB http://www.enterprisedb.com -- 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] pg_dump roles support [Review]
On Thu, Nov 6, 2008 at 8:08 PM, Benedek László [EMAIL PROTECTED] wrote: Hi, Thanks for your review. I created an updated patch according to your notices. 1 - Patch does not apply cleanly on latest git repository, although there is no hunk failed but there are some hunk succeeded messages. Rebased to the current HEAD. 2- Patch contains unnecessary spaces and tabs which makes the patch unnecessarily big. IMHO please read the patch before sending and make sure that patch only contains the changes you intended to send. Yes, there were trailing whitespaces in the original files which were removed by the previous patch. The attached version leaves them as is. 3 - We should follow the coding standards of existing code I tried, of course, but this escaped my observation. 4 - pg_restore gives error wile restoring custom format backup 5 - Do you really want to write this code like this Fixed. I also need some feedback about the role support in pg_restore (not implemented yet). Currently pg_restore sets the role during the restore process according to the TOC entry in the archive. It may also support the --role option (just like pg_dump). If specified it can be used to cancel the effect of the TOC entry and force the emitting of the SET ROLE ... command. With emtpy argument it can be used to omit the SET ROLE even if it is specified in the archieve. What do you think? Now this patch looks OK to me. As for as pg_restore is concern I think we should not add this option into pg_restore. What advantages do you want to get by using SET ROLE in pg_restore? Thank you again. doc/src/sgml/ref/pg_dump.sgml| 16 ++ doc/src/sgml/ref/pg_dumpall.sgml | 15 + src/bin/pg_dump/pg_backup.h |2 + src/bin/pg_dump/pg_backup_archiver.c | 36 +- src/bin/pg_dump/pg_dump.c| 53 ++ src/bin/pg_dump/pg_dumpall.c | 23 ++ 6 files changed, 143 insertions(+), 2 deletions(-) -- Ibrar Ahmed EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pg_dump roles support [Review]
Just a superficial review. I haven't really looked hard at this yet.
1 - Patch does not apply cleanly on latest git repository, although
there is no hunk failed but there are some hunk succeeded messages.
2- Patch contains unnecessary spaces and tabs which makes the patch
unnecessarily big. IMHO please read the patch before sending and make
sure that patch only contains the changes you intended to send.
3 - We should follow the coding standards of existing code
destroyPQExpBuffer(roleQry);
g_fout-rolename = pgrole;
} else {
g_fout-rolename = NULL;
}
Should be written like this
destroyPQExpBuffer(roleQry);
g_fout-rolename = pgrole;
}
else
{
g_fout-rolename = NULL;
}
4 - pg_restore gives error wile restoring custom format backup
pg_restore: [archiver] invalid ROLENAME item: SET role = 'ibrar';
(reason check point 5)
5 - Do you really want to write this code like this
+ if (ptr2)
+ {
+ *ptr2 = '\0';
+ AH-public.rolename = strdup(ptr1);
+ free(defn);5 -
+ }
+ else
+ free(defn);
+ die_horribly(AH, modulename, invalid ROLENAME item: %s\n,
+te-defn);
I think you missed curly brackets of else here.
Please send updated patch!
--
Ibrar Ahmed
EnterpriseDB http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] ERRORDATA_STACK_SIZE exceeded (server crash)
Hi, I have encountered a server crash while working with different locale settings. After searching on the internet I have seen a similar issue with 8.3.1 release and Tom has fixed that issue. That bug was only in Windows but I am getting same server crash on Linux, although I am using a later release with the patch applied. Here is the link of previous bug http://archives.postgresql.org/pgsql-committers/2008-05/msg00349.php OS = ubuntu PG Version = psql (8.4devel) postgres=# show server_encoding; server_encoding - UTF8 (1건 있음) postgres=# show client_encoding; client_encoding - UTF8 (1건 있음) postgres=# set client_encoding ='euc-jp'; SET postgres=# x; 서버가 갑자기 연결을 닫았음 이런 처리는 클라이언트의 요구를 처리하는 동안이나 처리하기 전에 서버가 갑자기 종료되었음을 의미함 서버로부터 연결이 끊어졌습니다. 다시 연결을 시도합니다: 실패. ! -- Ibrar Ahmed EnterpriseDB http://www.enterprisedb.com -- 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] ERRORDATA_STACK_SIZE exceeded (server crash)
Sure! CODE -- /configure --enable-nls --enable-depend --enable-debug make make install SERVER SIDE - 1 - export LANG=ko_KR.UTF-8 2 - ./initdb -E UTF8 -D ../data 3 - ./postmaster -D ../data CLIENT SIDE --- 1 - export LANG=ko_KR.UTF-8 2 - psql postgres postgres=# show server_encoding; server_encoding - UTF8 (1건 있음) postgres=# show client_encoding; client_encoding - UTF8 (1건 있음) postgres=# set client_encoding ='euc-jp';--[--Negative test scenario] SET postgres=# x; On Mon, Oct 27, 2008 at 6:42 PM, Tom Lane [EMAIL PROTECTED] wrote: Ibrar Ahmed [EMAIL PROTECTED] writes: I have encountered a server crash while working with different locale settings. Are you going to give us a hint what settings those would be? regards, tom lane -- Ibrar Ahmed EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [Review] fix dblink security hole
Hi,
Here is my review of the dblink security hole patch written by Marko Kreen:
http://archives.postgresql.org/message-id/[EMAIL PROTECTED]
1 - The patch applies cleanly to the latest GIT repository.
2 - Regression passed before/after the patch.
3 - I have played around with the patch a little and haven't found any
issue.
[Code review]
4 - In my opinion we should not add the superuser check in
DBLINK_GET_CONN macro and in dblink_connect function because we
already have a function named dblink_security_check and there is a
superuser check in the function. In my opinion we should use this
function and throw an error if non super user is detected, because
calling superuser function in multiple places is not a good idea.
Another point is that if we are not using the function
dblink_security_check then we should delete that function because
after the patch there will be no effect of this function (I think).
I have attached a patch just for the quick thought. Otherwise there is
no issue with patch.
[Reviewing a Patch] from the link
http://wiki.postgresql.org/wiki/Reviewing_a_Patch;
Submission review
-
- Is the patch in the standard form?( Yes )
- Does it apply cleanly to the current CVS HEAD?( I checked it
with the latest git repository)
- Does it include reasonable tests, docs, etc? ( In my opinion
there should be couple of test cases )
Usability review
--
- Does the patch actually implement that? (Yes)
- Do we want that?(Not sure about that
because we are restricting non super user to use dblink )
- Do we already have it?
- Does it follow SQL spec, or the community-agreed behavior? (Looks
like)
- Are there dangers? (I
don't think so )
- Have all the bases been covered? (Yes)
Feature test
- Does the feature work as advertised? (Yes)
- Are there corner cases the author has failed to consider?(I
don't think so)
Performance review
--
- Does the patch slow down simple tests? (No)
- If it claims to improve performance, does it? (No)
- Does it slow down other things? (No)
Architecture review
---
- Is everything done in a way that fits together coherently with
other features/modules? (I have added comments above)
- Are there interdependencies than can cause
problems? (I don't think so)
Review review
-
- Did the reviewer cover all the things that kind of reviewer is
supposed to do? (should I answer this too)
--
Ibrar Ahmed
EnterpriseDB http://www.enterprisedb.com
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 0e01470..0f90f1e 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -164,6 +164,7 @@ typedef struct remoteConnHashEnt
} \
else \
{ \
+dblink_security_check(conn, rconn); \
connstr = conname_or_str; \
conn = PQconnectdb(connstr); \
if (PQstatus(conn) == CONNECTION_BAD) \
@@ -175,7 +176,6 @@ typedef struct remoteConnHashEnt
errmsg(could not establish connection), \
errdetail(%s, msg))); \
} \
-dblink_security_check(conn, rconn); \
freeconn = true; \
} \
} while (0)
@@ -215,6 +215,8 @@ dblink_connect(PG_FUNCTION_ARGS)
PGconn *conn = NULL;
remoteConn *rconn = NULL;
+ dblink_security_check(conn, rconn);
+
DBLINK_INIT;
if (PG_NARGS() == 2)
@@ -246,9 +248,6 @@ dblink_connect(PG_FUNCTION_ARGS)
errdetail(%s, msg)));
}
- /* check password used if not superuser */
- dblink_security_check(conn, rconn);
-
if (connname)
{
rconn-conn = conn;
@@ -2236,6 +2235,11 @@ dblink_security_check(PGconn *conn, remoteConn *rconn)
{
if (!superuser())
{
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg(only superuser can specify connect string / call \dblink_connect()\)));
+#if 0
+
if (!PQconnectionUsedPassword(conn))
{
PQfinish(conn);
@@ -2248,6 +2252,7 @@ dblink_security_check(PGconn *conn, remoteConn *rconn)
errdetail(Non-superuser cannot connect if the server does not request a password.),
errhint(Target server's authentication method must be changed.)));
}
+#endif
}
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [Review] fix dblink security hole
Hi,
Here is my review of the dblink security hole patch written by Marko Kreen:
http://archives.postgresql.org/message-id/[EMAIL PROTECTED]
1 - The patch applies cleanly to the latest GIT repository.
2 - Regression passed before/after the patch.
3 - I have played around with the patch a little and haven't found any
issue.
[Code review]
4 - In my opinion we should not add the superuser check in DBLINK_GET_CONN
macro and in dblink_connect function because we already have a function
named dblink_security_check and there is a superuser check in the
function. In my opinion we should use this function and throw an error if
non super user is detected, because calling superuser function in multiple
places is not a good idea. Another point is that if we are not using the
function dblink_security_check then we should delete that function because
after the patch there will be no effect of this function (I think).
I have attached a patch just for the quick thought. Otherwise there is no
issue with patch.
[Reviewing a Patch] from the link
http://wiki.postgresql.org/wiki/Reviewing_a_Patch;http://wiki.postgresql.org/wiki/Reviewing_a_Patch
Submission review
-
- Is the patch in the standard form? ( Yes )
- Does it apply cleanly to the current CVS HEAD?( I checked it with the
latest git repository)
- Does it include reasonable tests, docs, etc? ( In my opinion
there should be couple of test cases )
Usability review
--
- Does the patch actually implement that? (Yes)
- Do we want that?(Not sure about that because we
are restricting non super user to use dblink )
- Do we already have it?
- Does it follow SQL spec, or the community-agreed behavior? (Looks like)
- Are there dangers? (I don't
think so )
- Have all the bases been covered? (Yes)
Feature test
- Does the feature work as advertised? (Yes)
- Are there corner cases the author has failed to consider?(I don't
think so)
Performance review
--
- Does the patch slow down simple tests? (No)
- If it claims to improve performance, does it? (No)
- Does it slow down other things? (No)
Architecture review
---
- Is everything done in a way that fits together coherently with other
features/modules? (I have added comments above)
- Are there interdependencies than can cause
problems? (I don't think so)
Review review
-
- Did the reviewer cover all the things that kind of reviewer is supposed
to do? (should I answer this too)
--
Ibrar Ahmed
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 0e01470..0f90f1e 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -164,6 +164,7 @@ typedef struct remoteConnHashEnt
} \
else \
{ \
+dblink_security_check(conn, rconn); \
connstr = conname_or_str; \
conn = PQconnectdb(connstr); \
if (PQstatus(conn) == CONNECTION_BAD) \
@@ -175,7 +176,6 @@ typedef struct remoteConnHashEnt
errmsg(could not establish connection), \
errdetail(%s, msg))); \
} \
-dblink_security_check(conn, rconn); \
freeconn = true; \
} \
} while (0)
@@ -215,6 +215,8 @@ dblink_connect(PG_FUNCTION_ARGS)
PGconn *conn = NULL;
remoteConn *rconn = NULL;
+ dblink_security_check(conn, rconn);
+
DBLINK_INIT;
if (PG_NARGS() == 2)
@@ -246,9 +248,6 @@ dblink_connect(PG_FUNCTION_ARGS)
errdetail(%s, msg)));
}
- /* check password used if not superuser */
- dblink_security_check(conn, rconn);
-
if (connname)
{
rconn-conn = conn;
@@ -2236,6 +2235,11 @@ dblink_security_check(PGconn *conn, remoteConn *rconn)
{
if (!superuser())
{
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg(only superuser can specify connect string / call \dblink_connect()\)));
+#if 0
+
if (!PQconnectionUsedPassword(conn))
{
PQfinish(conn);
@@ -2248,6 +2252,7 @@ dblink_security_check(PGconn *conn, remoteConn *rconn)
errdetail(Non-superuser cannot connect if the server does not request a password.),
errhint(Target server's authentication method must be changed.)));
}
+#endif
}
}
--
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] [Review] fix dblink security hole
I have attached a patch just for the quick thought. Otherwise there is no issue with patch. This is no good - the security_check() needs established connection to work on. I know it but after putting the superuser check just above the security_check function make this function almost useless and now it doesn't need PGconn parameter. BTW it was just my suggestion otherwise patch looks ok to me -- Ibrar Ahmed
Re: [HACKERS] Need more reviewers!
Josh Berkus wrote: Hackers, We currently have 38 patches pending, and only nine people reviewing them. At this rate, the September commitfest will take three months. If you are a postgresql hacker at all, or even want to be one, we need your help reviewing patches! There are several easy patches in the list, so I can assign them to beginners. Please volunteer now! Please assign me one; any of the easy ones. -- Ibrar Ahmed EnterpriseDB http://www.enterprisedb.com -- 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] Need more reviewers!
Josh Berkus wrote: Hackers, We currently have 38 patches pending, and only nine people reviewing them. At this rate, the September commitfest will take three months. If you are a postgresql hacker at all, or even want to be one, we need your help reviewing patches! There are several easy patches in the list, so I can assign them to beginners. Please volunteer now! Please assign me one; any of the easy ones. -- Ibrar Ahmed EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
