Re: [HACKERS] [PATCH] Add two-arg for of current_setting(NAME, FALLBACK)
> On Nov 1, 2017, at 1:02 PM, Nico Williams <n...@cryptonector.com> wrote: > > On Thu, Mar 19, 2015 at 05:41:02PM -0500, David Christensen wrote: >> The two-arg form of the current_setting() function will allow a >> fallback value to be returned instead of throwing an error when an >> unknown GUC is provided. This would come in most useful when using >> custom GUCs; e.g.: > > There already _is_ a two-argument form of current_setting() that yours > somewhat conflicts with: > > current_setting(setting_name [, missing_ok ]) > > https://www.postgresql.org/docs/current/static/functions-admin.html Apologies; I have no idea how this email got re-sent, but it is quite old and a mistake. David -- David Christensen End Point Corporation da...@endpoint.com 785-727-1171 signature.asc Description: Message signed with OpenPGP
[HACKERS] [PATCH] Add two-arg for of current_setting(NAME, FALLBACK)
The two-arg form of the current_setting() function will allow a
fallback value to be returned instead of throwing an error when an
unknown GUC is provided. This would come in most useful when using
custom GUCs; e.g.:
-- errors out if the 'foo.bar' setting is unset
SELECT current_setting('foo.bar');
-- returns current setting of foo.bar, or 'default' if not set
SELECT current_setting('foo.bar', 'default')
This would save you having to wrap the use of the function in an
exception block just to catch and utilize a default setting value
within a function.
---
src/backend/utils/misc/guc.c | 50 ---
src/include/catalog/pg_proc.h | 2 ++
src/include/utils/builtins.h | 1 +
src/include/utils/guc.h | 1 +
src/test/regress/expected/guc.out | 19 +++
src/test/regress/sql/guc.sql | 12 ++
6 files changed, 82 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 26275bd..002a926 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -7703,13 +7703,32 @@ ShowAllGUCConfig(DestReceiver *dest)
char *
GetConfigOptionByName(const char *name, const char **varname)
{
+ return GetConfigOptionByNameFallback(name, NULL, varname);
+}
+
+/*
+ * Return GUC variable value by name; optionally return canonical form of
+ * name. If GUC is NULL then optionally return a fallback value instead of an
+ * error. Return value is palloc'd.
+ */
+char *
+GetConfigOptionByNameFallback(const char *name, const char *default_value,
const char **varname)
+{
struct config_generic *record;
record = find_option(name, false, ERROR);
if (record == NULL)
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("unrecognized configuration parameter
\"%s\"", name)));
+ {
+ if (default_value) {
+ return pstrdup(default_value);
+ }
+ else
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("unrecognized configuration parameter
\"%s\"", name)));
+ }
+ }
if ((record->flags & GUC_SUPERUSER_ONLY) && !superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
@@ -8008,6 +8027,31 @@ show_config_by_name(PG_FUNCTION_ARGS)
}
/*
+ * show_config_by_name_fallback - equiv to SHOW X command but implemented as
+ * a function. If X does not exist, return a fallback datum instead of
erroring
+ */
+Datum
+show_config_by_name_fallback(PG_FUNCTION_ARGS)
+{
+ char *varname;
+ char *varfallback;
+ char *varval;
+
+ /* Get the GUC variable name */
+ varname = TextDatumGetCString(PG_GETARG_DATUM(0));
+
+ /* Get the fallback value */
+ varfallback = TextDatumGetCString(PG_GETARG_DATUM(1));
+
+ /* Get the value */
+ varval = GetConfigOptionByNameFallback(varname, varfallback, NULL);
+
+ /* Convert to text */
+ PG_RETURN_TEXT_P(cstring_to_text(varval));
+}
+
+
+/*
* show_all_settings - equiv to SHOW ALL command but implemented as
* a Table Function.
*/
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 6a757f3..71efed2 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -3025,6 +3025,8 @@ DESCR("convert bitstring to int8");
DATA(insert OID = 2077 ( current_setting PGNSP PGUID 12 1 0 0 0 f f f f
t f s 1 0 25 "25" _null_ _null_ _null_ _null_ show_config_by_name _null_ _null_
_null_ ));
DESCR("SHOW X as a function");
+DATA(insert OID = 3280 ( current_setting PGNSP PGUID 12 1 0 0 0 f f f f
t f s 2 0 25 "25 25" _null_ _null_ _null_ _null_ show_config_by_name_fallback
_null_ _null_ _null_ ));
+DESCR("SHOW X as a function");
DATA(insert OID = 2078 ( set_config PGNSP PGUID 12 1 0 0 0 f f f f
f f v 3 0 25 "25 25 16" _null_ _null_ _null_ _null_ set_config_by_name _null_
_null_ _null_ ));
DESCR("SET X as a function");
DATA(insert OID = 2084 ( pg_show_all_settings PGNSP PGUID 12 1 1000 0 0 f f f
f t t s 0 0 2249 "" "{25,25,25,25,25,25,25,25,25,25,25,1009,25,25,25,23}"
"{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}"
"{name,setting,unit,category,short_desc,extra_desc,context,vartype,source,min_val,max_val,enumvals,boot_val,reset_val,sourcefile,sourceline}"
_null_ show_all_settings _null_ _null_ _null_ ));
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index bc4517d..e3d9fbb 100644
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -1088,6 +1088,7 @@ extern Datum quote_nullable(PG_FUNCTION_ARGS);
/* guc.c */
extern Datum show_config_by_name(PG_FUNCTION_ARGS);
+extern Datum
[HACKERS] [PATCH] Add two-arg for of current_setting(NAME, FALLBACK)
The two-arg form of the current_setting() function will allow a
fallback value to be returned instead of throwing an error when an
unknown GUC is provided. This would come in most useful when using
custom GUCs; e.g.:
-- errors out if the 'foo.bar' setting is unset
SELECT current_setting('foo.bar');
-- returns current setting of foo.bar, or 'default' if not set
SELECT current_setting('foo.bar', 'default')
This would save you having to wrap the use of the function in an
exception block just to catch and utilize a default setting value
within a function.
---
src/backend/utils/misc/guc.c | 50 ---
src/include/catalog/pg_proc.h | 2 ++
src/include/utils/builtins.h | 1 +
src/include/utils/guc.h | 1 +
src/test/regress/expected/guc.out | 19 +++
src/test/regress/sql/guc.sql | 12 ++
6 files changed, 82 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 26275bd..002a926 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -7703,13 +7703,32 @@ ShowAllGUCConfig(DestReceiver *dest)
char *
GetConfigOptionByName(const char *name, const char **varname)
{
+ return GetConfigOptionByNameFallback(name, NULL, varname);
+}
+
+/*
+ * Return GUC variable value by name; optionally return canonical form of
+ * name. If GUC is NULL then optionally return a fallback value instead of an
+ * error. Return value is palloc'd.
+ */
+char *
+GetConfigOptionByNameFallback(const char *name, const char *default_value,
const char **varname)
+{
struct config_generic *record;
record = find_option(name, false, ERROR);
if (record == NULL)
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("unrecognized configuration parameter
\"%s\"", name)));
+ {
+ if (default_value) {
+ return pstrdup(default_value);
+ }
+ else
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("unrecognized configuration parameter
\"%s\"", name)));
+ }
+ }
if ((record->flags & GUC_SUPERUSER_ONLY) && !superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
@@ -8008,6 +8027,31 @@ show_config_by_name(PG_FUNCTION_ARGS)
}
/*
+ * show_config_by_name_fallback - equiv to SHOW X command but implemented as
+ * a function. If X does not exist, return a fallback datum instead of
erroring
+ */
+Datum
+show_config_by_name_fallback(PG_FUNCTION_ARGS)
+{
+ char *varname;
+ char *varfallback;
+ char *varval;
+
+ /* Get the GUC variable name */
+ varname = TextDatumGetCString(PG_GETARG_DATUM(0));
+
+ /* Get the fallback value */
+ varfallback = TextDatumGetCString(PG_GETARG_DATUM(1));
+
+ /* Get the value */
+ varval = GetConfigOptionByNameFallback(varname, varfallback, NULL);
+
+ /* Convert to text */
+ PG_RETURN_TEXT_P(cstring_to_text(varval));
+}
+
+
+/*
* show_all_settings - equiv to SHOW ALL command but implemented as
* a Table Function.
*/
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 6a757f3..71efed2 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -3025,6 +3025,8 @@ DESCR("convert bitstring to int8");
DATA(insert OID = 2077 ( current_setting PGNSP PGUID 12 1 0 0 0 f f f f
t f s 1 0 25 "25" _null_ _null_ _null_ _null_ show_config_by_name _null_ _null_
_null_ ));
DESCR("SHOW X as a function");
+DATA(insert OID = 3280 ( current_setting PGNSP PGUID 12 1 0 0 0 f f f f
t f s 2 0 25 "25 25" _null_ _null_ _null_ _null_ show_config_by_name_fallback
_null_ _null_ _null_ ));
+DESCR("SHOW X as a function");
DATA(insert OID = 2078 ( set_config PGNSP PGUID 12 1 0 0 0 f f f f
f f v 3 0 25 "25 25 16" _null_ _null_ _null_ _null_ set_config_by_name _null_
_null_ _null_ ));
DESCR("SET X as a function");
DATA(insert OID = 2084 ( pg_show_all_settings PGNSP PGUID 12 1 1000 0 0 f f f
f t t s 0 0 2249 "" "{25,25,25,25,25,25,25,25,25,25,25,1009,25,25,25,23}"
"{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}"
"{name,setting,unit,category,short_desc,extra_desc,context,vartype,source,min_val,max_val,enumvals,boot_val,reset_val,sourcefile,sourceline}"
_null_ show_all_settings _null_ _null_ _null_ ));
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index bc4517d..e3d9fbb 100644
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -1088,6 +1088,7 @@ extern Datum quote_nullable(PG_FUNCTION_ARGS);
/* guc.c */
extern Datum show_config_by_name(PG_FUNCTION_ARGS);
+extern Datum
[HACKERS] [PATCH] Remove defunct and unnecessary link
The HA docs reference a “glossary” link which is no longer accessible, nor is it likely to be useful in general to link off-site IMHO. This simple patch removes this link. Best, David -- David Christensen End Point Corporation da...@endpoint.com 785-727-1171 0001-Remove-defunct-and-unnecessary-doc-link.patch 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] [PATCH] Add pg_disable_checksums() and supporting infrastructure
> On Mar 9, 2017, at 1:01 PM, Robert Haas <robertmh...@gmail.com> wrote: > > On Sun, Feb 19, 2017 at 12:02 PM, David Christensen <da...@endpoint.com> > wrote: >> Hi Robert, this is part of a larger patch which *does* enable the checksums >> online; I’ve been extracting the necessary pieces out with the understanding >> that some people thought the disable code might be useful in its own merit. >> I can add documentation for the various states. The CHECKSUM_REVALIDATE is >> the only one I feel is a little wibbly-wobbly; the other states are required >> because of the fact that enabling checksums requires distinguishing between >> “in process of enabling” and “everything is enabled”. > > Ah, sorry, I had missed that patch. > >> My design notes for the patch were submitted to the list with little >> comment; see: >> https://www.postgresql.org/message-id/1E6E64E9-634B-43F4-8AA2-CD85AD92D2F8%40endpoint.com >> >> I have since added the WAL logging of checksum states, however I’d be glad >> to take feedback on the other proposed approaches (particularly the system >> catalog changes + the concept of a checksum cycle).] > > I think the concept of a checksum cycle might be overkill. It would > be useful if we ever wanted to change the checksum algorithm, but I > don't see any particular reason why we need to build infrastructure > for that. If we wanted to change the checksum algorithm, I think it > likely that we'd do that in the course of, say, widening it to 4 bytes > as part of a bigger change in the page format, and this infrastructure > would be too narrow to help with that. I hear what you are saying, however a boolean on pg_class/pg_database to show if the relation in question is insufficient if we allow arbitrary enabling/disabling while enabling is in progress. In particular, if we disable checksums while the enabling was in progress and had only a boolean to indicate whether the checksums are complete for a relation there will have been a window when new pages in a relation will *not* be checksummed but the system table flag will indicate that the checksum is up-to-date, which is false. This would lead to checksum failures when those pages are encountered. Similar failures will occur as well when doing a pg_upgrade of an in-progress enabling. Saying you can’t disable/cancel the checksum process while it’s running seems undesirable too, as what happens if you have a system failure mid-process. We could certainly avoid this problem by just saying that we have to start over with the entire cluster and process every page from scratch rather than trying to preserve/track state that we know is good, perhaps only dirtying buffers which have a non-matching checksum while we’re in the conversion state, but this seems undesirable to me, plus if we made it work in a single session we’d have a long-running snapshot open (presumably) which would wreak havoc while it processes the entire database (as someone had suggested by doing it in a single function that just hangs while it’s running) I think we still need a way to short-circuit the process/incrementally update and note which tables have been checksummed and which ones haven’t. (Perhaps I could make the code which currently checks DataChecksumsEnabled() a little smarter, depending on the relation it’s looking at.) As per one of Jim’s suggestions, I’ve been looking at making the data_checkum_state localized per database at postinit time, but really it probably doesn’t matter to anything but the buffer code whether it’s a global setting, a per-database setting or what. > While I'm glad to see work finally going on to allow enabling and > disabling checksums, I think it's too late to put this in v10. We > have a rough rule that large new patches shouldn't be appearing for > the first time in the last CommitFest, and I think this falls into > that category. I also think it would be unwise to commit just the > bits of the infrastructure that let us disable checksums without > having time for a thorough review of the whole thing; to me, the > chances that we'll make decisions that we later regret seem fairly > high. I would rather wait for v11 and have a little more time to try > to get everything right. Makes sense, but to that end I think we need to have at least some agreement on how this should work ahead of time. Obviously it’s easier to critique existing code, but I don’t want to go out in left field of a particular approach is going to be obviously unworkable per some of the more experienced devs here. -- David Christensen End Point Corporation da...@endpoint.com 785-727-1171 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [PATCH] Teach Catalog.pm how many attributes there should be per DATA() line
> Hi David, > > Here's a review of your patch. Hi Ilmari, thanks for your time and review. I’m fine with the revised version. Best, David -- David Christensen End Point Corporation da...@endpoint.com 785-727-1171 -- 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] [PATCH] Add pg_disable_checksums() and supporting infrastructure
> On Feb 19, 2017, at 8:14 PM, Jim Nasby <jim.na...@bluetreble.com> wrote: > > On 2/19/17 11:02 AM, David Christensen wrote: >> My design notes for the patch were submitted to the list with little >> comment; see: >> https://www.postgresql.org/message-id/1E6E64E9-634B-43F4-8AA2-CD85AD92D2F8%40endpoint.com >> >> I have since added the WAL logging of checksum states, however I’d be glad >> to take feedback on the other proposed approaches (particularly the system >> catalog changes + the concept of a checksum cycle).] > > A couple notes: > > - AFAIK unlogged tables get checksummed today if checksums are enabled; the > same should hold true if someone enables checksums on the whole cluster. Agreed; AFAIK this should already be working if it’s using the PageIsVerified() API, since I just effectively modified the logic there, depending on state. > - Shared relations should be handled as well; you don't mention them. I agree that they should be handled as well; I thought I had mentioned it later in the design doc, though TBH I’m not sure if there is more involved than just visiting the global relations in pg_class. In addition we need to visit all forks of each relation. I’m not certain if loading those into shared_buffers would be sufficient; I think so, but I’d be glad to be informed otherwise. (As long as they’re already using the PageIsVerified() API call they get this logic for free. > - If an entire cluster is going to be considered as checksummed, then even > databases that don't allow connections would need to get enabled. Yeah, the workaround for now would be to require “datallowconn" to be set to ’t' for all databases before proceeding, unless there’s a way to connect to those databases internally that bypasses that check. Open to ideas, though for a first pass seems like the “datallowconn” approach is the least amount of work. > I like the idea of revalidation, but I'd suggest leaving that off of the > first pass. Yeah, agreed. > It might be easier on a first pass to look at supporting per-database > checksums (in this case, essentially treating shared catalogs as their own > database). All normal backends do per-database stuff (such as setting > current_database) during startup anyway. That doesn't really help for things > like recovery and replication though. :/ And there's still the question of > SLRUs (or are those not checksum'd today??). So you’re suggesting that the data_checksums GUC get set per-database context, so once it’s fully enabled in the specific database it treats it as in enforcing state, even if the rest of the cluster hasn’t completed? Hmm, might think on that a bit, but it seems pretty straightforward. What issues do you see affecting replication and recovery specifically (other than the entire cluster not being complete)? Since the checksum changes are WAL logged, seems you be no worse the wear on a standby if you had to change things. > BTW, it occurs to me that this is related to the problem we have with trying > to make changes that break page binary compatibility. If we had a method for > handling that it would probably be useful for enabling checksums as well. > You'd essentially treat an un-checksum'd page as if it was an "old page > version". The biggest problem there is dealing with the potential that the > new page needs to be larger than the old one was, but maybe there's some > useful progress to be had in this area before tackling the "page too small" > problem. I agree it’s very similar; my issue is I don’t want to have to postpone handling a specific case for some future infrastructure. That being said, what I could see being a general case is the piece which basically walks all pages in the cluster; as long as the page checksum/format validation happens at Page read time, we could do page version checks/conversions at the same time, and the only special code we’d need is to keep track of which files still need to be visited and how to minimize the impact on the cluster via some sort of throttling/leveling. (It’s also similar to vacuum in that way, however we have been going out of our way to make vacuum smart enough to *avoid* visiting every page, so I think it is a different enough use case that we can’t tie the two systems together, nor do I feel like taking that project on.) We could call the checksum_cycle something else (page_walk_cycle? bikeshed time!) and basically have the infrastructure to initiate online/gradual conversion/processing of all pages for free. Thoughts? David -- David Christensen End Point Corporation da...@endpoint.com 785-727-1171 -- 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] [PATCH] Add pg_disable_checksums() and supporting infrastructure
> On Feb 19, 2017, at 5:05 AM, Robert Haas <robertmh...@gmail.com> wrote: > > On Fri, Feb 17, 2017 at 2:28 AM, David Christensen <da...@endpoint.com> wrote: >> - Change "data_checksums" from a simple boolean to "data_checksum_state", an >> enum type for all of >> the potentially-required states for this feature (as well as enabling). > > Color me skeptical. I don't know what CHECKSUMS_ENABLING, > CHECKSUMS_ENFORCING, and CHECKSUMS_REVALIDATING are intended to > represent -- and there's no comments in the patch explaining it -- but > if we haven't yet written the code to enable checksums, how do we know > for sure which states it will require? > > If we're going to accept a patch to disable checksums without also > having the capability to enable checksums, I think we should leave out > the speculative elements about what might be needed on the "enable" > side and just implement the minimal "disable" side. > > However, FWIW, I don't accept that being able to disable checksums > online is a sufficient advance to justify enabling checksums by > default. Tomas had some good points on another thread about what > might be needed to really make that a good choice, and I'm still > skeptical about whether checksums catch any meaningful number of > errors that wouldn't be caught otherwise, and about the degree to > which any complaints it issues are actionable. I'm not really against > this patch on its own merits, but I think it's a small advance in an > area that needs a lot of work. I think it would be a lot more useful > if we had a way to *enable* checksums online. Then people who find > out that checksums exist and want them have an easier way of getting > them, and anyone who uses the functionality in this patch and then > regrets it has a way to get back. Hi Robert, this is part of a larger patch which *does* enable the checksums online; I’ve been extracting the necessary pieces out with the understanding that some people thought the disable code might be useful in its own merit. I can add documentation for the various states. The CHECKSUM_REVALIDATE is the only one I feel is a little wibbly-wobbly; the other states are required because of the fact that enabling checksums requires distinguishing between “in process of enabling” and “everything is enabled”. My design notes for the patch were submitted to the list with little comment; see: https://www.postgresql.org/message-id/1E6E64E9-634B-43F4-8AA2-CD85AD92D2F8%40endpoint.com I have since added the WAL logging of checksum states, however I’d be glad to take feedback on the other proposed approaches (particularly the system catalog changes + the concept of a checksum cycle).] Best, David -- David Christensen End Point Corporation da...@endpoint.com 785-727-1171 -- 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] [PATCH] Add pg_disable_checksums() and supporting infrastructure
On Feb 17, 2017, at 10:31 AM, Magnus Hagander <mag...@hagander.net> wrote: > > On Thu, Feb 16, 2017 at 9:58 PM, David Christensen <da...@endpoint.com> wrote: > Extracted from a larger patch, this patch provides the basic infrastructure > for turning data > checksums off in a cluster. This also sets up the necessary pg_control > fields to support the > necessary multiple states for handling the transitions. > > We do a few things: > > - Change "data_checksums" from a simple boolean to "data_checksum_state", an > enum type for all of > the potentially-required states for this feature (as well as enabling). > > - Add pg_control support for parsing/displaying the new setting. > > - Distinguish between the possible checksum states and be specific about > whether we care about > checksum read failures or write failures at all call sites, turning > DataChecksumsEnabled() into two > functions: DataChecksumsEnabledReads() and DataChecksumsEnabledWrites(). > > - Create a superuser function pg_disable_checksums() to perform the actual > disabling of this in the > cluster. > > I have *not* changed the default in initdb to enable checksums, but this > would be trivial. > > > Per the point made by somebody else (I think Simon?) on the other thread, I > think it also needs WAL support. Otherwise you turn it off on the master, but > it remains on on a replica which will cause failures once datablocks without > checksum starts replicating. Enclosed is a version which supports WAL logging and proper application of the checksum state change. I have verified it works with a replica as far as applying the updated data_checksum_state, though I had to manually call pg_switch_wal() on the master to get it to show up on the replica. (I don’t know if this is a flaw in my patch or just a natural consequence of testing on a low-volume local cluster.) -- David Christensen End Point Corporation da...@endpoint.com 785-727-1171 disable-checksums.patch 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] [PATCH] Add pg_disable_checksums() and supporting infrastructure
> On Feb 17, 2017, at 10:31 AM, Magnus Hagander <mag...@hagander.net> wrote: > > Per the point made by somebody else (I think Simon?) on the other thread, I > think it also needs WAL support. Otherwise you turn it off on the master, but > it remains on on a replica which will cause failures once datablocks without > checksum starts replicating. Good point; I’ll send a revision soon. -- David Christensen End Point Corporation da...@endpoint.com 785-727-1171 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] Add pg_disable_checksums() and supporting infrastructure
Extracted from a larger patch, this patch provides the basic infrastructure for turning data checksums off in a cluster. This also sets up the necessary pg_control fields to support the necessary multiple states for handling the transitions. We do a few things: - Change "data_checksums" from a simple boolean to "data_checksum_state", an enum type for all of the potentially-required states for this feature (as well as enabling). - Add pg_control support for parsing/displaying the new setting. - Distinguish between the possible checksum states and be specific about whether we care about checksum read failures or write failures at all call sites, turning DataChecksumsEnabled() into two functions: DataChecksumsEnabledReads() and DataChecksumsEnabledWrites(). - Create a superuser function pg_disable_checksums() to perform the actual disabling of this in the cluster. I have *not* changed the default in initdb to enable checksums, but this would be trivial. disable-checksums.patch Description: Binary data -- David Christensen End Point Corporation da...@endpoint.com 785-727-1171 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] Fix pg_proc comment grammar
Fixes some DESCR() grammar mistakes introduced by the xlog -> wal changes.
---
src/include/catalog/pg_proc.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 41c12af..bb7053a 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -3160,9 +3160,9 @@ DESCR("current wal insert location");
DATA(insert OID = 3330 ( pg_current_wal_flush_location PGNSP PGUID 12 1 0 0 0
f f f f t f v s 0 0 3220 "" _null_ _null_ _null_ _null_ _null_
pg_current_wal_flush_location _null_ _null_ _null_ ));
DESCR("current wal flush location");
DATA(insert OID = 2850 ( pg_walfile_name_offset PGNSP PGUID 12 1 0 0 0 f f f f
t f i s 1 0 2249 "3220" "{3220,25,23}" "{i,o,o}"
"{wal_location,file_name,file_offset}" _null_ _null_ pg_walfile_name_offset
_null_ _null_ _null_ ));
-DESCR("wal filename and byte offset, given an wal location");
+DESCR("wal filename and byte offset, given a wal location");
DATA(insert OID = 2851 ( pg_walfile_name PGNSP PGUID 12
1 0 0 0 f f f f t f i s 1 0 25 "3220" _null_ _null_ _null_ _null_ _null_
pg_walfile_name _null_ _null_ _null_ ));
-DESCR("wal filename, given an wal location");
+DESCR("wal filename, given a wal location");
DATA(insert OID = 3165 ( pg_wal_location_diff PGNSP PGUID 12 1 0 0 0
f f f f t f i s 2 0 1700 "3220 3220" _null_ _null_ _null_ _null_ _null_
pg_wal_location_diff _null_ _null_ _null_ ));
DESCR("difference in bytes, given two wal locations");
--
2.8.4 (Apple Git-73)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] Teach Catalog.pm how many attributes there should be per DATA() line
Throws a build error if we encounter a different number of fields in a
DATA() line than we expect for the catalog in question.
Previously, it was possible to silently ignore any mismatches at build
time which could result in symbol undefined errors at link time. Now
we stop and identify the infringing line as soon as we encounter it,
which greatly speeds up the debugging process.
---
src/backend/catalog/Catalog.pm | 26 +-
src/backend/utils/Gen_fmgrtab.pl | 18 --
2 files changed, 41 insertions(+), 3 deletions(-)
diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index e1f3c3a..86f5b59 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -46,6 +46,9 @@ sub Catalogs
open(INPUT_FILE, '<', $input_file) || die "$input_file: $!";
+ my ($filename) = ($input_file =~ m/(\w+)\.h$/);
+ my $natts_pat = "Natts_$filename";
+
# Scan the input file.
while ()
{
@@ -70,8 +73,15 @@ sub Catalogs
s/\s+/ /g;
# Push the data into the appropriate data structure.
- if
(/^DATA\(insert(\s+OID\s+=\s+(\d+))?\s+\(\s*(.*)\s*\)\s*\)$/)
+ if (/$natts_pat\s+(\d+)/)
+ {
+ $catalog{natts} = $1;
+ }
+ elsif
(/^DATA\(insert(\s+OID\s+=\s+(\d+))?\s+\(\s*(.*)\s*\)\s*\)$/)
{
+ check_natts($filename, $catalog{natts},$3) or
+ die sprintf "Wrong number of Natts in DATA()
line %s:%d\n", $input_file, INPUT_FILE->input_line_number;
+
push @{ $catalog{data} }, { oid => $2,
bki_values => $3 };
}
elsif (/^DESCR\(\"(.*)\"\)$/)
@@ -216,4 +226,18 @@ sub RenameTempFile
rename($temp_name, $final_name) || die "rename: $temp_name: $!";
}
+# verify the number of fields in the passed-in bki structure
+sub check_natts
+{
+ my ($catname, $natts, $bki_val) = @_;
+ unless ($natts)
+ {
+ die "Could not find definition for Natts_${catname} before
start of DATA()\n";
+ }
+
+ # we're working with a copy and need to count the fields only, so
collapse
+ $bki_val =~ s/"[^"]*?"/xxx/g;
+
+ return (split /\s+/ => $bki_val) == $natts;
+}
1;
diff --git a/src/backend/utils/Gen_fmgrtab.pl b/src/backend/utils/Gen_fmgrtab.pl
index cdd603a..49a5d80 100644
--- a/src/backend/utils/Gen_fmgrtab.pl
+++ b/src/backend/utils/Gen_fmgrtab.pl
@@ -56,9 +56,11 @@ foreach my $column (@{ $catalogs->{pg_proc}->{columns} })
}
my $data = $catalogs->{pg_proc}->{data};
+my $natts = $catalogs->{pg_proc}->{natts};
+my $elem = 0;
+
foreach my $row (@$data)
{
-
# To construct fmgroids.h and fmgrtab.c, we need to inspect some
# of the individual data fields. Just splitting on whitespace
# won't work, because some quoted fields might contain internal
@@ -67,7 +69,19 @@ foreach my $row (@$data)
# fields that might need quoting, so this simple hack is
# sufficient.
$row->{bki_values} =~ s/"[^"]*"/"xxx"/g;
- @{$row}{@attnames} = split /\s+/, $row->{bki_values};
+my @bki_values = split /\s+/, $row->{bki_values};
+
+# verify we've got the expected number of data fields
+if (@bki_values != $natts)
+{
+die sprintf <
Re: [HACKERS] Online enabling of page level checksums
> On Jan 23, 2017, at 10:59 AM, David Christensen <da...@endpoint.com> wrote: > >> >> On Jan 23, 2017, at 10:53 AM, Simon Riggs <si...@2ndquadrant.com> wrote: >> >> On 23 January 2017 at 16:32, David Christensen <da...@endpoint.com> wrote: >> >>> ** Handling checksums on a standby: >>> >>> How to handle checksums on a standby is a bit trickier since checksums are >>> inherently a local cluster state and not WAL logged but we are storing >>> state in the system tables for each database we need to make sure that the >>> replicas reflect truthful state for the checksums for the cluster. >> >> Not WAL logged? If the objective of this feature is robustness, it >> will need to be WAL logged. >> >> Relation options aren't accessed by the startup process during >> recovery, so that part won't work in recovery. Perhaps disable >> checksumming until the everything is enabled rather than relation by >> relation. >> >> If y'all serious about this then you're pretty late in the cycle for >> such a huge/critical patch. So lets think of ways of reducing the >> complexity... >> >> Seems most sensible to add the "enable checksums for table" function >> into VACUUM because then it will happen automatically via autovacuum, >> or you could force the issue using something like >> vacuumdb --jobs 4 --enable-checksums >> That gives you vacuum_delay and a background worker for no effort > > I’m not sure that this will work as-is, unless we’re forcing VACUUM to ignore > the visibility map. I had originally considered having this sit on top of > VACUUm though, we just need a way to iterate over all relations and read > every page. Another issue with this (that I think would still exist with the bgworker approach) is how to handle databases with datallowconn = 0. `vacuumdb`, at least, explicitly filters out these rows when iterating over databases to connect to, so while we could enable them for all databases, we can’t enable for the cluster without verifying that these disallowed dbs are already checksummed. -- David Christensen End Point Corporation da...@endpoint.com 785-727-1171 -- 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] Online enabling of page level checksums
> On Jan 23, 2017, at 10:53 AM, Simon Riggs <si...@2ndquadrant.com> wrote: > > On 23 January 2017 at 16:32, David Christensen <da...@endpoint.com> wrote: > >> ** Handling checksums on a standby: >> >> How to handle checksums on a standby is a bit trickier since checksums are >> inherently a local cluster state and not WAL logged but we are storing state >> in the system tables for each database we need to make sure that the >> replicas reflect truthful state for the checksums for the cluster. > > Not WAL logged? If the objective of this feature is robustness, it > will need to be WAL logged. > > Relation options aren't accessed by the startup process during > recovery, so that part won't work in recovery. Perhaps disable > checksumming until the everything is enabled rather than relation by > relation. > > If y'all serious about this then you're pretty late in the cycle for > such a huge/critical patch. So lets think of ways of reducing the > complexity... > > Seems most sensible to add the "enable checksums for table" function > into VACUUM because then it will happen automatically via autovacuum, > or you could force the issue using something like > vacuumdb --jobs 4 --enable-checksums > That gives you vacuum_delay and a background worker for no effort I’m not sure that this will work as-is, unless we’re forcing VACUUM to ignore the visibility map. I had originally considered having this sit on top of VACUUm though, we just need a way to iterate over all relations and read every page. -- David Christensen End Point Corporation da...@endpoint.com 785-727-1171 -- 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] Online enabling of page level checksums
to handle checksums on a standby is a bit trickier since checksums are inherently a local cluster state and not WAL logged but we are storing state in the system tables for each database we need to make sure that the replicas reflect truthful state for the checksums for the cluster. In order to manage this discrepency, we WAL log a few additional pieces of information; specifically: - new events to capture/propogate any of the pg_control fields, such as: checksum version data, checksum cycle increases, enabling/disabling actions - checksum background worker block ranges. (XXX: we could decide that we're okay with relations being all or nothing here, rendering this point moot.) Some notes on the block ranges: This would effectively be a series of records containing (datid, relid, start block, end block) for explicit checksum ranges, generated by the checksum bgworker as it checksums individual relations. This could be broken up into a series of blocks so rather than having the granularity be by relation we could have these records get generated periodicaly (say in groups of 10K blocks or whatever, number to be determined) to allow standby checksum recalculation to be incremental so as not to delay replay unnecessarily as checksums are being created. Since the block range WAL records will be replayed before any of the pg_class/pg_database catalog records are replay, we'll be guaranteed to have the checksums calculated on the standby by the time it appears valid due to system state. We may also be able to use the WAL records to speed up the processing of existing heap files if they are interrupted for some reason, this remains to be seen. ** Testing changes: We need to add separate initdb checksum regression test which are outside of the normal pg_regress framework. -- David Christensen End Point Corporation da...@endpoint.com 785-727-1171 -- 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] [PATCH] Add EXPLAIN (ALL) shorthand
> On May 19, 2016, at 5:24 PM, Евгений Шишкин <itparan...@gmail.com> wrote: > > >> On 20 May 2016, at 01:12, Tom Lane <t...@sss.pgh.pa.us> wrote: >> >> >> I'm a bit inclined to think that what this is really about is that we >> made the wrong call on the BUFFERS option, and that it should default >> to ON just like COSTS and TIMING do. Yeah, that would be an incompatible >> change, but that's what major releases are for no? > > After thinking about it, i think this is a better idea. Yeah, if that’s the only practical difference, WORKSFORME; I can see the point about boxing us into a corner at some future time. +1. -- David Christensen End Point Corporation da...@endpoint.com 785-727-1171 -- 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] [PATCH] Add EXPLAIN (ALL) shorthand
> On May 19, 2016, at 3:17 PM, Евгений Шишкин <itparan...@gmail.com> wrote: > > >> On 19 May 2016, at 22:59, Tom Lane <t...@sss.pgh.pa.us> wrote: >> >> David Christensen <da...@endpoint.com> writes: >>> This simple patch adds “ALL” as an EXPLAIN option as shorthand for “EXPLAIN >>> (ANALYZE, VERBOSE, COSTS, TIMING, BUFFERS)” for usability. >> >> I'm not sure this is well thought out. It would mean for example that >> we could never implement EXPLAIN options that are mutually exclusive >> ... at least, not without having to redefine ALL as all-except-something. >> Non-boolean options would be problematic as well. >> > > Maybe EVERYTHING would be ok. > But it is kinda long word to type. If it’s just a terminology issue, what about EXPLAIN (*); already a precedent with SELECT * to mean “everything”. (MAX? LIKE_I’M_5?) Let the bikeshedding begin! In any case, I think a shorthand for “give me the most possible detail without me having to lookup/type/remember the options” is a good tool. David -- David Christensen End Point Corporation da...@endpoint.com 785-727-1171 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] Add EXPLAIN (ALL) shorthand
This simple patch adds “ALL” as an EXPLAIN option as shorthand for “EXPLAIN (ANALYZE, VERBOSE, COSTS, TIMING, BUFFERS)” for usability. 0001-Add-EXPLAIN-ALL-shorthand.patch Description: Binary data -- David Christensen End Point Corporation da...@endpoint.com 785-727-1171 -- 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] [PATCH] Teach Catalog.pm how many attributes there should be per DATA() line
> On Oct 9, 2015, at 2:17 PM, Robert Haas <robertmh...@gmail.com> wrote: > > On Thu, Oct 8, 2015 at 12:43 PM, David Christensen <da...@endpoint.com> wrote: >> I’m happy to move it around, but If everything is in order, how will this >> affect things at all? If we’re in a good state this condition should never >> trigger. > > Right, but I think it ought to be Catalog.pm's job to parse the config > file. The job of complaining about what it contains is a job worth > doing, but it's not the same job. Personally, I hate it when parsers > take it upon themselves to do semantic analysis, because then what > happens if you want to write, say, a tool to repair a broken file? > You need to be able to read it in without erroring out so that you can > frob whatever's busted and write it back out, and the parser is > getting in your way. Maybe that's not going to come up here, but I'm > just explaining my general philosophy on these things… Not disagreeing with you in general, but this is a very specific use case and I think we lose the niceness of being able to tie back to the specific line number for the file in question—the alternative being to track that information as well in a separate structure which we then pass around, which seems like overkill. The only two consumers of the catalog-specific data lines (at least via direct access in Perl) are genbki.pl and Gen_fmgtab.pl. We would need to add these checks anyway in both call sites, so to me it seems important to bail early if we see any issues, so I think putting the failure as soon as we notice it with as much context to fix it (i.e., as written) is the right choice. We can certainly pretty up the messages. The consistency of the system catalogs in the development state is something that is fundamental to whether there is any information that is sensible to query, and by definition if we are missing columns in the data rows this is a mistake and whatever parsed data in here will be worse than useless (as who knows the order of the missing column, data can/will end up being misaligned). Thus I don’t believe that we’d want other (hypothetical) Catalog.pm consumers to try to use data that we know is bad. -- David Christensen PostgreSQL Team Manager End Point Corporation da...@endpoint.com 785-727-1171 -- 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] [PATCH] Teach Catalog.pm how many attributes there should be per DATA() line
> On Oct 8, 2015, at 11:23 AM, Robert Haas <robertmh...@gmail.com> wrote: > > On Tue, Oct 6, 2015 at 9:15 AM, David Christensen <da...@endpoint.com> wrote: >> Fixes a build issue I ran into while adding some columns to system tables: >> >>Throws a build error if we encounter a different number of fields in a >>DATA() line than we expect for the catalog in question. >> >>Previously, it was possible to silently ignore any mismatches at build >>time which could result in symbol undefined errors at link time. Now >>we stop and identify the infringing line as soon as we encounter it, >>which greatly speeds up the debugging process. > > I think this is a GREAT idea, but this line made me laugh[1]: > > +warn "No Natts defined yet, silently skipping check...\n"; > > I suggest that we make that a fatal error. Like "Could not find > definition Natts_pg_proc before start of DATA”. That’s fine with me; my main consideration was to make sure nothing broke in the status quo, including dependencies I was unaware of. > Secondly, I don't think we should check this at this point in the > code, because then it blindly affects everybody who uses Catalog.pm. > Let's pick one specific place to do this check. I suggest genbki.pl, > inside the loop with this comment: "# Ordinary catalog with DATA > line(s)" I’m happy to move it around, but If everything is in order, how will this affect things at all? If we’re in a good state this condition should never trigger. -- David Christensen PostgreSQL Team Manager End Point Corporation da...@endpoint.com 785-727-1171 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] Teach Catalog.pm how many attributes there should be per DATA() line
Fixes a build issue I ran into while adding some columns to system tables: Throws a build error if we encounter a different number of fields in a DATA() line than we expect for the catalog in question. Previously, it was possible to silently ignore any mismatches at build time which could result in symbol undefined errors at link time. Now we stop and identify the infringing line as soon as we encounter it, which greatly speeds up the debugging process. 0001-Teach-Catalog.pm-how-many-attributes-there-should-be.patch Description: Binary data -- David Christensen PostgreSQL Team Manager End Point Corporation da...@endpoint.com 785-727-1171 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] Comment fixes
Use the correct name “pgindent” in comments. 0001-Make-pgindent-references-consistent.patch Description: Binary data -- David Christensen PostgreSQL Team Manager End Point Corporation da...@endpoint.com 785-727-1171 -- 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] [DESIGN] Incremental checksums
- pg_disable_checksums(void) = turn checksums off for a cluster. Sets the state to disabled, which means bg_worker will not do anything. - pg_request_checksum_cycle(void) = if checksums are enabled, increment the data_checksum_cycle counter and set the state to enabling. If the cluster is already enabled for checksums, then what is the need for any other action? You are assuming this is a one-way action. No, I was confused by the state (enabling) this function will set. Okay. Requesting an explicit checksum cycle would be desirable in the case where you want to proactively verify there is no cluster corruption to be found. Sure, but I think that is different from setting the state to enabling. In your proposal above, in enabling state cluster needs to write checksums, where for such a feature you only need read validation. With “revalidating” since your database is still actively making changes, you need to validate writes too (think new tables, etc). “Enabling” needs reads unvalidated because you’re starting from an unknown state (i.e., not checksummed already). Thanks, David -- David Christensen PostgreSQL Team Manager End Point Corporation da...@endpoint.com 785-727-1171 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] Comment fix for miscinit.c
Quick comment fix for edit issue. 0001-Fix-comment.patch Description: Binary data -- David Christensen PostgreSQL Team Manager End Point Corporation da...@endpoint.com 785-727-1171 -- 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] [DESIGN] Incremental checksums
On Jul 15, 2015, at 3:18 AM, Amit Kapila amit.kapil...@gmail.com wrote: - pg_disable_checksums(void) = turn checksums off for a cluster. Sets the state to disabled, which means bg_worker will not do anything. - pg_request_checksum_cycle(void) = if checksums are enabled, increment the data_checksum_cycle counter and set the state to enabling. If the cluster is already enabled for checksums, then what is the need for any other action? You are assuming this is a one-way action. Some people may decide that checksums end up taking too much overhead or similar, we should support disabling of this feature; with this proposed patch the disable action is fairly trivial to handle. Requesting an explicit checksum cycle would be desirable in the case where you want to proactively verify there is no cluster corruption to be found. David -- David Christensen PostgreSQL Team Manager End Point Corporation da...@endpoint.com 785-727-1171 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [DESIGN] Incremental checksums
WAL log a few additional pieces of information; specifically: - new events to capture/propogate any of the pg_control fields, such as: checksum version data, checksum cycle increases, enabling/disabling actions - checksum background worker block ranges. Some notes on the block ranges: This would effectively be a series of records containing (datid, relid, start block, end block) for explicit checksum ranges, generated by the checksum bgworker as it checksums individual relations. This could be broken up into a series of blocks so rather than having the granularity be by relation we could have these records get generated periodicaly (say in groups of 10K blocks or whatever, number to be determined) to allow standby checksum recalculation to be incremental so as not to delay replay unnecessarily as checksums are being created. Since the block range WAL records will be replayed before any of the pg_class/pg_database catalog records are replay, we'll be guaranteed to have the checksums calculated on the standby by the time it appears valid due to system state. We may also be able to use the WAL records to speed up the processing of existing heap files if they are interrupted for some reason, this remains to be seen. ** Testing changes: We need to add separate initdb checksum regression test which are outside of the normal pg_regress framework. ** Roadmap: - Milestone 1 (master support) [0/7] - [ ] pg_control updates for new data_checksum_cycle, data_checksum_state - [ ] pg_class changes - [ ] pg_database changes - [ ] function API - [ ] autovac launcher modifications - [ ] checksum bgworker - [ ] doc updates - Milestone 2 (pg_upgrade support) [0/4] - [ ] no checksum - no checksum - [ ] checksum - no checksum - [ ] no checksum - checksum - [ ] checksum - checksum - Milestone 3 (standby support) [0/4] - [ ] WAL log checksum cycles - [ ] WAL log enabling/disabling checksums - [ ] WAL log checksum block ranges - [ ] Add standby WAL replay I look forward to any feedback; thanks! David -- David Christensen PostgreSQL Team Manager End Point Corporation da...@endpoint.com 785-727-1171 -- 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] [DESIGN] Incremental checksums
On Jul 13, 2015, at 3:50 PM, Jim Nasby jim.na...@bluetreble.com wrote: On 7/13/15 3:26 PM, David Christensen wrote: * Incremental Checksums PostgreSQL users should have a way up upgrading their cluster to use data checksums without having to do a costly pg_dump/pg_restore; in particular, checksums should be able to be enabled/disabled at will, with the database enforcing the logic of whether the pages considered for a given database are valid. Considered approaches for this are having additional flags to pg_upgrade to set up the new cluster to use checksums where they did not before (or optionally turning these off). This approach is a nice tool to have, but in order to be able to support this process in a manner which has the database online while the database is going throught the initial checksum process. It would be really nice if this could be extended to handle different page formats as well, something that keeps rearing it's head. Perhaps that could be done with the cycle idea you've described. I had had this thought too, but the main issues I saw were that new page formats were not guaranteed to take up the same space/storage, so there was an inherent limitation on the ability to restructure things out *arbitrarily*; that being said, there may be a use-case for the types of modifications that this approach *would* be able to handle. Another possibility is some kind of a page-level indicator of what binary format is in use on a given page. For checksums maybe a single bit would suffice (indicating that you should verify the page checksum). Another use case is using this to finally ditch all the old VACUUM FULL code in HeapTupleSatisfies*(). There’s already a page version field, no? I assume that would be sufficient for the page format indicator. I don’t know about using a flag for verifying the checksum, as that is already modifying the page which is to be checksummed anyway, which we want to avoid having to rewrite a bunch of pages unnecessarily, no? And you’d presumably need to clear that state again which would be an additional write. This was the issue that the checksum cycle was meant to handle, since we store this information in the system catalogs and the types of modifications here would be idempotent. David -- David Christensen PostgreSQL Team Manager End Point Corporation da...@endpoint.com 785-727-1171 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] correct the initdb.log path for pg_regress
When encountering an initdb failure in pg_regress, we were displaying the incorrect path to the log file; this commit fixes all 3 places this could occur. 0001-Output-the-correct-path-for-initdb.log-in-pg_regress.patch Description: Binary data -- David Christensen PostgreSQL Team Manager End Point Corporation da...@endpoint.com 785-727-1171 -- 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] [PATCH] Add missing \ddp psql command
On Jul 7, 2015, at 3:53 AM, Fujii Masao masao.fu...@gmail.com wrote: On Tue, Jul 7, 2015 at 2:11 AM, David Christensen da...@endpoint.com wrote: Quickie patch for spotted missing psql \ddp tab-completion. Thanks for the report and patch! I found that tab-completion was not supported in not only \ddp but also other psql meta commands like \dE, \dm, \dO, \dy, \s and \setenv. Attached patch adds all those missing tab-completions. From a completeness standpoint makes sense to me to have all of them, but from a practical standpoint a single-character completion like \s isn’t going to do much. :) David -- David Christensen PostgreSQL Team Manager End Point Corporation da...@endpoint.com 785-727-1171 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] Add missing \ddp psql command
Quickie patch for spotted missing psql \ddp tab-completion. 0001-Add-missing-ddp-psql-tab-completion.patch Description: Binary data -- David Christensen PostgreSQL Team Manager End Point Corporation da...@endpoint.com 785-727-1171 -- 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] Add checksums without --initdb
On Jul 2, 2015, at 3:43 PM, Heikki Linnakangas hlinn...@iki.fi wrote: On 07/02/2015 11:28 PM, Andres Freund wrote: On 2015-07-02 22:53:40 +0300, Heikki Linnakangas wrote: Add a enabling-checksums mode to the server where it calculates checksums for anything it writes, but doesn't check or complain about incorrect checksums on reads. Put the server into that mode, and then have a background process that reads through all data in the cluster, calculates the checksum for every page, and writes all the data back. Once that's completed, checksums can be fully enabled. You'd need, afaics, a bgworker that connects to every database to read pg_class, to figure out what type of page a relfilenode has. And this probably should call back into the relevant AM or such. Nah, we already assume that every relation data file follows the standard page format, at least enough to have the checksum field at the right location. See FlushBuffer() - it just unconditionally calculates the checksum before writing out the page. (I'm not totally happy about that, but that ship has sailed) - Heikki So thinking some more about the necessary design to support enabling checksums post-initdb, what about the following?: Introduce a new field in pg_control, data_checksum_state - (0 - disabled, 1 - enabling in process, 2 - enabled). This could be set via (say) a pg_upgrade flag when creating a new cluster with --enable-checksums or a standalone program to adjust that field in pg_control. Checksum enforcing behavior will be dependent on that setting; 0 is non-enforcing read or write, 1 is enforcing checksums on buffer write but ignoring on read, and 2 is the normal enforcing read/write mode. Disabling checksums could be done with this tool as well, and would trivially just cause it to ignore the checksums (or alternately set to 0 on page write, depending on if we think it matters). Add new catalog fields pg_database.dathaschecksum, pg_class.relhaschecksum; initially set to 0, or 1 if checksums were enabled at initdb time. Augment autovacuum to check if we are currently enabling checksums based on the value in pg_control; if so, loop over any database with !pg_database.dathaschecksum. For any relation in said database, check for relations with !pg_class.relhaschecksum; if found, read/dirty/write (however) each block to force the checksum written out for each page. As each relation is completely verified checksummed, update relhaschecksum = t. When no relations remain, set pg_database.dathaschecksum = t. (There may need to be some additional considerations for storing the checksum state of global relations or any other thing that uses the standard page format that live outside a specific database; i.e., all shared catalogs, quite possibly some things I haven't considered yet.) If the data_checksum_state is enabling and there are no database needing to be enabled, then we can set data_checksum_state to enabled; everything then works as expected for the normal enforcing state. External programs needing to be adjusted: - pg_reset_xlog -- add the persistence of the data_checksum_state - pg_controldata -- add the display of the data_checksum_state - pg_upgrade -- add an --enable-checksums flag to transition a new cluster with the data pages. May need some adjustments for the data_checksum_state field Possible new tool: - pg_enablechecksums -- basic tool to set the data_checksum_state flag of pg_control Other thoughts Do we need periodic CRC scanning background worker just to check buffers periodically? - if so, does this cause any interference with frozen relations? What additional changes would be required or what wrinkles would we have to work out? David -- David Christensen PostgreSQL Team Manager End Point Corporation da...@endpoint.com 785-727-1171 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Add checksums without --initdb
So on #postgresql, I was musing about methods of getting checksums enabled/disabled without requiring a separate initdb step and minimizing the downtime required to get such functionality enabled. What about adapting pg_basebackup to add the following options: -k|--checksums - build the replica with checksums enabled. -K|—no-checksums - build the replica with checksums disabled. The way this would work would be to have pg_basebackup's ReceiveAndUnpackTarFile() calculate and/or remove the checksums from each heap page as it is streamed and update the pg_control file to reflect the new checksums setting. After this checksum-enabled replica is created, then it could stream/process WAL and get caught up, then the user fails over to their brand-spanking-new checksum-enabled database. Obviously this would be a bit slower to calculate each page’s checksum than it would be just to write the data out from the tar stream, but it seems to me like this is a single point where the whole database would need to be processed page-by-page as it is. Possible concerns here are whether checksums are included in WAL full_page_writes or if they are independently calculated; if the latter I think we’d be fine. If checksums are all handled at the layer below WAL than any streamed/processed changes should be fine to get us to the point where we could come up as a master. We’d also need to be careful to add checksums to only heap files, but that would be able to be handled via the filename prefixes (base|global) (I’m not sure if the relation forks are in standard Page format, but if not we could exclude those as well). Obviously this bakes quite a bit of cluster structural awareness into pg_basebackup and may tie it more strongly to a specific major version, but it seems to me like the tradeoffs would be worth it if you wanted to have that option and the code paths could exist to keep the existing behavior if so. Andres suggested a separate tool that would basically rewrite the existing data directory heap files in place, which I can also see a use case for, but I also think there’s some benefit to be found in having it happen while the replica is being streamed/built. Ideas/thoughts/reasons this wouldn’t work? David -- David Christensen PostgreSQL Team Manager End Point Corporation da...@endpoint.com 785-727-1171 -- 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] [PATCH] two-arg current_setting() with fallback
On Mar 19, 2015, at 6:27 PM, Tom Lane t...@sss.pgh.pa.us wrote:
David Christensen da...@endpoint.com writes:
The two-arg form of the current_setting() function will allow a
fallback value to be returned instead of throwing an error when an
unknown GUC is provided. This would come in most useful when using
custom GUCs; e.g.:
-- errors out if the 'foo.bar' setting is unset
SELECT current_setting('foo.bar');
-- returns current setting of foo.bar, or 'default' if not set
SELECT current_setting('foo.bar', 'default')
This would save you having to wrap the use of the function in an
exception block just to catch and utilize a default setting value
within a function.
That seems kind of ugly, not least because it assumes that you know
a value that couldn't be mistaken for a valid value of the GUC.
(I realize that you are thinking of cases where you want to pretend
that the GUC has some valid value, but that's not the only use case.)
ISTM, since we don't allow GUCs to have null values, it'd be better to
define the variant function as returning NULL for no-such-GUC. Then the
behavior you want could be achieved by wrapping that in a COALESCE, but
the behavior of probing whether the GUC is set at all would be achieved
with an IS NULL test.
regards, tom lane
In that case, the other thought I had here is that we change the function
signature of current_setting() to be a two-arg form where the second argument
is a boolean throw_error, with a default argument of true to preserve
existing semantics, and returning NULL if that argument is false. However, I'm
not sure if there are some issues with changing the signature of an existing
function (e.g., with pg_upgrade, etc.).
My *impression* is that since pg_upgrade rebuilds the system tables for a new
install it shouldn't be an issue, but not sure if having the same pg_proc OID
with different values or an alternate pg_proc OID would cause issues down the
line; anyone know if this is a dead-end?
Regards,
David
--
David Christensen
End Point Corporation
da...@endpoint.com
785-727-1171
--
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] [PATCH] two-arg current_setting() with fallback
On Mar 20, 2015, at 11:10 AM, David G. Johnston david.g.johns...@gmail.com wrote: On Fri, Mar 20, 2015 at 9:04 AM, Robert Haas robertmh...@gmail.com wrote: On Fri, Mar 20, 2015 at 10:54 AM, David Christensen da...@endpoint.com wrote: In that case, the other thought I had here is that we change the function signature of current_setting() to be a two-arg form where the second argument is a boolean throw_error, with a default argument of true to preserve existing semantics, and returning NULL if that argument is false. However, I'm not sure if there are some issues with changing the signature of an existing function (e.g., with pg_upgrade, etc.). My *impression* is that since pg_upgrade rebuilds the system tables for a new install it shouldn't be an issue, but not sure if having the same pg_proc OID with different values or an alternate pg_proc OID would cause issues down the line; anyone know if this is a dead-end? I think if the second argument is defaulted it would be OK. However it might make sense to instead add a new two-argument function and leave the existing one-argument function alone, because setting default arguments for functions defined in pg_proc.h is kind of a chore. Isn't there some other update along this whole error-vs-null choice going around where a separate name was chosen for the new null-returning function instead of adding a boolean switch argument? Well, speaking of the two-arg form vs alternate name, here's a version of the patch which includes the new behavior. (I couldn't think of a good name to expose for an alternate function, but I'm open to suggestions.) Regards, David -- David Christensen End Point Corporation da...@endpoint.com 785-727-1171 0001-Add-two-arg-form-of-current_setting-to-optionally-su.patch 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
[HACKERS] [PATCH] two-arg current_setting() with fallback
Apologies if this is a double-post.
Enclosed is a patch that creates a two-arg current_setting() function. From
the commit message:
The two-arg form of the current_setting() function will allow a
fallback value to be returned instead of throwing an error when an
unknown GUC is provided. This would come in most useful when using
custom GUCs; e.g.:
-- errors out if the 'foo.bar' setting is unset
SELECT current_setting('foo.bar');
-- returns current setting of foo.bar, or 'default' if not set
SELECT current_setting('foo.bar', 'default')
This would save you having to wrap the use of the function in an
exception block just to catch and utilize a default setting value
within a function.
0001-Add-two-arg-for-of-current_setting-NAME-FALLBACK.patch
Description: Binary data
--
David Christensen
End Point Corporation
da...@endpoint.com
785-727-1171
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Simple documentation typo patch
Hey folks, this corrects typos going back to 75c6519ff68dbb97f73b13e9976fb8075bbde7b8 where EUC_JIS_2004 and SHIFT_JIS_2004 were first added. These typos are present in all supported major versions of PostgreSQL, back through 8.3; I didn't look any further than that. :-) 0001-Doc-patch-for-typo-in-built-in-conversion-type-names.patch Description: Binary data Regards, David -- David Christensen End Point Corporation da...@endpoint.com 785-727-1171 -- 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] Deriving release notes from git commit messages
On Jun 24, 2011, at 2:28 PM, Christopher Browne wrote:
On Fri, Jun 24, 2011 at 6:47 PM, Greg Smith g...@2ndquadrant.com wrote:
On 06/24/2011 01:42 PM, Robert Haas wrote:
I am not inclined to try to track sponsors in the commit message at
all.
I was not suggesting that information be part of the commit. We've worked
out a reasonable initial process for the people working on sponsored
features to record that information completely outside of the commit or
release notes data. It turns out though that process would be easier to
drive if it were easier to derive a feature-{commit,author} list
though--and that would spit out for free with the rest of this. Improving
the ability to do sponsor tracking is more of a helpful side-effect of
something that's useful for other reasons rather than a direct goal.
In taking a peek at the documentation and comments out on the interweb
about git amend, I don't think that it's going to be particularly
successful if we try to capture all this stuff in the commit message
and metadata, because it's tough to guarantee that *all* this data
would be correctly captured at commit time, and you can't revise it
after the commit gets pushed upstream.
Perhaps `git notes` could be something used to annotate these:
http://www.kernel.org/pub/software/scm/git/docs/git-notes.html
Regards,
David
--
David Christensen
End Point Corporation
da...@endpoint.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] WIP pgindent replacement
# Avoid bug that converts 'x =- 1' to 'x = -1' $source =~ s!=- !-= !g; I haven't looked at the shell script this replaces, but is that the correct substitution pattern? (BTW, I'm not seeing the token =- anywhere except in the Makefile, which wouldn't be run against, no? Am I missing something?) Regards, David -- David Christensen End Point Corporation da...@endpoint.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] LOCK DATABASE
On May 18, 2011, at 6:11 PM, Alvaro Herrera wrote: Excerpts from Christopher Browne's message of mié may 18 18:33:14 -0400 2011: On Wed, May 18, 2011 at 1:02 AM, Jaime Casanova ja...@2ndquadrant.com wrote: So we the lock will be released at end of the session or when the UNLOCK DATABASE command is invoked, right? A question: why will we beign so rude by killing other sessions instead of avoid new connections and wait until the current sessions disconnect? There were multiple alternatives suggested, which is probably useful to outline. 1. I suggested that this looks a lot like the controls of pg_hba.conf When our DBAs are doing major management of replication, they are known to reconfigure pg_hba.conf to lock out all users save for the one used by Slony. Yeah, I mentioned this but I think it actually sucks. How would this differ from just UPDATE pg_database SET datallowconn = FALSE for the databases in question? Regards, David -- David Christensen End Point Corporation da...@endpoint.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] branching for 9.2devel
On Apr 25, 2011, at 3:28 PM, Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: On 04/25/2011 03:30 PM, Tom Lane wrote: *Ouch*. Really? It's hard to believe that anyone would consider it remotely usable for more than toy-sized projects, if you have to list all the typedef names on the command line. Looks like BSD does the same. It's just that we hide it in pgindent: Oh wow, I never noticed that. That's going to be a severe problem for the run it anywhere goal. The typedefs list is already close to 32K, and is not going anywhere but up. There are already platforms on which a shell command line that long will fail, and I think once we break past 32K we might find it failing on even pretty popular ones. I take it the behavior of the `indent` program is sufficiently complex that it couldn't be modeled sufficiently easily by a smart enough perl script? Regards, David -- David Christensen End Point Corporation da...@endpoint.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] stored procedures
On Apr 22, 2011, at 3:50 PM, Tom Lane wrote: Merlin Moncure mmonc...@gmail.com writes: On Fri, Apr 22, 2011 at 1:28 PM, Peter Eisentraut pete...@gmx.net wrote: It would probably be more reasonable and feasible to have a setup where you can end a transaction in plpgsql but a new one would start right away. ya, that's an idea. Yeah, that's a good thought. Then we'd have a very well-defined collection of state that had to be preserved through such an operation, ie, the variable values and control state of the SP. It also gets rid of the feeling that you ought not be in a transaction when you enter the SP. There's still the problem of whether you can invoke operations such as VACUUM from such an SP. I think we'd want to insist that they terminate the current xact, which is perhaps not too cool. Dumb question, but wouldn't this kind of approach open up a window where (say) datatypes, operators, catalogs, etc, could disappear/change out from under you, being that you're now in a different transaction/snapshot; presuming there is a concurrent transaction from a different backend modifying the objects in question? In the non-explicit transaction case, locking wouldn't work to keep these objects around due to the transaction scope of locks (unless locks are part of the transaction state carried forward across the implicit transactions). If so, could that be done in such a way that it would take precedence over a parallel backend attempting to acquire the same locks without blocking the procedure? Regards, David -- David Christensen End Point Corporation da...@endpoint.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] multiple -f support
On Mar 11, 2011, at 6:17 AM, Bruce Momjian wrote: Robert Haas wrote: On Sun, Feb 6, 2011 at 11:16 AM, Bruce Momjian br...@momjian.us wrote: I assume having psql support multiple -f files is not a high priority or something we don't want. IIRC, nobody objected to the basic concept, and it seems useful. I thought we were pretty close to committing something along those lines at one point, actually. I don't remember exactly where the wheels came off. Maybe a TODO? Added to the psql section: |Allow processing of multiple -f (file) options The original patch was a fairly trivial WIP one, which I started working on to add support for multiple -c flags interspersed as well. I haven't looked at it in quite some time, though; there had been some concerns about how it worked in single-transaction mode and some other issues I don't recall off the top of my head. On this topic, I was thinking that it may be useful to provide an alternate multi-file syntax, a la git, with any argument following '--' in the argument list being interpreted as a file to process; i.e.,: $ psql -U user [option] database -- file1.sql file2.sql file3.sql This would allow things like shell expansion to work as expected: $ ls 01-schema.sql02-data1.sql03-fixups.sql $ psql database -- *.sql etc. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] Re: [COMMITTERS] pgsql: Add missing keywords to gram.y's unreserved_keywords list.
On Mar 11, 2011, at 1:40 PM, Robert Haas wrote: On Fri, Mar 11, 2011 at 2:39 PM, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: On 11.03.2011 20:59, Robert Haas wrote: On Tue, Mar 8, 2011 at 4:44 PM, Tom Lanet...@sss.pgh.pa.us wrote: Add missing keywords to gram.y's unreserved_keywords list. We really need an automated check for this ... and did VALIDATE really need to become a keyword at all, rather than picking some other syntax using existing keywords? I think we ought to try to do something about this, so that VALIDATE doesn't need to become a keyword. How about instead of VALIDATE CONSTRAINT we simply write ALTER CONSTRAINT ... VALID? (Patch attached, passes make check.) ALTER CONSTRAINT ... VALID sounds like it just marks the constraint as valid. VALIDATE CONSTRAINT sounds like it scans and checks that the constraint is valid. Yeah, it's a little awkward, but I think it's still better than adding another keyword. Any other ideas for wording? CHECK VALID? Regards, David -- David Christensen End Point Corporation da...@endpoint.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] Re: [ADMIN] PD_ALL_VISIBLE flag was incorrectly set happend during repeatable vacuum
On Feb 28, 2011, at 3:28 PM, daveg wrote:
On Wed, Jan 12, 2011 at 10:46:14AM +0200, Heikki Linnakangas wrote:
On 12.01.2011 06:21, Fujii Masao wrote:
On Sat, Dec 25, 2010 at 2:09 PM, Maxim Bogukmaxim.bo...@gmail.com wrote:
While I trying create reproducible test case for BUG #5798 I
encountered very strange effect on two of my servers (both servers
have same hardware platform/OS (freebsd 7.2) and PostgreSQL 8.4.4).
Very simple test table created as:
CREATE TABLE test (id integer);
INSERT INTO test select generate_series(0,1);
And I trying repeateble vacuum of that table with script:
perl -e foreach (1..10) {system \psql -d test -h -c 'vacuum
test'\;}
And once per like an minute (really random intervals can be 5 minutes
without problems can be 3 vacuum in row show same error) I getting
next errors:
WARNING: PD_ALL_VISIBLE flag was incorrectly set in relation test page
1
...
WARNING: PD_ALL_VISIBLE flag was incorrectly set in relation test
page 30 for all pages of the relation.
Oh, interesting. This is the first time anyone can reliably reproducible
that. I can't reproduce that on my laptop with that script, though, so
I'm going to need your help to debug this.
Can you compile PostgreSQL with the attached patch, and rerun the test?
It will dump the pages with incorrectly set flags to files in /tmp/, and
adds a bit more detail in the WARNING. Please run the test until you
get those warnings, and tar up the the created /tmp/pageimage* files,
and post them along with the warning generated.
We'll likely need to go back and forth a few times with various
debugging patches until we get to the heart of this..
Anything new on this? I'm seeing at on one of my clients production boxes.
Also, what is the significance, ie what is the risk or damage potential if
this flag is set incorrectly?
Was this cluster upgraded to 8.4.4 from 8.4.0? It sounds to me like a known
bug in 8.4.0 which was fixed by this commit:
commit 7fc7a7c4d082bfbd579f49e92b046dd51f1faf5f
Author: Tom Lane t...@sss.pgh.pa.us
Date: Mon Aug 24 02:18:32 2009 +
Fix a violation of WAL coding rules in the recent patch to include an
all tuples visible flag in heap page headers. The flag update *must*
be applied before calling XLogInsert, but heap_update and the tuple
moving routines in VACUUM FULL were ignoring this rule. A crash and
replay could therefore leave the flag incorrectly set, causing rows
to appear visible in seqscans when they should not be. This might explain
recent reports of data corruption from Jeff Ross and others.
In passing, do a bit of editorialization on comments in visibilitymap.c.
oy:postgresql machack$ git describe --tag
7fc7a7c4d082bfbd579f49e92b046dd51f1faf5f
REL8_4_0-190-g7fc7a7c
If the flag got twiddled while running as 8.4.0, the incorrect PD_ALL_VISIBLE
flag would (obviously) not be fixed by the upgrade to 8.4.4. (Is this a
separate issue?)
Regards,
David
--
David Christensen
End Point Corporation
da...@endpoint.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] ALTER EXTENSION UPGRADE, v3
I don't see how that affects my point? You can spell 1.0 as 0.1 and 1.1 as 0.2 if you like that kind of numbering, but I don't see that that has any real impact. At the end of the day an author is going to crank out a series of releases, and if he cares about people using those releases for production, he's going to have to provide at least a upgrade script to move an existing database from release N to release N+1. Yeah, but given a rapidly-developing extension, that could create a lot of extra work. I don't know that there's much of a way around that, other than concatenating files to build migration scripts from parts (perhaps via `Make` as dim suggested). But it can get complicated pretty fast. My desire here is to keep the barrier to creating PostgreSQL extensions as low as is reasonably possible. I assume this has already been discussed and rejected (or it wouldn't still be an issue), but what's wrong with the equivalent of \i in the successive .sql upgrade files? Or is the server running the scripts itself and no equivalent include feature exists in raw sql? Regards, David -- David Christensen End Point Corporation da...@endpoint.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] plperlu problem with utf8
On Dec 19, 2010, at 2:20 AM, Alex Hunsaker wrote:
On Sat, Dec 18, 2010 at 20:29, David E. Wheeler da...@kineticode.com wrote:
...
I would argue that it should output the same as the first example. That is,
PL/Perl should have decoded the latin-1 before passing the text to the Perl
function.
Yeah, I don't think you will find anyone who disagrees :) PL/TCL and
PL/Python get this right FWIW. Anyway find attached a patch that does
just this.
Cool, thanks for taking this on.
With the attached we:
- for function arguments, convert (using pg_do_encoding_conversion) to
utf8 from the current database encoding. We also turn on the utf8
flag so perl knows it was given utf8. Pre patch things only really
worked for SQL_ASCII or PG_UTF8 databases. In practice everything
worked fine for single byte charsets. However things like uc() or
lc() on bytes with high bits set were probably broken.
How does this deal with input records of composite type?
- for output from perl convert from perls internal format to utf8
(using SvPVutf8()), and then convert that to the current database
encoding. This sounds unoptimized, but in the common case SvPVutf8()
should be a noop. Pre patch this was random (dependent on how perl
decided to represent the string internally) but it worked 99% of the
time (at least in the single byte charset or UTF8 cases).
- fix errors so they properly encode their message to the current
database encoding (pre patch we were doing no conversion at all,
similar to the output case were it worked most of the time)
This sounds good; I imagine in practice most errors contain just 7-bit ascii
which should be acceptable in any server_encoding, but in the case where
something is returned that is unable to be represented in the server_encoding
(thinking values defined/used in the function itself), does it degrade to the
current behavior, as opposed to fail or eat the error message without
outputting?
- always do the utf8 hack so utf8.pm is loaded (fixes utf8 regexs in
plperl). Pre patch this only happened on a UTF8 database. That meant
multi-byte character regexs were broken on non utf8 databases.
This sounds good in general, but I think should be skipped if
GetDatabaseEncoding() == SQL_ASCII.
-remove some old perl version checks for 5.6 and 5.8. We require
5.8.1 so these were nothing but clutter.
+1. Can't complain about removing clutter :-).
Something interesting to note is when we are SQL_ASCII,
pg_do_encoding_conversion() does nothing, yet we turn on the utf8
flag. This means if you pass in valid utf8 perl will treat it as
such. It also means on output it will hand utf8 back. Both PL/Tcl
and PL/Python do the same thing so I suppose its sensible to match
their behavior (and it was the lazy thing to do). The difference
being with PL/Python if you return said string you get ERROR:
PL/Python: could not convert Python Unicode object to PostgreSQL
server encoding. While PL/Tcl and now Pl/perl give you back a utf8
version. For example:
(SQL_ASCII database)
=# select length('☺');
length
3
=# CREATE FUNCTION tcl_len(text) returns text as $$ return [string
length $1] $$ language pltcl;
CREATE FUNCTION
postgres=# SELECT tcl_len('☺');
tcl_len
1
(1 row)
=# CREATE FUNCTION py_len(str text) returns text as $$ return
len(str) $$ language plpython3;
=# SELECT py_len('☺');
py_len
1
(1 row)
I wouldn't really say thats right, but its at least consistent...
I think this could/should be adequately handled by not calling the function
when the DatabaseEncoding is SQL_ASCII. Since SQL_ASCII basically makes no
assumptions about any representation other than arbitrary 8-bit encoding, this
demonstrated behavior is more-or-less attaching incorrect semantics to values
that are being returned, and is completely bunko IMHO. (Not that many people
are hopefully running SQL_ASCII at this point, but you never know...) Also,
I'd argue that pltcl and plpython should be fixed as well for the same reasons.
This does *not* address the bytea issue where currently if you have
bytea input or output we try to encode that the same as any string. I
think thats going to be a bit more invasive and this patch should
stands on its own.
plperl_fix_enc.patch.gz
Yeah, I'm not sure how invasive that will end up being, or if there are other
datatypes which should skip the text processing.
I noticed you moved the declaration of perl_sys_init_done; was that an
independent bug fix, or did something in the patch require that?
Cheers,
David
--
David Christensen
End Point Corporation
da...@endpoint.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] plperlu problem with utf8
= uri_unescape($_[0]);
warn(length($str));
return $str;
$$ LANGUAGE plperlu;
# SELECT url_decode('comment%20passer%20le%20r%C3%A9veillon');
WARNING: 28 at line 5.
CONTEXT: PL/Perl function url_decode
url_decode
-
comment passer le réveillon
(1 row)
Looks harmless enough...
Looks far better, in fact. Interesting that URI::Escape does the right thing
only if the utf8 flag has been turned on in the string passed to it. But in
Perl it usually won't be, because the encoded string should generally have
only ASCII characters.
I think you'll find that this correct display is actually an artifact of your
terminal type being set to a UTF-8 compatible encoding and interpreting the raw
output as the UTF-8 sequence in its output display; that returned count is
actually the number of octets, compare:
$ perl -MURI::Escape -e'print
length(uri_unescape(q{comment%20passer%20le%20r%C3%A9veillon}))'
28
$ perl -MEncode -MURI::Escape -e'print
length(decode_utf8(uri_unescape(q{comment%20passer%20le%20r%C3%A9veillon})))'
27
# SELECT length(url_decode('comment%20passer%20le%20r%C3%A9veillon'));
WARNING: 28 at line 5.
CONTEXT: PL/Perl function url_decode
length
27
(1 row)
Wait a minute... those lengths should match.
Post patch they do:
# SELECT length(url_decode('comment%20passer%20le%20r%C3%A9veillon'));
WARNING: 28 at line 5.
CONTEXT: PL/Perl function url_decode
length
28
(1 row)
Still confused? Yeah me too.
Yeah…
As shown above, the character length for the example should be 27, while the
octet length for the UTF-8 encoded version is 28. I've reviewed the source of
URI::Escape, and can say definitively that: a) regular uri_escape does not
handle 255 code points in the encoding, but there exists a uri_escape_utf8
which will convert the source string to UTF8 first and then escape the encoded
value, and b) uri_unescape has *no* logic in it to automatically decode from
UTF8 into perl's internal format (at least as far as the version that I'm
looking at, which came with 5.10.1).
Maybe this will help:
#!/usr/bin/perl
use URI::Escape;
my $str = uri_unescape(%c3%a9);
die first match if($str =~ m/\xe9/);
utf8::decode($str);
die 2nd match if($str =~ m/\xe9/);
gives:
$ perl t.pl
2nd match at t.pl line 6.
see? Either uri_unescape() should be decoding that utf8() or you need
to do it *after* you call uri_unescape(). Hence the maybe it could be
considered a bug in uri_unescape().
Agreed.
-1; if you need to decode from an octets-only encoding, it's your
responsibility to do so after you've unescaped it. Perhaps later versions of
the URI::Escape module contain a uri_unescape_utf8() function, but it's
trivially: sub uri_unescape_utf8 { Encode::decode_utf8(uri_unescape(shift))}.
This is definitely not a bug in uri_escape, as it is only defined to return
octets.
* Values returned from PL/Perl functions that are in Perl's internal
representation should be encoded into the server encoding before they're
returned.
I didn't really follow all of the above; are you aiming for the same thing?
Yeah, the patch address this part. Right now we just spit out
whatever the internal format happens to be.
Ah, excellent.
I agree with the sentiments that: data (server_encoding) - function parameters
(- perl internal) - function return (- server_encoding). This should be for
any character-type data insofar as it is feasible, but ISTR there is already
datatype-specific marshaling occurring.
Anyway its all probably clear as mud, this part of perl is one of the
hardest IMO.
No question.
There is definitely a lot of confusion surrounding perl's handling of character
data; I hope this was able to clear a few things up.
Regards,
David
--
David Christensen
End Point Corporation
da...@endpoint.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] ALTER TABLE ... REPLACE WITH
On Dec 15, 2010, at 4:39 AM, Simon Riggs wrote: On Wed, 2010-12-15 at 10:54 +0100, Csaba Nagy wrote: On Tue, 2010-12-14 at 14:36 -0500, Robert Haas wrote: Well, you have to do that for DROP TABLE as well, and I don't see any way around doing it for REPLACE WITH. Sure, but in Simon's proposal you can load the data FIRST and then take a lock just long enough to do the swap. That's very different from needing to hold the lock during the whole data load. Except Simon's original proposal has this line in it: * new_table is TRUNCATEd. I guess Simon mixed up new_table and old_table, and the one which should get truncated is the replaced one and not the replacement, otherwise it doesn't make sense to me. What I meant was... REPLACE TABLE target WITH source; * target's old rows are discarded * target's new rows are all of the rows from source. * source is then truncated, so ends up empty Perhaps a more useful definition would be EXCHANGE TABLE target WITH source; which just swaps the heap and indexes of each table. You can then use TRUNCATE if you want to actually destroy data. Are there any considerations with toast tables and the inline line pointers for toasted tuples? Regards, David -- David Christensen End Point Corporation da...@endpoint.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] SQL command to edit postgresql.conf, with comments
On Oct 13, 2010, at 10:24 AM, Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: +1. Preserving the comments when you change the value could make the comments totally bogus. Separating machine-generated values into a separate file makes plenty of sense to me. Which one wins, though? I can see cases being made for both. IIRC the proposal was that postgresql.conf (the people-editable file) would have include postgresql.auto in it. You could put that at the top, bottom, or even middle to control how the priority works. So it's user-configurable. I think the factory default ought to be to have it at the top, which would result in manually edited settings (if any) overriding SET PERMANENT. Since this is just touching the local servers' postgresql.conf.auto (or whatever), any reason why SET PERMANENT couldn't be used on a read-only standby? Could this be to manage some of the failover scenarios (i.e., setting any relevant config from a central clusterware|whatever)? Regards, David -- David Christensen End Point Corporation da...@endpoint.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] Which file does the SELECT?
On Oct 10, 2010, at 12:21 PM, Vaibhav Kaushal wrote: Thanks to both hitoshi and tom for your replies. I think I need to look into the Postgres code itself (I am better at code than documentation). But since I have not been touch with C lately (these days I am programming on PHP) I think I have forgot a few rules of game (afterall PHP is so much more easy than C :P ). Moreover, postgres is the first Open Source software whose code I am interested in. I have never looked into other OSS codes much except correcting a few compilation errors here and there on beta / alpha releases. I have had the chance and success to compile my own Linux OS and it was fun to do so... but I guess development is a tougher job. With an idea in mind, and a thankful feeling towards postgres is what drives me to do this tougher job. When I was designing my database for a web app, I found so many problems in MySQL that I could not continue (the best of all, I can't use the commands written in my DB book to create a foreign key, it does not natively support foreign keys, confusing storage engines and so on).. and then I got postgres which I am a fan of. I hope I will not be flamed when I will ask those questions (some of them are actually very silly ones). I will look inside the code now and will get back after i get some progress with it. However, I find too many references to the Data structure datum what is it and where is it defined? Can someone tell me please? Also, what role does it play? Thanks to you all for your replies. -Vaibhav Depending on your text editor, you may be able to utilize TAGS files; see src/tools/make_(e|c)tags for creating TAGS files for your editor of choice (emacs/vim, although other editors may support specific formats). This will allow you to navigate to the specific definition of the type/function/macro, and can be very enlightening and help answer some of these questions. `git grep` will also come in handy if you're working directly from a git checkout. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] standby registration (was: is sync rep stalled?)
On Oct 4, 2010, at 2:02 PM, Robert Haas wrote: On Mon, Oct 4, 2010 at 1:57 PM, Markus Wanner mar...@bluegap.ch wrote: On 10/04/2010 05:20 PM, Robert Haas wrote: Quorum commit, even with configurable vote weights, can't handle a requirement that a particular commit be replicated to (A || B) (C || D). Good point. Can the proposed standby registration configuration format cover such a requirement? Well, if you can name the standbys, there's no reason there couldn't be a parameter that takes a string that looks pretty much like the above. There are, of course, some situations that could be handled more elegantly by quorum commit (any 3 of 5 available standbys) but the above is more general and not unreasonably longwinded for reasonable numbers of standbys. Is there any benefit to be had from having standby roles instead of individual names? For instance, you could integrate this into quorum commit to express 3 of 5 reporting standbys, 1 berlin standby and 1 tokyo standby from a group of multiple per data center, or even just utilize role sizes of 1 if you wanted individual standbys to be named in this fashion. This role could be provided on connect of the standby is more-or-less tangential to the specific registration issue. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] [COMMITTERS] pgsql: Still more tweaking of git_changelog.
On Sep 26, 2010, at 2:24 PM, Tom Lane wrote: I wrote: I think we could get that behavior fairly easily by remembering the last tag matching one of the commits on a branch, as we chase the branch backwards. I find that this works just fine for the branches, but not so well for master, because more often than not the initial RELx_y_z tag is applied on the release's branch and not on master. So for example commits on master appear to jump from REL7_2 development to REL8_0 development, because 7.3 and 7.4 have no release tag on the master branch. We could perhaps fix that if there were an inexpensive way to get the SHA1 of the master commit that each branch is sprouted from. However, I'm inclined to propose that we instead manually place a tag at each sprout point. Any thoughts about that? In particular, what should the naming convention for such tags be? I would have liked to use RELx_y, but that's out because before 8.0 we named the initial releases that way. Particularly with PostgreSQL's linear branch history, `git merge-base` will show you the point at which the branches diverged from master: $ git merge-base origin/REL9_0_STABLE master 1084f317702e1a039696ab8a37caf900e55ec8f2 $ git show 1084f317702e1a039696ab8a37caf900e55ec8f2 commit 1084f317702e1a039696ab8a37caf900e55ec8f2 Author: Marc G. Fournier scra...@hub.org Date: Fri Jul 9 02:43:12 2010 + tag beta3 Also, the `git describe` command can be used to identify the closest tag a specific commit is a part of: $ git describe --tags 6d297e0551a2a3cc048655796230cdff5e559952 REL9_0_BETA2-153-g6d297e0 This indicates that the indicated commit is the 153rd commit after the REL9_0_BETA2 tag (and includes the abbreviated SHA1 for unique identification in the case that there are multiple branches which have since been re-merged; not the case in this project, but still handy for uniqueness). The command `git branch --contains` will come in handy for commits which are the exact same content (ignoring the commit parentage and time and using only the patch-id (`git --help patch-id`)). This obviously won't catch commits where the changed content had to be modified in the back-patching process, but could serve as a quick basis. (In truth, this may be relatively useless, as I tried to find an example which included back-branches, but wasn't able to find an example off-hand.) In any case, the option exists... :-) $ git branch -a --contains 2314baef38248b31951d3c8e285e6f8e4fd7dd05 * master remotes/origin/HEAD - origin/master remotes/origin/REL8_4_STABLE remotes/origin/REL8_5_ALPHA1_BRANCH remotes/origin/REL8_5_ALPHA2_BRANCH remotes/origin/REL8_5_ALPHA3_BRANCH remotes/origin/REL9_0_ALPHA4_BRANCH remotes/origin/REL9_0_ALPHA5_BRANCH remotes/origin/REL9_0_STABLE remotes/origin/master Regards, David -- David Christensen End Point Corporation da...@endpoint.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] Multi-branch committing in git, revisited
If I commit in master before I start working on 9.0, and so on back, then the commits will be separated in time by a significant amount, thus destroying any chance of having git_topo_order recognize them as related. So we're back up against the problem of git not really understanding the relationships of patches in different branches. Is the issue here the clock time spent between the commits, i.e., the possibility that someone is going to push to the specific branches in between or the date/time that the commit itself displays? Because if it's specifically commit time that's at issue, I believe `git cherry-pick` preserves the original commit time from the original commit, which should make that a non-issue. Even if you need to fix up a commit to get the cherry-pick to apply, you can always `git commit -C ref-of-cherry-pick` to preserve the authorship/commit time for the commit in question. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] WIP: Triggers on VIEWs
On Sep 5, 2010, at 3:09 AM, Dean Rasheed wrote: On 15 August 2010 18:38, Dean Rasheed dean.a.rash...@gmail.com wrote: Here is a WIP patch implementing triggers on VIEWs ... snip There are still a number of things left todo: - extend ALTER VIEW with enable/disable trigger commands - much more testing - documentation Attached is an updated patch with more tests and docs, and a few minor At least for me, there are some portions of the docs which could use some formatting changes in order to not be confusing grammatically; e.g., this was confusing to me on the first read: -trigger name. In the case of before triggers, the +trigger name. In the case of before and instead of triggers, the I realize this lack of formatting was inherited from the existing docs, but this would make more sense to me if this (and presumably the other related instances of before and after) were set apart with literal/ or similar. This is already in use in some places in this patch, so seems like the correct markup. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] permissions bug in RI checks?
Hey -hackers, In doing a schema upgrade, we noticed the following behavior, which certainly seems like a bug. Steps to reproduce: CREATE USER a; CREATE USER b; CREATE TABLE t1 (id serial primary key); CREATE TABLE t2 (id int references t1(id)); ALTER TABLE t1 OWNER TO a; ALTER TABLE t2 OWNER TO a; \c - a REVOKE ALL ON t1 FROM a; REVOKE ALL ON t2 FROM a; GRANT ALL ON t1 TO b; GRANT ALL ON t2 TO b; \c - b INSERT INTO t2 (id) VALUES (1); ERROR: permission denied for relation t1 CONTEXT: SQL statement SELECT 1 FROM ONLY public.t1 x WHERE id OPERATOR(pg_catalog.=) $1 FOR SHARE OF x The bug in this case is that b has full permissions on all of the underlying tables, but runs into issues when trying to access the referenced tables. I traced this down to the RI checks, specifically the portion in ri_PlanCheck() where it calls SetUserIdAndSecContext() and then later runs the queries in the context of the owner of the relation. Since the owner a lacks SELECT and UPDATE privileges on the table, it is not able to take the ShareLock, and spits out the above error. This behavior does not occur if the object owner is a database superuser. This is presumably because the superuser bypasses the regular ACL checks and succeeds regardless. The behavior was originally noted in 8.1.21, but exists as well in HEAD. No real resolution proposed, but I wanted to understand the reason behind the restrictions if it was intentional behavior. Thanks, David Regards, David -- David Christensen End Point Corporation da...@endpoint.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] git: uh-oh
On Aug 17, 2010, at 2:29 PM, Magnus Hagander wrote: On Tue, Aug 17, 2010 at 21:28, Robert Haas robertmh...@gmail.com wrote: On Tue, Aug 17, 2010 at 3:23 PM, Magnus Hagander mag...@hagander.net wrote: The tip of every branch, and every single tag, all have the correct data in them, but some revisions in between seem majorly confused. It seems to me that what we'll need to do here is write a script to look through the CVS history of each file and make sure that the versions of that file which appear on each branch match the revs in CVS in content, order, and the commit message associated with any changes. However, that's not going to do get done today. Yeah. Unless someone comes up with a good way to fix this, or even better an explanation why it's actually ont broken and we're looking at things wrong :D, I think we have no choice but aborting the conversion for now and come back to it later. Can you post the cvs2svn command line used for conversion? Regards, David -- David Christensen End Point Corporation da...@endpoint.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] Patch for 9.1: initdb -C option
On Jul 23, 2010, at 6:36 AM, Robert Haas wrote: 2010/7/23 KaiGai Kohei kai...@ak.jp.nec.com: Sorry for the confusion. What I wanted to say is the patch itself is fine but we need to make consensus before the detailed code reviewing. I guess we probably need some more people to express an opinion, then. Do you have one? I'm not sure I do, yet. I'd like to hear the patch author's response to Itagaki Takahiro's question upthread: Why don't you use just echo 'options' $PGDATA/postgresql.conf ? Could you explain where the -C options is better than initdb + echo? At this point, I have no real preference for this patch; it is just as easy to echo line datadir/postgresql.conf, so perhaps that makes this patch somewhat pointless. I suppose there's a shaky argument to be made for Windows compatibility, but I'm sure there's also an equivalent functionality to be found in the windows shell. Reception to this idea has seemed pretty lukewarm, although I think Peter expressed some interest. Some of the previous linked correspondence in the review referred to some of the proposed split configuration file mechanisms. My particular implementation is fairly limited to the idea of a single configuration file, so compared to some of the other proposed approaches including split .conf files, it may not cover the same ground. Like I said in the original submission, I found it helpful for the programmatic configuration of a number of simultaneous node, but if it's not generally useful to the community at large, I'll understand if it's punted. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] Explicit psqlrc
On Jul 21, 2010, at 9:42 AM, Robert Haas wrote: On Wed, Jul 21, 2010 at 10:24 AM, Peter Eisentraut pete...@gmx.net wrote: On tis, 2010-07-20 at 11:48 -0400, Robert Haas wrote: It's tempting to propose making .psqlrc apply only in interactive mode, period. But that would be an incompatibility with previous releases, and I'm not sure it's the behavior we want, either. What is a use case for having .psqlrc be read in noninteractive use? Well, for example, if I hate the new ASCII format with a fiery passion that can never be quenched (and, by the way, I do), then I'd like this to apply: \pset linestyle old-ascii Even when I do this: psql -c '...whatever...' Well, tossing out two possible solutions: 1) .psqlrc + .psql_profile (kinda like how bash separates out the interactive/non-interactive parts). Kinda yucky, but it's a working solution. 2) have a flag which explicitly includes the psqlrc file in non-interactive use (perhaps if -x is available, use it for the analogue to -X). Regards, David -- David Christensen End Point Corporation da...@endpoint.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] documentation for committing with git
On Jul 21, 2010, at 2:20 PM, Robert Haas wrote: On Wed, Jul 21, 2010 at 3:11 PM, Magnus Hagander mag...@hagander.net wrote: 6. Finally, you must push your changes back to the server. git push This will push changes in all branches you've updated, but only branches that also exist on the remote side will be pushed; thus, you can have local working branches that won't be pushed. == This is true, but I have found it saner to configure push.default = tracking, so that only the current branch is pushes. Some people might find that useful. Indeed. Why don't I do that more often... +1 on making that a general recommendation, and have people only not do that if they really know what they're doing :-) Hmm, I didn't know about that option. What makes us think that's the behavior people will most often want? Because it doesn't seem like what I want, just for one example... So you're working on some back branch, and make a WIP commit so you can switch to master to make a quick commit. Create a push on master. Bare git push. WIP commit gets pushed upstream. Oops. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] documentation for committing with git
On Jul 21, 2010, at 2:39 PM, Magnus Hagander wrote: On Wed, Jul 21, 2010 at 21:37, Andrew Dunstan and...@dunslane.net wrote: Robert Haas wrote: At the developer meeting, I promised to do the work of documenting how committers should use git. So here's a first version. http://wiki.postgresql.org/wiki/Committing_with_Git Note that while anyone is welcome to comment, I mostly care about whether the document is adequate for our existing committers, rather than whether someone who is not a committer thinks we should manage the project differently... that might be an interesting discussion, but we're theoretically making this switch in about a month, and getting agreement on changing our current workflow will take about a decade, so there is not time now to do the latter before we do the former. So I would ask everyone to consider postponing those discussions until after we've made the switch and ironed out the kinks. On the other hand, if you have technical corrections, or if you have suggestions on how to do the same things better (rather than suggestions on what to do differently), that would be greatly appreciated. Well, either we have a terminology problem or a statement of policy that I'm not sure I agree with, in point 2. IMNSHO, what we need to forbid is commits that are not fast-forward commits, i.e. that do not have the current branch head as an ancestor, ideally as the immediate ancestor. Personally, I have a strong opinion that for everything but totally trivial patches, the committer should create a short-lived work branch where all the work is done, and then do a squash merge back to the main branch, which is then pushed. This pattern is not mentioned at all. In my experience, it is essential, especially if you're working on more than one thing at a time, as many people often are. Uh, that's going to create an actual merge commit, no? Or you mean squash-merge-but-only-fast-forward? I *think* the docs is based off the pattern of the committer having two repositories - one for his own work, one for comitting, much like I assume all of us have today in cvs. You can also do a rebase after the merge to remove the local merge commit before pushing. I tend to do this anytime I merge a local branch, just to rebase on top of the most recent origin/master. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] psql \conninfo command (was: Patch: psql \whoami option)
On Jul 21, 2010, at 8:48 PM, Fujii Masao wrote: On Wed, Jul 21, 2010 at 7:29 PM, Robert Haas robertmh...@gmail.com wrote: On Wed, Jul 21, 2010 at 1:07 AM, Fujii Masao masao.fu...@gmail.com wrote: On Tue, Jul 20, 2010 at 11:14 PM, Robert Haas robertmh...@gmail.com wrote: OK, committed. When I specify the path of the directory for the Unix-domain socket as the host, \conninfo doesn't mention that this connection is based on the Unix-domain socket. Is this intentional? $ psql -h/tmp -c\conninfo You are connected to database postgres on host /tmp at port 5432 as user postgres. I expected that something like You are connected to database postgres via local socket on /tmp at port 5432 as user postgres. :-( No, I didn't realize the host field could be used that way. It's true that you get a fairly similar message from \c, but that's not exactly intuitive either. rhaas=# \c - - /tmp - You are now connected to database rhaas on host /tmp. OK. The attached patch makes \conninfo command emit the following message if the host begins with a slash: $ psql -h/tmp -c\conninfo You are connected to database postgres via local socket on /tmp at port 5432 as user postgres. Similarly, it makes \c command emit the following message in that case: $ psql -hlocalhost -c\c - - /tmp - You are now connected to database postgres via local socket on /tmp. If we print the local socket when it's been explicitly set via the host= param, why not display the actual socket path in the general local socket case? Also, while we're still tweaking this patch, I've had a couple requests for the SSL status of the connection as well; does this seem like a generally useful parameter to display as well? Regards, David -- David Christensen End Point Corporation da...@endpoint.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] Explicit psqlrc
On Jul 21, 2010, at 12:31 PM, Alvaro Herrera wrote:
Excerpts from Peter Eisentraut's message of mié jul 21 10:24:26 -0400 2010:
On tis, 2010-07-20 at 11:48 -0400, Robert Haas wrote:
It's tempting to propose making .psqlrc apply only in interactive
mode, period. But that would be an incompatibility with previous
releases, and I'm not sure it's the behavior we want, either.
What is a use case for having .psqlrc be read in noninteractive use?
Even if there weren't one, why does it get applied to -f but not -c?
They're both noninteractive.
So not to let the thread drop, it appears that we're faced with the following
situation:
1) The current behavior is inconsistent with the psqlrc handling of -c and -f.
2) The current behavior is still historical and we presumably want to maintain
it.
I'm not sure of the cases where we're willing to break backwards compatibility,
but I do know that it's not done lightly. So perhaps for this specific patch,
we'd need/want to punt supporting both -c's in conjunction with -f's, at least
until we can resolve some of the ambiguities in what the actual behavior should
be in each of these cases. This could still be a followup patch for 9.1, if we
get these issues resolved.
And as long as we're redesigning the bike shed, I think a better use case for
supporting multiple sql files would be to support them in such a way that you
wouldn't need to provide explicit -f flags for each. Many programs utilize the
'--' token for an end of options flag, with the rest of the arguments then
becoming something special, such as filenames. So what about adding the
interpretation that anything after '--' is interpreted as a filename? That
will allow the use of shell wildcards to specify multiple files, and thus allow
something like:
$ psql -U myuser mydatabase -- *.sql
$ psql -- {pre-,,post-}migration.sql
while still being unambiguous with the current convention of having an
unspecified argument be interpreted as a database name. This would make it
possible to actually specify/use multiple files in a fashion that people are
used to doing, as opposed to having to explicitly type things out or do
contortions will shell substitutions, etc.
Regards,
David
--
David Christensen
End Point Corporation
da...@endpoint.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] Explicit psqlrc
On Jul 19, 2010, at 10:40 PM, Robert Haas wrote:
On Wed, Jun 23, 2010 at 9:22 AM, Robert Haas robertmh...@gmail.com wrote:
On Wed, Jun 23, 2010 at 9:17 AM, gabrielle gor...@gmail.com wrote:
On Mon, Jun 21, 2010 at 6:16 PM, Robert Haas robertmh...@gmail.com wrote:
Well, that might be a good idea, too, but my expectation is that:
psql -f one -f two -f three
ought to behave in a manner fairly similar to:
cat one two three all
psql -f all
and it sounds like with this patch that's far from being the case.
Correct. Each is handled individually.
Should I continue to check on ON_ERROR_ROLLBACK results, or bounce
this back to the author?
It doesn't hurt to continue to review and find other problems so that
the author can try to fix them all at once, but it certainly seems
clear that it's not ready to commit at this point, so it definitely
needs to go back to the author for a rework.
Since it has been over a month since this review was posted and no new
version of the patch has appeared, I think we should mark this patch
as Returned with Feedback.
Sorry for the delays in response. This is fine; I think there are some
semantic questions that should still be resolved at this point, particularly if
we're moving toward supporting multiple -c and -f lines as expressed (an idea
that I agree would be useful). Specifically:
1) Does the -1 flag with multiple files indicate a single transaction for all
commands/files or that each command/file is run in its own transaction? I'd
implemented this with the intent that all files were run in a single
transaction, but it's at least a bit ambiguous, and should probably be
documented at the very least.
2) I had a question (expressed in the comments) about how the final error
handling status should be reported/handled. I can see this affecting a number
of things, particularly ON_ERROR_{STOP,ROLLBACK} behavior; specifically, if
there was an error, it sounds like it should just abort processing of any other
queued files/commands at this point (in the case of ON_ERROR_STOP, at least).
3) With the switch to multiple intermixed -c and -f flags, the internal
representation will essentially have to change to a queue of structs; I think
in that case, we'd want to modify the current .psqlrc handling to push a struct
representing the .psqlrc file at the front of the queue, depending on the code
paths that currently set this up. Are there any gotchas to this approach?
(I'm looking essentially for odd code paths where say .psqlrc was not loaded
before, but now would be given the proper input of -c, -f file, -f -.)
I'll look more in-depth at the posted feedback and Mark's proposed patch.
Regards,
David
--
David Christensen
End Point Corporation
da...@endpoint.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] Explicit psqlrc
On Jul 20, 2010, at 9:05 AM, Robert Haas wrote:
On Tue, Jul 20, 2010 at 10:00 AM, David Christensen da...@endpoint.com
wrote:
Sorry for the delays in response. This is fine; I think there are some
semantic questions that should still be resolved at this point, particularly
if we're moving toward supporting multiple -c and -f lines as expressed (an
idea that I agree would be useful). Specifically:
1) Does the -1 flag with multiple files indicate a single transaction for
all commands/files or that each command/file is run in its own transaction?
I'd implemented this with the intent that all files were run in a single
transaction, but it's at least a bit ambiguous, and should probably be
documented at the very least.
I think your implementation is right. Documentation is always good.
2) I had a question (expressed in the comments) about how the final error
handling status should be reported/handled. I can see this affecting a
number of things, particularly ON_ERROR_{STOP,ROLLBACK} behavior;
specifically, if there was an error, it sounds like it should just abort
processing of any other queued files/commands at this point (in the case of
ON_ERROR_STOP, at least).
Right. I think it should behave much as if you concatenated the files
and then ran psql on the result. Except with better reporting of
error locations, etc.
3) With the switch to multiple intermixed -c and -f flags, the internal
representation will essentially have to change to a queue of structs; I
think in that case, we'd want to modify the current .psqlrc handling to push
a struct representing the .psqlrc file at the front of the queue, depending
on the code paths that currently set this up. Are there any gotchas to this
approach? (I'm looking essentially for odd code paths where say .psqlrc was
not loaded before, but now would be given the proper input of -c, -f file,
-f -.)
I'll look more in-depth at the posted feedback and Mark's proposed patch.
Well, IIRC, one of -c and -f suppresses psqlrc, and the other does
not. This doesn't seem very consistent to me, but I'm not sure
there's much to be done about it at this point. I guess if you use
whichever one suppresses psqlrc even once, it's suppressed, and
otherwise it's not. :-(
That seems sub-optimal; I can see people wanting to use this feature to do
something like:
psql -c 'set work_mem = blah' -f script.sql
and then being surprised when it works differently than just `psql -f
script.sql`.
psql -c select 'starting' -f file1.sql -c select 'midway' -f file2.sql -c
select 'done!'
Although I wonder if the general usecase for .psqlrc is just in interactive
mode; i.e., hypothetically if you're running scripts that are sensitive to
environment, you're running with -X anyway; so maybe that's not that big of a
deal, as it's kind of an implied -X with multiple -c or -f commands. And if
you really wanted it, we could add a flag to explicitly include .psqlrc (or the
user could just specify -f path/to/psqlrc).
Regards,
David
--
David Christensen
End Point Corporation
da...@endpoint.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] Explicit psqlrc
On Jul 20, 2010, at 2:18 PM, Alvaro Herrera wrote: Excerpts from Robert Haas's message of mar jul 20 11:48:29 -0400 2010: That seems sub-optimal; I can see people wanting to use this feature to do something like: psql -c 'set work_mem = blah' -f script.sql and then being surprised when it works differently than just `psql -f script.sql`. I agree... but then they might also be surprised if psql -c 'something' works differently from psql -c 'something' -f /dev/null I think we should just make sure -X works, and have .psqlrc be read when it's not specified regardless of -f and -c switches. Otherwise it's just plain confusing. +1. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] Patch for 9.1: initdb -C option
On Jul 20, 2010, at 5:00 PM, Kevin Grittner wrote: Top posting. On purpose. :p This patch seems to be foundering in a sea of opinions. It seems that everybody wants to do *something* about this, but what? For one more opinion, my shop has chosen to never touch the default postgresql.conf file any more, beyond adding one line to the bottom of it which is an include directive, to bring in our overrides. What will make everyone happy here? So you'll now issue: $ initdb ... -C 'include localconfig.conf' ? :-) Regards, David -- David Christensen End Point Corporation da...@endpoint.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] psql \conninfo command (was: Patch: psql \whoami option)
On Jul 19, 2010, at 10:34 PM, Robert Haas wrote: On Sun, Jul 18, 2010 at 2:00 PM, David Christensen da...@endpoint.com wrote: Updated the commitfest entry with the patch, updated the title to reflect the actual name of the command, and marked as ready for committer. I took a look at this patch. One problem is that it doesn't handle the case where there is no database connection (for example, shut down the database with pg_ctl, then do select 1, then do \conninfo). I've fixed that in the attached version. Thanks, I hadn't considered that case. However, I'm also wondering about the output format. I feel like it would make sense to use a format similar to what we do for \c, which looks like this: You are now connected to database %s. It prints out more parameters if they've changed. The longest possible version is: You are now connected to database %s on host %s at port %s as user %s. My suggestion is that we use the same format, except (1) always include all the components, since that's the point; (2) don't include the word now; and (3) if there is no host, then print via local socket rather than on host...port So where the current patch prints: Connected to database: rhaas, user: rhaas, port: 5432 via local domain socket I would propose to print instead: You are connected to database rhaas via local socket as user rhaas. If people strongly prefer the way the patch does it now, I don't think it's horrible... but it seems like it would be nicer to be somewhat consistent with the existing message. Thoughts? +1 from me; I don't care what color the bikeshed is, as long as it gets the point across, which this does, and is consistent to boot. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] psql \conninfo command (was: Patch: psql \whoami option)
I would propose to print instead: You are connected to database rhaas via local socket as user rhaas. One minor quibble here; you lose the ability to see which pg instance you're running on if there are multiple ones running on different local sockets, so maybe either the port or the socket path should show up here still. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] psql \conninfo command (was: Patch: psql \whoami option)
On Jul 19, 2010, at 11:10 PM, Robert Haas wrote: On Tue, Jul 20, 2010 at 12:07 AM, Robert Haas robertmh...@gmail.com wrote: On Tue, Jul 20, 2010 at 12:02 AM, David Christensen da...@endpoint.com wrote: I would propose to print instead: You are connected to database rhaas via local socket as user rhaas. One minor quibble here; you lose the ability to see which pg instance you're running on if there are multiple ones running on different local sockets, so maybe either the port or the socket path should show up here still. Doh. Will fix. Something like the attached? Looks good to me. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] Patch: psql \whoami option
On Jun 21, 2010, at 9:00 AM, Tom Lane wrote: Robert Haas robertmh...@gmail.com writes: On Sun, Jun 20, 2010 at 10:51 PM, Steve Singer ssinger...@sympatico.ca wrote: One comment I have on the output format is that values (ie the database name) are enclosed in double quotes but the values being quoted can contain double quotes that are not being escaped. This is the same as standard practice in just about every other message... It seems like for user and database it might be sensible to apply PQescapeIdentifier to the value before printing it. I think this would actually be a remarkably bad idea in this particular instance, because in the majority of cases psql does not apply identifier dequoting rules to user and database names. What is printed should be the same as what you'd need to give to \connect, for example. So I'm not quite sure how the above two paragraphs resolve? Should the user/database names be quoted or not? I have a new version of this patch available which has incorporated the feedback to this point? As an example of the current behavior, consider: machack:machack:5432=# create database foobar machack-# ; CREATE DATABASE [Sun Jul 18 12:14:49 CDT 2010] machack:machack:5432=# \c foobar unterminated quoted string You are now connected to database machack. [Sun Jul 18 12:14:53 CDT 2010] machack:machack:5432=# \c foobar unterminated quoted string You are now connected to database machack. [Sun Jul 18 12:14:59 CDT 2010] machack:machack:5432=# \c foobar You are now connected to database foobar. As you can see, the value passed to connect differs from the output in the connected to database string. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] Patch: psql \whoami option
On Jul 18, 2010, at 12:17 PM, David Christensen wrote: On Jun 21, 2010, at 9:00 AM, Tom Lane wrote: Robert Haas robertmh...@gmail.com writes: On Sun, Jun 20, 2010 at 10:51 PM, Steve Singer ssinger...@sympatico.ca wrote: One comment I have on the output format is that values (ie the database name) are enclosed in double quotes but the values being quoted can contain double quotes that are not being escaped. This is the same as standard practice in just about every other message... It seems like for user and database it might be sensible to apply PQescapeIdentifier to the value before printing it. I think this would actually be a remarkably bad idea in this particular instance, because in the majority of cases psql does not apply identifier dequoting rules to user and database names. What is printed should be the same as what you'd need to give to \connect, for example. So I'm not quite sure how the above two paragraphs resolve? Should the user/database names be quoted or not? I have a new version of this patch available which has incorporated the feedback to this point? As an example of the current behavior, consider: machack:machack:5432=# create database foobar machack-# ; CREATE DATABASE [Sun Jul 18 12:14:49 CDT 2010] machack:machack:5432=# \c foobar unterminated quoted string You are now connected to database machack. [Sun Jul 18 12:14:53 CDT 2010] machack:machack:5432=# \c foobar unterminated quoted string You are now connected to database machack. [Sun Jul 18 12:14:59 CDT 2010] machack:machack:5432=# \c foobar You are now connected to database foobar. As you can see, the value passed to connect differs from the output in the connected to database string. It's helpful when you attach said patch. This has been rebased to current HEAD. Regards, David -- David Christensen End Point Corporation da...@endpoint.com psql-conninfo-v2.patch 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] Patch: psql \whoami option
On Jul 18, 2010, at 12:30 PM, Tom Lane wrote: David Christensen da...@endpoint.com writes: machack:machack:5432=# \c foobar You are now connected to database foobar. What this is reflecting is that backslash commands have their own weird rules for processing double quotes. What I was concerned about was that double quotes in SQL are normally used for protecting mixed case, and you don't need that for \c: regression=# create database FooBar; CREATE DATABASE regression=# \c foobar FATAL: database foobar does not exist Previous connection kept regression=# \c FooBar You are now connected to database FooBar. FooBar=# The fact that there are double quotes around the database name in the You are now connected... message is *not* meant to imply that that is a valid double-quoted SQL identifier, either. It's just an artifact of how we set off names in English-language message style. In another language it might look like FooBar or some such. My opinion remains that you should just print the user and database names as-is, without trying to inject any quoting into the mix. You're more likely to confuse people than help them if you do that. Okay, understood. Then consider my updated patch (just sent attached to a recent message) to reflect the desired behavior. (I'll update the commitfest patch entry when it shows up in the archives.) Thanks, David -- David Christensen End Point Corporation da...@endpoint.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] psql \conninfo command (was: Patch: psql \whoami option)
On Jul 18, 2010, at 12:33 PM, David Christensen wrote: On Jul 18, 2010, at 12:30 PM, Tom Lane wrote: David Christensen da...@endpoint.com writes: machack:machack:5432=# \c foobar You are now connected to database foobar. What this is reflecting is that backslash commands have their own weird rules for processing double quotes. What I was concerned about was that double quotes in SQL are normally used for protecting mixed case, and you don't need that for \c: regression=# create database FooBar; CREATE DATABASE regression=# \c foobar FATAL: database foobar does not exist Previous connection kept regression=# \c FooBar You are now connected to database FooBar. FooBar=# The fact that there are double quotes around the database name in the You are now connected... message is *not* meant to imply that that is a valid double-quoted SQL identifier, either. It's just an artifact of how we set off names in English-language message style. In another language it might look like FooBar or some such. My opinion remains that you should just print the user and database names as-is, without trying to inject any quoting into the mix. You're more likely to confuse people than help them if you do that. Okay, understood. Then consider my updated patch (just sent attached to a recent message) to reflect the desired behavior. (I'll update the commitfest patch entry when it shows up in the archives.) Updated the commitfest entry with the patch, updated the title to reflect the actual name of the command, and marked as ready for committer. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] SHOW TABLES
On Jul 15, 2010, at 2:45 PM, Heikki Linnakangas wrote: On 15/07/10 19:06, Aaron W. Swenson wrote: The best solution is to offer a hint to the user in psql when they submit 'SHOW . . . .' with a response like: SHOW . . . . is not a valid command. Perhaps you mean \d . . . . +1. That doesn't force us to implement a whole new set of commands and syntax to describe stuff in the backend, duplicating the \d commands, but is polite to the users, and immediately guides them to the right commands. You could even do that in the backend for a few simple commands like SHOW TABLES: ERROR: syntax error at SHOW TABLES HINT: To list tables in the database, SELECT * FROM pg_tables or use the \d psql command. This sounds roughly like the patch I submitted in January (linked upthread), although that swiped the input before it hit the backend. I don't know if I like the idea of that HINT or not. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] GSoC - code of implementation of materialized views
On Jun 29, 2010, at 3:31 PM, Pavel Baroš wrote: Robert Haas napsal(a): 2010/6/25 Pavel Baros baro...@seznam.cz: On http://github.com/pbaros/postgres can be seen changes and my attempt to implement materialized views. The first commit to the repository implements following: Materialized view can be created, dropped and used in SELECT statement. CREATE MATERIALIZED VIEW mvname AS SELECT ...; DROP MATERIALIZED VIEW mvname [CASCADE]; SELECT * FROM mvname; also works: COMMENT ON MATERIALIZED VIEW mvname IS 'etc.'; SELECT pg_get_viewdef(mvname); ... also you can look at enclosed patch. So, this patch doesn't actually seem to do very much. It doesn't appear that creating the materialized view actually populates it with any data; and the refresh command doesn't work either. So it appears that you can create a materialized view, but it won't actually contain any data - which doesn't seem at all useful. Yeah, it is my fault, I did not mentioned that this patch is not final. It is only small part of whole implementation. I wanted to show just this, because I think that is the part that should not change much. And to show I did something, I am not ignoring GSoC. Now I can fully focus on the program. Most of the problems you mentioned (except pg_dump) I have implemented and I will post it to HACKERS soon. Until now I've not had much time, because I just finished my BSc. studies yesterday. And again, sorry for misunderstanding. Pavel Baros Some other problems: - The command tag for CREATE MATERIALIZED VIEW should return CREATE MATERIALIZED VIEW rather than CREATE VIEW, since we're treating it as a separate object type. I note that dropping a materialized view already uses DROP MATERIALIZED VIEW, so right now it isn't symmetrical. - Using \d with no argument doesn't list materialized views. - Using \d with a materialized view as an argument doesn't work properly - the first line says something like ?m? public.m instead of materialized view public.m. - Using \d+ with a materialized view as an argument should probably should the view definition. - Using \dd doesn't list comments on materialized views. - Commenting on a column of a materialized view should probably be allowed. - pg_dump fails with a message like this: failed sanity check, parent table OID 24604 of pg_rewrite entry OID 24607 not found - ALTER MATERIALIZED VIEW name OWNER TO role, RENAME TO role, and SET SCHEMA schema either fall to work or fail to parse (plan ALTER VIEW also doesn't work on a materialized view) - ALTER MATERIALIZED VIEW name SET/DROP DEFAULT also doesn't work, which is OK: it shouldn't work. But the error message needs work. - The error message CREATE OR REPLACE on materialized view is not support! shouldn't end with an exclamation point. Do we see supporting the creation of a materialized view from a regular view, as in ALTER VIEW regular_view SET MATERIALIZED or some such? Since we're treating this as a distinct object type, instead of repeatedly typing MATERIALIZED VIEW, is there a possibility of introducing a keyword alias MATVIEW without complicating the grammar/code all that much, or is that frowned upon? Paintbrushes, anyone? Regards, David -- David Christensen End Point Corporation da...@endpoint.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] How to pass around collation information
On Jun 3, 2010, at 2:43 AM, Peter Eisentraut wrote: On ons, 2010-06-02 at 16:56 -0400, Robert Haas wrote: But in the end the only purpose of setting it on a column is to set which one will be used for operations on that column. And the user might still override it for a particular query. Of course. I'm just saying that it *can* be useful to attach a collation to a column definition, rather than only allowing it to be specified along with the sort operation. How does collation relate to per-table/column encodings? For that matter, are per-table/column encodings spec, and/or something that we're looking to implement down the line? Regards, David -- David Christensen End Point Corporation da...@endpoint.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] Tags missing from GIT mirror?
On May 12, 2010, at 1:43 PM, Magnus Hagander wrote: On Wed, May 12, 2010 at 8:27 PM, Florian Pflug f...@phlo.org wrote: Hi I just tried to checkout REL9_0_BETA1 from my local clone of the GIT repository at git.postgresql.org and discovered that none of the tags from CVS seem to exist in there. For alpha1 to alpha1 each tag is accompanied by a corresponding brach, and those *do* exist on the GIT mirror. For beta1, however, no branch seems to exist, and hence there is no trace of it on the GIT mirror. Why is there a branch plus a tag for alphas, but only a tag for betas? I'd have assumed that both would just be tags, but obviously I'm missing something there... I think we branched the alphas so we could version-stamp them while keeping HEAD as 9.0devel. But when we switch to beta, we don't have a devel tree anymore, it's just beta and backbranches. Is there anything to do about the missing tags in git? I've wished for those to be available as well. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] Patch for 9.1: initdb -C option
Hackers, Enclosed is a patch to add a -C option to initdb to allow you to easily append configuration directives to the generated postgresql.conf file for use in programmatic generation. In my case, I'd been creating multiple db clusters with a script and would have specific overrides that I needed to make. This patch fell out of the desire to make this a little cleaner. Please review and comment. From the commit message: This is a simple mechanism to allow you to provide explicit overrides to any GUC at initdb time. As a basic example, consider the case where you are programmatically generating multiple db clusters in order to test various configurations: $ for cluster in 1 2 3 4 5 6; do initdb -D data$cluster -C port = 1234$cluster -C 'max_connections = 10' -C shared_buffers=1M; done A possible future improvement would be to provide some basic formatting corrections to allow specificications such as -C 'port 1234', -C port=1234, and -C 'port = 1234' to all be ultimately output as 'port = 1234' in the final output. This would be consistent with postmaster's parsing. The -C flag was chosen to be a mnemonic for config. Regards, David -- David Christensen End Point Corporation da...@endpoint.com 0001-Add-C-option-to-initdb-to-allow-invocation-time-GUC-.patch Description: Binary data initdb-dash-C.diff 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] SQL compatibility reminder: MySQL vs PostgreSQL
On Mar 8, 2010, at 9:16 AM, Kevin Grittner wrote: Robert Haas robertmh...@gmail.com wrote: Wolfgang Wilhelm wolfgang20121...@yahoo.de wrote: Isn*t that a good time to think to put that question into the list of things PostgreSQL doesn*t want to do? Yes. Done. http://wiki.postgresql.org/wiki/Todo#Features_We_Do_Not_Want Does this conflict conceptually with the item from Exotic Features on the same page?: * Add pre-parsing phase that converts non-ISO syntax to supported syntax This could allow SQL written for other databases to run without modification. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] Explicit psqlrc
On Mar 7, 2010, at 9:22 AM, Tom Lane wrote: Magnus Hagander mag...@hagander.net writes: 2010/3/6 Tom Lane t...@sss.pgh.pa.us: The analogy I was thinking about was psql -X, but I agree that it's not obvious why this shouldn't be thought of as an additional -f file. Uh, I don't follow. When we use -f, we'll run the script and then exit. The whole point is to run it and *not* exit, since you are normally using it to set up the environment in psql. If we were going to support multiple -f options, it would be sensible to interpret -f - as read from stdin until EOF. Then you could interleave prepared scripts and stdin, which could be pretty handy. The default behavior would be equivalent to a single instance of -f -, and what you are looking for would be -X -f substitute-psqlrc -f -. Here's an initial stab at supporting multiple -f's (not counting the interpretation of -f - as STDIN). There are also a few pieces that are up for interpretation, such as the propagation of the return value of the MainLoop(). Also, while this patch supports the single- transaction mode, it does so in a way that will break if one of the scripts include explicit BEGIN/COMMIT statements (although it is no different than the existing code in this regard). Regards, David -- David Christensen End Point Corporation da...@endpoint.com psql_process_multiple_files.patch 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] Explicit psqlrc
On Mar 4, 2010, at 4:00 PM, Magnus Hagander wrote: I've now for the second time found myself wanting to specify an explicit psqlrc file instead of just parsing ~/.psqlrc, so attached is a patch that accepts the PSQLRC environment variable to control which psqlrc file is used. Any objections to this (obviously including documentation - this is just the trivial code) My bikeshed has a --psqlrc path/to/file, but +1 on the idea. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] tie user processes to postmaster was:(Re: [HACKERS] scheduler in core)
On Feb 22, 2010, at 5:22 PM, Jaime Casanova wrote: On Mon, Feb 22, 2010 at 4:37 PM, Tom Lane t...@sss.pgh.pa.us wrote: Dimitri Fontaine dfonta...@hi-media.com writes: Tom Lane t...@sss.pgh.pa.us writes: This seems like a solution in search of a problem to me. The most salient aspect of such processes is that they would necessarily run as the postgres user The precedent are archive and restore command. They do run as postgres user too, don't they? Well, yeah, but you *must* trust those commands because every last bit of your database content passes through their hands. That is not an argument why you need to trust a scheduling facility --- much less the tasks it schedules. Ok, let's forget the scheduler for a minute... this is not about that anymore, is about having the ability to launch user processes when the postmaster is ready to accept connections, this could be used for launching an scheduler but also for launching other tools (ie: pgbouncer, slon daemons, etc) Just a few questions off the top of my head: What are the semantics? If you launch a process and it crashes, is the postmaster responsible for relaunching it? Is there any additional monitoring of that process it would be expected to do? What defined hooks/events would you want to launch these processes from? If you have to kill a backend postmaster, do the auxiliary processes get killed as well, and with what signal? Are they killed when you stop the postmaster, and are they guaranteed to have stopped at this point? Can failing to stop prevent/delay the shutdown/restart of the server? Etc. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] PGXS: REGRESS_OPTS=--load-language=plpgsql
On Feb 20, 2010, at 5:16 PM, Bruce Momjian wrote: Robert Haas wrote: On Sat, Feb 20, 2010 at 5:53 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Sat, Feb 20, 2010 at 2:51 PM, Bruce Momjian br...@momjian.us wrote: Well, I was asking why you labeled it must fix rather than should fix. ?I am fine with the pg_regress.c change. Yeah, if it makes life easier for other people, I say we go for it. I don't think that the way to fix this is to have an ugly kluge in pg_dump and another ugly kluge in pg_regress (and no doubt ugly kluges elsewhere by the time all the dust settles). IMO, the non-ugly kludges are (1) implement CREATE OR REPLACE LANGUAGE and (2) revert the original patch. Do you want to do one of those (which?) or do you have another idea? For #2, if you mean the pg_dump.c plpgsql hack for pg_migrator, that is not an option unless you want to break pg_migrator for 9.0. If you implement #1, why would you have pg_dump issue CREATE OR REPLACE LANGUAGE? We don't do the OR REPLACE part for any other object I can think of, so why would pg_dump do it for languages by default? In what cases would one be able to meaningfully REPLACE a language, other than to not break when encountering an already installed language? i.e., in which cases would this not invalidate functions already written if you were changing from trusted to untrusted status or a different call handler, etc. If there is not a meaningful case for the OR REPLACE, and it is just a syntactic loophole to allow the errorless recreation of an existing language and if the parameters for the CREATE LANGUAGE call indicate identical final state, why aren't we free change change the semantics of CREATE LANGUAGE to just issue a NOTIFY instead of an error in that case, and only complain if there are differences in the call handler, trusted status, etc? I am including a preliminary patch to implement this behavior in the pg_pltemplate case; since we are already using the defaults from that entry and ignoring any explicitly provided ones in the command, this seems to be a safe assumption. Presumably you could do the same in the other case, if you verified that the existing pg_language tuple had the same relevant fields (i.e., notify without error). This would have the benefit of allowing CREATE LANGUAGE langname for those languages with pg_pltemplate entries (specifically plpgsql, but any with the same parameters) and would mean that we could use dumps from pre 9.0 in 9.0 without breaking, appears to fix --single, the pg_regress case, etc. Thoughts on the approach? Regards, David -- David Christensen End Point Corporation da...@endpoint.com skip-create-lang-dupe.patch 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] Avoiding bad prepared-statement plans.
On Feb 18, 2010, at 2:19 PM, Pierre C wrote: What about catching the error in the application and INSERT'ing into the current preprepare.relation table? The aim would be to do that in dev or in pre-prod environments, then copy the table content in production. Yep, but it's a bit awkward and time-consuming, and not quite suited to ORM-generated requests since you got to generate all the plan names, when the SQL query itself would be the most convenient unique identifier... A cool hack would be something like that : pg_execute( SELECT ..., arguments... ) By inserting a hook which calls a user-specified function on non- existing plan instead of raising an error, this could work. However, this wouldn't work as-is since the plan name must be = NAMEDATALEN, but you get the idea ;) How about the SHA1 hash of the query? Hey, it works for git... :-) Regards, David -- David Christensen End Point Corporation da...@endpoint.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] Function to return whole relation path?
On Feb 7, 2010, at 11:04 AM, Tom Lane wrote: In connection with the relation-mapping patch I proposed a function pg_relation_filenode(regclass) returns oid which client code would need to use instead of looking at pg_class.relfilenode, if it wants to get a number that will be meaningful for mapped system catalogs. I still think we need that, but while thinking about how it'd be used, I wondered if it wouldn't be far more useful to provide pg_relation_filepath(regclass) returns text which would expose the output of relpath(), ie, the $PGDATA-relative path name of the relation. So you'd get something like base/58381/92034 or pg_tblspc/48372/8.5_201002061/68483/172744. For clients that are actually trying to match up files with tables, this would avoid having to essentially duplicate the knowledge embedded in relpath(). In particular, the recent decision to include catversion in tablespace subdirectories is going to be a significant PITA for such clients, as there is no easy way to discover catversion by asking the backend. I don't see any security issue here, since this wouldn't give you any information that's not readily available (like absolute pathnames on the server) --- but it would avoid code duplication. Objections, better ideas? Should this return multiple values (text[] or SETOF text) for tables which wrapped over the single file-limits (1GB, IIRC)? So: pg_tblspc/ 48372/8.5_201002061/68483/172744, pg_tblspc/ 48372/8.5_201002061/68483/172744.1, etc? Regards, David -- David Christensen End Point Corporation da...@endpoint.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] Function to return whole relation path?
On Feb 7, 2010, at 1:30 PM, Tom Lane wrote: David Christensen da...@endpoint.com writes: On Feb 7, 2010, at 11:04 AM, Tom Lane wrote: pg_relation_filepath(regclass) returns text which would expose the output of relpath(), ie, the $PGDATA-relative path name of the relation. Should this return multiple values (text[] or SETOF text) for tables which wrapped over the single file-limits (1GB, IIRC)? So: pg_tblspc/ 48372/8.5_201002061/68483/172744, pg_tblspc/ 48372/8.5_201002061/68483/172744.1, etc? No, I'm not inclined to go there. The set of actually existing segments seems too volatile; and anyone worried about this probably can add a star to the end of the pathname ... True, although it'd need to be more refined than just *, as you'd need to be careful to only pick up those related to the actual relid: 172744, 172744.1, etc, and not those with a common prefix: 1727441, 1727441.1, etc. (common prefix). If that needs to be someone else's problem, makes sense here. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] Patch: psql \whoami option
On Jan 27, 2010, at 4:01 AM, Magnus Hagander wrote: 2010/1/27 Josh Berkus j...@agliodbs.com: On 1/26/10 3:24 PM, David Christensen wrote: -hackers, In the spirit of small, but hopefully useful interface improvement patches, enclosed for your review is a patch for providing psql with a \whoami command (maybe a better name is \conninfo or similar). Its purpose is to print information about the current connection, by default in a human-readable format. There is also an optional format parameter which currently accepts 'dsn' as an option to output the current connection information as a DSN. On a first note, it seems like the check for the parameter dsn isn't complete. Without testing it, it looks like it would be possible to run \whoami foobar, which should give an error. Yeah, I debated that; right now, it just ignores any output it doesn't know about and spits out the human-readable format. oooh, I could really use this. +1 to put it in 9.1-first CF. however, \conninfo is probably the better name. And what about a +1 on that name. That makes at least three, including me. :-) postgresql function version for non-psql connections? How could that function possibly know what the connection looks like from the client side? Think NAT, think proxies, think connection poolers. Yes, this doesn't seem to be a feasible thing to detect in all (many?) cases. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] Patch: psql \whoami option
On Jan 27, 2010, at 5:23 AM, Martin Atukunda wrote: How about using the psql prompt to convey this information? IIRC the psql prompt can be configured to show the hostname, server, port and other fields. Wouldn't this be enough? or am I missing something? Prompt customization is certainly something that could be done (and I use in my .psqlrc), but consider someone unaware of the psql prompt customization or people who are not using their own setup/account, etc. This is a command that could be useful for anyone; as experts, we tend to miss some of the holes in the current interfaces. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] Patch: psql \whoami option
On Jan 27, 2010, at 8:08 AM, Magnus Hagander wrote: 2010/1/27 David Christensen da...@endpoint.com: On Jan 27, 2010, at 4:01 AM, Magnus Hagander wrote: 2010/1/27 Josh Berkus j...@agliodbs.com: On 1/26/10 3:24 PM, David Christensen wrote: -hackers, In the spirit of small, but hopefully useful interface improvement patches, enclosed for your review is a patch for providing psql with a \whoami command (maybe a better name is \conninfo or similar). Its purpose is to print information about the current connection, by default in a human-readable format. There is also an optional format parameter which currently accepts 'dsn' as an option to output the current connection information as a DSN. On a first note, it seems like the check for the parameter dsn isn't complete. Without testing it, it looks like it would be possible to run \whoami foobar, which should give an error. Yeah, I debated that; right now, it just ignores any output it doesn't know about and spits out the human-readable format. yeah, that's not very forwards-compatible. Someone uses it in the wrong way, and suddenly their stuff gets broken if we choose to modify it in the future. If we say we're only going ot accept two options, let's enforce that and show an error/help message if the user typos. That's a good point about forward-compatibility. In that case, I'm not sure if default is the best name for the human-readable format, but I didn't like human-readable ;-). I assume that should have an explicit spelling, and not just be the format that we get if we don't otherwise specify. Ideas, anyone? Regards, David -- David Christensen End Point Corporation da...@endpoint.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] Patch: psql \whoami option
-hackers,
In the spirit of small, but hopefully useful interface improvement
patches, enclosed for your review is a patch for providing psql with a
\whoami command (maybe a better name is \conninfo or similar). Its
purpose is to print information about the current connection, by
default in a human-readable format. There is also an optional format
parameter which currently accepts 'dsn' as an option to output the
current connection information as a DSN.
Example output:
$psql -d postgres -p 8555
psql (8.5devel)
You are now connected to database postgres.
[Tue Jan 26 17:17:31 CST 2010]
machack:postgres:8555=# \whoami
Connected to database: postgres, user: machack, port: 8555
via local domain socket
[Tue Jan 26 17:17:34 CST 2010]
machack:postgres:8555=# \c - - localhost 8555
psql (8.5devel)
You are now connected to database postgres on host localhost.
[Tue Jan 26 17:17:42 CST 2010]
machack:postgres:8555=# \whoami
Connected to database: postgres, user: machack, host:
localhost, port: 8555
[Tue Jan 26 17:17:46 CST 2010]
machack:postgres:8555=# \whoami dsn
dbname=postgres;user=machack;host=localhost;port=8555
[Tue Jan 26 17:19:02 CST 2010]
machack:postgres:8555=# \q
Regards,
David
--
David Christensen
End Point Corporation
da...@endpoint.com
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-
ref.sgml
index 3ce5996..b58b24d 100644
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
*** lo_import 152801
*** 2149,2154
--- 2149,2167
varlistentry
+ termliteral\whoami/literal [ replaceable
class=parameterdefault/replaceable | replaceable
class=parameterdsn/replaceable ] /term
+ listitem
+ para
+ Outputs connection information about the current database
+ connection. When passed parameter literaldsn/literal,
+ outputs as a DSN. If parameter is unspecified or
+ unrecognized, outputs in a human-readable format.
+ /para
+ /listitem
+ /varlistentry
+
+
+ varlistentry
termliteral\x/literal/term
listitem
para
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 5188b18..21b2468 100644
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
*** exec_command(const char *cmd,
*** 1106,
--- 1106,1156
free(fname);
}
+ /* \whoami -- display information about the current connection */
+ else if (strcmp(cmd, whoami) == 0)
+ {
+ char *format = psql_scan_slash_option(scan_state,
+
OT_NORMAL, NULL, true);
+ char *host = PQhost(pset.db);
+
+ if (format !pg_strcasecmp(format, dsn)) {
+ if (host) {
+ printf(dbname=%s;user=%s;host=%s;port=%s\n,
+ PQdb(pset.db),
+ PQuser(pset.db),
+ host,
+ PQport(pset.db)
+ );
+ }
+ else {
+ printf(dbname=%s;user=%s;port=%s\n,
+ PQdb(pset.db),
+ PQuser(pset.db),
+ PQport(pset.db)
+ );
+ }
+ }
+ else {
+ /* default case */
+ if (host) {
+ printf(Connected to database: \%s\, user: \%s\, host: \%s
\, port: \%s\\n,
+ PQdb(pset.db),
+ PQuser(pset.db),
+ host,
+ PQport(pset.db)
+ );
+ }
+ else {
+ printf(Connected to database: \%s\, user: \%s\, port: \%s
\ via local domain socket\n,
+ PQdb(pset.db),
+ PQuser(pset.db),
+ PQport(pset.db)
+ );
+ }
+ }
+ free(format);
+ }
+
/* \x -- toggle expanded table representation */
else if (strcmp(cmd, x) == 0)
{
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index 6037351..802b76d 100644
*** a/src/bin/psql/help.c
--- b/src/bin/psql/help.c
*** slashUsage(unsigned short int pager)
*** 249,254
--- 249,256
PQdb(pset.db));
fprintf(output, _( \\encoding [ENCODING
[HACKERS] Patch: regschema OID type
Hey -hackers, Enclosed is a patch adding a 'regschema' OID type. I'm really just hoping to get this out there, don't worry about committing it at this point. This is something that I've always wanted in the field (yes, I'm lazy). Many thanks to RhodiumToad for pointers about the necessary system table entries and general advice. Example usage: machack:postgres:8555=# select relnamespace::regschema, relname from pg_class limit 10; relnamespace| relname +-- pg_catalog | pg_type pg_catalog | pg_attribute information_schema | foreign_data_wrapper_options information_schema | foreign_data_wrappers information_schema | _pg_foreign_servers information_schema | foreign_server_options information_schema | foreign_servers information_schema | _pg_user_mappings information_schema | user_mapping_options information_schema | user_mappings (10 rows) It uses the same quoting mechanism as regclass, and I've tested against some odd schema names such as fooschema; I updated the docs as I was able, but am not familiar enough with the regression tests to add those yet. I hope to address that in a future revision. Thanks, David -- David Christensen End Point Corporation da...@endpoint.com regschema.patch 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] MySQL-ism help patch for psql
On Jan 21, 2010, at 11:48 AM, Florian Weimer wrote: * David Christensen: Currently, a session will look like the following: machack:machack:5485=# show tables; See: \d or \? for general help with psql commands machack:machack:5485=# Said formatting looks like it could use some improvement, open to suggestions, but something on a single line seems more useful. You should at least make clear that this is an error message due to an unsupported command. The output above looks broken. Something like this should be okay, I think: ERROR: unrecognized configuration parameter tables NOTICE: use \d to list tables, or \? for general help with psql commands ERROR: unrecognized configuration parameter databases NOTICE: use \l to list databases, or \? for general help with psql commands That's a very good point as far as the visibility is concerned. Should the error messages between the SHOW cases and the others be consistent (ERROR: unsupported command or similar)? It's worth noting that this is only in the psql client, but we could simulate the ereport output from the server. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] Patch: regschema OID type
On Jan 21, 2010, at 11:56 AM, Tom Lane wrote: David Christensen da...@endpoint.com writes: Enclosed is a patch adding a 'regschema' OID type. What in the world is the point of that? The regfoo types are for things that have schema-qualified names. Perhaps the naming is a bit disingenuous, and I'm not tied to it; I like the ability to translate between oid - name that regclass, regproc, etc. provide. This simplifies query lookups and manual examination of the system tables and for me at least fills a need. Do you have a better type name? Regards, David -- David Christensen End Point Corporation da...@endpoint.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] MySQL-ism help patch for psql
On Jan 21, 2010, at 12:02 PM, Tom Lane wrote: David Christensen da...@endpoint.com writes: Should the error messages between the SHOW cases and the others be consistent (ERROR: unsupported command or similar)? It's worth noting that this is only in the psql client, but we could simulate the ereport output from the server. No. Not unless you want to simulate it to the point of honoring the different verbosity settings, which would greatly expand the size of the patch. We do not try to make the response to help look like an error message, and I don't see the value of doing so here either. (I think Florian's real problem with the proposed output is that it's ugly, which I agree with --- the formatting is strange.) I'm with you on that one; I tried to address that in the second revision of the patch. But I'm definitely open to suggestions. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] MySQL-ism help patch for psql
Hey -hackers,
I whipped up a quick patch for supporting some of the common mysql-
based meta commands; this is different than some things which have
been discussed in the past, in that it provides just a quick direction
to the appropriate psql command, not an actual alternative syntax for
the same action. This is not intended to be comprehensive, but just
to provide proper direction
The changes are in a single hunk touching only src/bin/psql/
mainloop.c; I modeled the code against the logic currently in place
for the help command.
First postgres patch, so bring it on^W^W^Wbe gentle. I obviously
don't expect this to not promote a wild debate/flamewar... ;-) The
formatting and specific wording for the various messages are totally
up-in-the-air, and gladly up for debate.
Regards,
David
--
David Christensen
End Point Corporation
da...@endpoint.com
diff --git a/src/bin/psql/mainloop.c b/src/bin/psql/mainloop.c
index e2914ae..cc89728 100644
--- a/src/bin/psql/mainloop.c
+++ b/src/bin/psql/mainloop.c
@@ -197,6 +197,48 @@ MainLoop(FILE *source)
continue;
}
+#define MYSQL_HELP_CHECK(o) \
+ (pg_strncasecmp(line, (o), strlen(o)) == 0 \
+ (line[strlen(o)] == '\0' || line[strlen(o)] == ';' ||
isspace((unsigned char) line[strlen(o)])))
+
+#define MYSQL_HELP_OUTPUT(o) \
+ free(line);\
+ printf(_(See:\n\
+o\
+\n\
+ or \\? for general
help with psql commands\n));\
+ fflush(stdout);\
+ continue;
+
+ /* Present the Postgres equivalent common mysqlisms */
+ if (pset.cur_cmd_interactive query_buf-len == 0)
+ {
+ if (MYSQL_HELP_CHECK(use))
+ {
+ MYSQL_HELP_OUTPUT(\\c database);
+ }
+ else if (MYSQL_HELP_CHECK(show tables))
+ {
+ MYSQL_HELP_OUTPUT(\\dt);
+ }
+ else if (MYSQL_HELP_CHECK(source))
+ {
+ MYSQL_HELP_OUTPUT(\\i filename);
+ }
+ else if (MYSQL_HELP_CHECK(show databases))
+ {
+ MYSQL_HELP_OUTPUT(\\l);
+ }
+ else if (MYSQL_HELP_CHECK(describe))
+ {
+ MYSQL_HELP_OUTPUT(\\d tablename);
+ }
+ else if (MYSQL_HELP_CHECK(load data infile))
+ {
+ MYSQL_HELP_OUTPUT(\\copy);
+ }
+ }
+
/* echo back if flag is set */
if (pset.echo == PSQL_ECHO_ALL !
pset.cur_cmd_interactive)
puts(line);
--
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] MySQL-ism help patch for psql
The previous discussion started from the idea that only DESCRIBE, SHOW DATABASES/TABLES, and USE were worth worrying about. If we were to agree that we'd go that far and no farther, the potential conflict with SQL syntax would be pretty limited. I have little enough experience with mysql to not want to opine too much on how useful that would be, but it does seem like those are commands I use right away anytime I am using mysql. I have no problems paring down the list of cases; these were the correspondences I saw off the top of my head. I definitely don't want to conflict with any SQL syntax. The exact wording/output of the messages can be adjusted at whim. Regards, David -- David Christensen End Point Corporation da...@endpoint.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] MySQL-ism help patch for psql
On Jan 19, 2010, at 2:58 PM, Stefan Kaltenbrunner wrote: Tom Lane wrote: Jeff Davis pg...@j-davis.com writes: That being said, I don't have much of an opinion, so if you see a problem, then we can forget it. After all, we would need some kind of a prefix anyway to avoid conflicting with actual SQL... maybe \m? And that defeats a lot of the purpose. Yeah, requiring a prefix would make it completely pointless I think. The submitted patch tries to avoid that by only matching syntax that's invalid in Postgres, but that certainly limits how far we can go with it. (And like you, I'm a bit worried about the LOAD case.) yeah requiring a prefix would make it completely pointless Agreed. The last go-round on this was just a couple months ago: http://archives.postgresql.org/pgsql-bugs/2009-11/msg00241.php although I guess that was aimed at a slightly different idea, namely making show databases etc actually *work*. This one at least has a level of complication that's more in keeping with the possible gain. well providing a hint that one should use different command will only lead to the path uhm why not make it work as well - and we also need to recongnized that our replacements for some of those commands are not really equivalent in most cases. I think if you set this line ahead of time, you don't have to worry about the detractors; this is equivalent to vim outputting Type :quitEnter to exit Vim when you type emacs' quit sequence. The intent is to show the correct way or to provide a helpful reminder to people new to psql, not to make it work the same. The previous discussion started from the idea that only DESCRIBE, SHOW DATABASES/TABLES, and USE were worth worrying about. If we were to agree that we'd go that far and no farther, the potential conflict with SQL syntax would be pretty limited. I have little enough experience with mysql to not want to opine too much on how useful that would be, but it does seem like those are commands I use right away anytime I am using mysql. well those are the most common ones I guess for the current version of the mysql commandline client - but what about future versions or the fact that we only have partial replacements for some of those that people are really asking for? I think it captures the intent of the people using the tool, and that it adds a small net benefit in usability for those people. Deciding to support this small subset does not obligate you to expand the scope in the future. (Hey ma, this slope ain't slippery!) Regards, David -- David Christensen End Point Corporation da...@endpoint.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] MySQL-ism help patch for psql
On Jan 19, 2010, at 3:12 PM, Andrew Dunstan wrote:
David Christensen wrote:
+ if (MYSQL_HELP_CHECK(use))
+ {
+ MYSQL_HELP_OUTPUT(\\c database);
+ }
[snip]
+ else if (MYSQL_HELP_CHECK(load data
infile))
+ {
+ MYSQL_HELP_OUTPUT(\\copy);
+ }
Quite apart from any considerations covered by other people, these
two at least could be positively misleading ... the psql commands
are not exact equivalents of the MySQL commands, AIUI.
The \copy could definitely be considered a stretch; I know \c supports
more options than the equivalent USE, but isn't it a proper superset
of functionality?
Regards,
David
--
David Christensen
End Point Corporation
da...@endpoint.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] MySQL-ism help patch for psql
On Jan 19, 2010, at 3:39 PM, Tom Lane wrote: David Christensen da...@endpoint.com writes: On Jan 19, 2010, at 2:58 PM, Stefan Kaltenbrunner wrote: well those are the most common ones I guess for the current version of the mysql commandline client - but what about future versions or the fact that we only have partial replacements for some of those that people are really asking for? I think it captures the intent of the people using the tool, and that it adds a small net benefit in usability for those people. Deciding to support this small subset does not obligate you to expand the scope in the future. (Hey ma, this slope ain't slippery!) I thought Magnus had a really good point: covering these four cases will probably be enough to teach newbies to look at psql's backslash commands. And once they absorb that, we're over a big hump. Okay, so I can revise the code to cover those four cases specifically (or three, depending); is there any specific feedback as to the output/ formatting of the messages themselves? Currently, a session will look like the following: machack:machack:5485=# show tables; See: \d or \? for general help with psql commands machack:machack:5485=# Said formatting looks like it could use some improvement, open to suggestions, but something on a single line seems more useful. Also, TTBOMK these commands have been in mysql for many years. I don't think that commands that might get introduced in future versions would have anywhere near the same degree of wired-into-the-fingertips uptake to them. They were in there since when I last used mysql on a regular basis, so going on 10 years now. i.e., pretty safe, and pretty engrained in muscle-memory. Regards, David -- David Christensen End Point Corporation da...@endpoint.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
