Re: Greenplum patch as a diff file

Dave,

My apologies for making extra work for you. I've been trying to work on several different projects, all with deadlines happening at nearly the same time, and am still learning the pgAdmin code, so things take me longer than they otherwise would. And our QA team is just getting ready to start testing my pgAdmin changes.

> I've committed the revised patch, though I did need to make some
> fairly extensive changes to get it into an acceptable state. I still
> consider it to be fairly incomplete feature-wise, though it is an
> improvement over the previous Greenplum support, and I think all the
> nasty gotchas have now been cleaned up.

Yes, it is very much not feature complete. We had our schedule set to have a first beta-ready version in about a month from now, not realizing the pgAdmin beta schedule. So this is somewhat of a rush job.

>
> In the interests of preventing the same problems occuring again,
> here's what I changed:
>
> - Changes to hbaConfigFileFactory::StartDialog and
> mainConfigFileFactory::StartDialog were removed. The changes made
> changed the behaviour of them into hbaConfigFactory::StartDialog and
> mainConfigFactory::StartDialog. The former are intended to open config
> files from the local filesystem, the latter using pg_file_read() via a
> connection to a server.

Ok, how do I get the menu to use the latter versions when someone asked to open a config file?
In the Greenplum DB case, there are no config files on the machine running pgAdmin. The config files are all on the server, which the user doesn't have access to except through libpq.

>
> - I added a gpPartition::Refresh function so the treeview could
> refresh properly in the even of a change to a partition object.
>

Thanks... I hadn't put that in yet, since right now you can't change them, but had planned to do this before enabling changing them.

> - Added code to gpResQueue::ShowTreeDetail to display the thresholds
> and auto commit setting in the properties list for resource queues.
>
> - Fixed the graphical explain to allow it to render a plan with nodes
> with an average cost of zero. This doesn't happen in
> PostgreSQL/EnterpriseDB (at least without some pretty funky
> non-standard tuning parameters), but does on Greenplum with a brand
> new empty table which caused an arrow to be rendered such that it
> obscured the entire plan.

Wow... I never thought to test this case!

>
> - Fixed dlgExtTable:
> * When viewing an existing table, display the definition.
> * Remove 'AS' from the generated SQL, which is not part of the
> Greenplum syntax.
> * Fix the ALTER TABLE ... SET OWNER statement (ALTER EXTERNAL
> TABLE is not supported).
> * Fix the dialogue so it doesn't think there has been a change to
> the object when nothing has been modified.
> * Fix the COMMENT ON statement (COMMENT ON EXTERNAL TABLE is not
> supported).
> * Removed the useless Apply button.
>
> - Disabled the default value textbox when viewing the properties of an
> external table column.
>
> - Removed a broken sub-collection of external tables form under an
> external table node (copy 'n' paste error??)
>
> - Prevented the addition of columns to external tables.
>
> - Removed dlgPartition entirely. It was a 2000+ line copy of dlgTable,
> with the buttons to add/remove columns removed!?!?! (that's completely
> unacceptable from a maintenance perspective) This appears to have been
> in response to my comments previously about removing or fixing the
> options to add columns to partitions - however you seem to have fixed
> those options if selected from the treeview, so currently they are
> also available on dlgTable which is once again used for partitions.

Yes, dlgPartition was just a placeholder for the code to allow creating/modifying partitions.
I was thinking it was better to get something that compiles and doesn't fail when run into those modules before the beta, because I was worried you wouldn't accept adding new files after the beta starts.

As soon as I have time, I plan to implement working dlgPartition code to allow adding/changing partitions.

>
> - Added support for PDF helpfiles, which wasn't implemented per my
> previous comments.
>
> - Modfiied gpPartition::GetSql() to allow localisation of the note
> about the DDL, and shorten said note.
>
> I should note that I would normally have bounced this back for
> reworking, particularly as some of these issues could have been picked
> up with basic testing, however in this case I fixed it as I reviewed
> in the interests of not delaying the beta.
>
> --
> Dave Page
> EnterpriseDB UK: http://www.enterprisedb.com

Yes, I understand. Like I said, I'm sorry for the extra work, and we will try to not have that happen in the future.