Re: dblink patches for comment

The attached addresses items#2 and 3 as listed by Bruce here:
http://momjian.us/cgi-bin/pgsql/joe

I think it is consistent with the discussions we had a PGCon last week.
Any objections to me committing this for 8.4?

On a side note, should I try to address items #1 & #4 for 8.4 as well?
Perhaps #4 yes since it is arguably a bug fix, but no to #1?

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> The attached addresses items#2 and 3 as listed by Bruce here:
> http://momjian.us/cgi-bin/pgsql/joe

> I think it is consistent with the discussions we had a PGCon last week.
> Any objections to me committing this for 8.4?

It's hard to review it without any docs that say what it's supposed to do.
(And you'd need to patch the docs anyway, eh?)

> On a side note, should I try to address items #1 & #4 for 8.4 as well?
> Perhaps #4 yes since it is arguably a bug fix, but no to #1?

Yeah, my feeling too. #1 is a new feature that was submitted too late
for 8.4. I wouldn't have argued if you'd committed it anyway during
the commitfest, but it's definitely too late now. But #4 seems like
a bugfix.

regards, tom lane

Tom Lane wrote:
> It's hard to review it without any docs that say what it's supposed to do.
> (And you'd need to patch the docs anyway, eh?)

Fair enough :-)

Probably better if I break this up in logical chunks too. This patch
only addresses the refactoring you requested here:
http://archives.postgresql.org/message-id/28719.1230996378@sss.pgh.pa.us

I'll follow up later today with a SQL/MED only patch which includes docs.

Joe

Tom Lane wrote:
> It's hard to review it without any docs that say what it's supposed to do.
> (And you'd need to patch the docs anyway, eh?)

Here's a much simpler SQL/MED support patch for dblink.

This enforces security in the same manner for FOREIGN SERVER connections
as that worked out over time for other dblink connections. Essentially,
the FOREIGN SERVER and associated user MAPPING provides the needed info
for the libpq connection, but otherwise behavior is the same.

I've also attached a doc patch.

Comments?

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> Probably better if I break this up in logical chunks too. This patch
> only addresses the refactoring you requested here:
> http://archives.postgresql.org/message-id/28719.1230996378@sss.pgh.pa.us

This looks sane to me in a quick once-over, though I've not tested it.

A small suggestion for future patches: don't bother to reindent code
chunks that aren't changing --- it just complicates the diff with a
lot of uninteresting whitespace changes. You can either do that after
review, or leave it to be done by pgindent. (Speaking of which, we
need to schedule that soon...)

regards, tom lane

Joe Conway <mail(at)joeconway(dot)com> writes:
> Here's a much simpler SQL/MED support patch for dblink.

> This enforces security in the same manner for FOREIGN SERVER connections
> as that worked out over time for other dblink connections. Essentially,
> the FOREIGN SERVER and associated user MAPPING provides the needed info
> for the libpq connection, but otherwise behavior is the same.

> I've also attached a doc patch.

The docs patch looks okay, except this comment is a bit hazy:

> + -- Note: local connection must require authentication for this to work properly

I think what it means is

> + -- Note: local connection must require password authentication for this to work properly

If not, please clarify some other way. It might also be good to be a
bit more clear about what "fail to work properly" might entail.

As far as the code goes, hopefully Peter will take a look since he's
spent more time on the SQL/MED code than I have. The only thing I can
see that looks bogus is that get_connect_string() is failing to handle
any quoting/escaping that might be needed for the values to be inserted
into the connection string. I don't recall offhand what rules libpq
has for that, but I hope it at least implements doubled single quotes...

regards, tom lane

Tom Lane wrote:
> Joe Conway <mail(at)joeconway(dot)com> writes:
>> Probably better if I break this up in logical chunks too. This patch
>> only addresses the refactoring you requested here:
>> http://archives.postgresql.org/message-id/28719.1230996378@sss.pgh.pa.us
>
> This looks sane to me in a quick once-over, though I've not tested it.

Thanks -- committed.

> A small suggestion for future patches: don't bother to reindent code
> chunks that aren't changing --- it just complicates the diff with a
> lot of uninteresting whitespace changes. You can either do that after
> review, or leave it to be done by pgindent. (Speaking of which, we
> need to schedule that soon...)

Sorry. "cvs diff -cb" seems to help (see attached). It is half the size
and much more readable.

Joe

Tom Lane wrote:
> The docs patch looks okay, except this comment is a bit hazy:
>
>> + -- Note: local connection must require authentication for this to work properly
>
> I think what it means is
>
>> + -- Note: local connection must require password authentication for this to work properly
>
> If not, please clarify some other way. It might also be good to be a
> bit more clear about what "fail to work properly" might entail.

OK, hopefully the attached is more clear.

> As far as the code goes, hopefully Peter will take a look since he's
> spent more time on the SQL/MED code than I have. The only thing I can
> see that looks bogus is that get_connect_string() is failing to handle
> any quoting/escaping that might be needed for the values to be inserted
> into the connection string. I don't recall offhand what rules libpq
> has for that, but I hope it at least implements doubled single quotes...

Added quote_literal_cstr() around the connection string params. Also
found I needed to restrict the servername string length to NAMEDATALEN
in order to avoid an assert if a full connection string is passed to
dblink_connect().

Other comments?

Thanks,

Joe